Two-body jastrow optimization bug fix : Set opt offset to be equal to first active variable #2834
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Detailed disussion on issue QMCPACK#2814
|
Can one of the admins verify this patch? |
|
OK to test |
ye-luo
requested changes
Jan 1, 2021
|
@shivupa Since this is more complicated than we were expecting, should I find someone else to continue this? I know you have your own research to get on with, and on our side this might be the catalyst to setup a broader set of quick to run deterministic optimization tests, check if this error pattern is repeated in other jastrows etc.. Cheers, Paul |
|
Thanks. I tried to take a look, but it is definitely more complicated than I have the bandwidth for right now! I'd appreciate that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Detailed disussion on issue #2814
Proposed changes
The two body jastrow optimization would fail when the first optimizable parameters were not the first parameters (e.g. if the u-u terms were frozen). This implements @markdewing's suggestions from #2814, which fixes the offset variable to point at the correct set of parameters.
Closes #2814
What type(s) of changes does this code introduce?
Delete the items that do not apply
Does this introduce a breaking change?
What systems has this change been tested on?
Laptop Intel i7-7700HQ
Desktop Intel i7-2600S
Checklist
Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.