-
Notifications
You must be signed in to change notification settings - Fork 139
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
J1Spin indexing issue #4612
J1Spin indexing issue #4612
Conversation
for (int j = 0; j < NumTargetGroups; j++) | ||
if (j == target_type && J1UniqueFunctors[igroup * NumTargetGroups + j] == nullptr) | ||
J1UniqueFunctors[igroup * Nelec + j] = std::move(afunc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (int j = 0; j < NumTargetGroups; j++) | |
if (j == target_type && J1UniqueFunctors[igroup * NumTargetGroups + j] == nullptr) | |
J1UniqueFunctors[igroup * Nelec + j] = std::move(afunc); | |
if (J1UniqueFunctors[igroup * NumTargetGroups + target_type] == nullptr) | |
J1UniqueFunctors[igroup * Nelec + target_type] = std::move(afunc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the check use igroup * NumTargetGroups
and the assignment use igroup * Nelec
?
How was this found? |
I hit this in a positron and Be cluster calculation. I am trying now to make a lightweight version of that system to use as a test |
Important catch. Worth studying the J2 and J3 implementations for similar faults once this is resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a unit test?
J1UniqueFunctors[i * NumTargetGroups + j] == nullptr) | ||
J1UniqueFunctors[igroup * Nelec + j] = std::move(afunc); | ||
} | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why loop i is even needed.
J1UniqueFunctors[source_type * NumTargetGroups + target_type]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there were more than one combination of i
and j
then afunc
would be moved multiple times.
Looks like that is happening on line 160 😟
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very true. target_type == -1
case seems broken. That needs to be sorted out when unifying J1 and J1Spin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah not sure what I was doing in the past but this was definitely wrong. target_type
should only be -1
for spin independent J1 so the radial jastrow builder should never call this function with target_type == -1
. I now raise a runtime error if we ever hit that case.
Also, yes, no loop is even needed.
Still needs a test |
No issues in J2/J3 |
02e14d9
to
5b2d327
Compare
Ok I added a test that fails on the current develop only for the cloned version of the Jastrow (which makes sense since I saw this error when performing a Jastrow optimization).
|
Upload coverage step failing in CI. Seems unrelated |
Test this please |
Test this please |
Proposed changes
This indexing in the spin dependent 1 body jastrow is wrong. I need to figure out a test that triggers this.
What type(s) of changes does this code introduce?
Does this introduce a breaking change?
What systems has this change been tested on?
AMD workstation
Checklist