Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed missing subtype.field() bug #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bglaessle
Copy link

Generally if there any issues with my changes, I am happy to provide refined fixes
if I get some guidance related to the code design.

  • the bug appeared because field() member functions where missing
    in template instantations of copymask, random, gaussian ...
  • the fix mostly allows code to be compiled against qdp++, the result should be ok
    for scalar and parscalar builds without threading, but I havent
    run any tests! For threaded builds, it is at least slower, if not wrong
  • I cannot predict the behaviour for jit, parvecscalar, etc builds
    additional fixes might be necessary ...
  • two issues worth mentioning:
    1. it is an architectural choice how threaded RNG is supposed to behave
      (same result as non-threaded? how is this supposed to be enforced?)
    2. currently "random(ferm[subset])" and "random(ferm,subset)" might
      behave differently (same for gaussian)
  • I am unsure about the OSubScalar behaviour, is it correct that
    this is simply supposed to be OScalar again, with no effective Subset?

Benjamin Glaessle added 2 commits November 12, 2014 14:55
* the bug appeared because field() member functions where missing
  in template instantations of copymask, random, gaussian ...

* the fix mostly allows code to be compiled against qdp++, the result should be ok
  for scalar and parscalar builds without threading, but I havent
  run any tests! For threaded builds, it is at least slower, if not wrong

* I cannot predict the behaviour for jit, parvecscalar, etc builds
  additional fixes might be necessary ...

* two issues worth mentioning:
  1. it is an architectural choice how threaded RNG is supposed to behave
     (same result as non-threaded? how is this supposed to be enforced?)
  2. currently "random(ferm[subset])" and "random(ferm,subset)" might
     behave differently (same for gaussian)

* I am unsure about the OSubScalar behaviour, is it correct that
  this is simply supposed to be OScalar again, with no effective Subset?
@bjoo
Copy link
Contributor

bjoo commented May 8, 2017

Is this still an issue in the devel branch?

@bglaessle
Copy link
Author

Commit 36e5dde is already resolved (in devel),
whereas the first commit be03cee can still be applied but I currently don't
know anymore how to trigger the error.
Regardless the previously mentioned "random(ferm[subset])" != "random(ferm,subset)" issue
is still unresolved and threading is definitely slow.
For now do not merge and I will try to reproduce the error within 1-2 weeks.

@bglaessle
Copy link
Author

bglaessle commented May 15, 2017

I checked the devel-branch and the Subtype::field() method in include/qdp_qdpsubtype.h is still commented out, e.g.

LatticeFermion f = zero;
random(f[rb[0]]);

does not compile. Using random(f,rb[0]) instead works and seems to be threaded correctly.
The same applies for gaussian and copymask.

There are therefore have several options:

  • Enable Subtype::field(), the resulting functions which take OSubLattice<T>& arguments should be/are hopefully implemented correctly. Only field() needs to correctly provide the outer lattice instance.
  • Remove function and methods with "OSubLattice&" argument since apparently no one uses them and they are incorrect in the worst case.
  • Or, since be03cee is rather hacky and messes up consistency/threading I would recommend against it and maybe try to write a patch based on new suggestions.

BTW: in scalar and parscalar arch the argument of void random(OSubLattice<T> dd) should be a reference. Again the same applies for gaussian and copymask.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants