Merge DiffOneBodyJastrowOrbital.h into J1OrbitalSoA.h #3243
Conversation
|
Can one of the admins verify this patch? |
|
I noticed some things that need changed on this PR. I'll update here when I finish |
|
Fixed |
|
Thanks. Will wait for @ye-luo comments. |
|
heads up. It takes a bit longer than expected to review this. I found a quite a few issues not necessarily directly caused by this PR but surfaced. |
|
No worries no rush, just let me know if there is anything I should do to help. |
|
@shivupa I fixed the legacy CUDA build but the actual code needs to be updated. Pull before you continue. |
|
Test this please |
shivupa
force-pushed
the
onebodyjasmerge
branch
3 times, most recently
from
7582d4d to
4b6f060
Compare
|
Rebased on develop and ready for review! Thanks @ye-luo |
There was a problem hiding this comment.
the nullptr early bails are quite a code smell. I assume somewhere you don't have to add input to define a functor for each pair or triple and this is a lazy and potentially error prone way to deal with that. sdt::Optional or a no-op functor see a better way to deal with this. It doesn't look right the the J1 jastrow at all.
|
The test does not check the very likely case that for multiple ions and "electron" species some of the Jastrows are not defined in input. From my reading of the code this really needs to be part of the testing of these classes. |
|
^^^ Good point on testing for the missing in input case, and not just for the code in this PR. Most times this would be an error but it could also be intentional. The code should handle all cases. |
|
Thanks for the review I'll start working on these! |
|
Test this please |
|
The current GPU CI failure is actually caused by develop branch. The legacy CUDA build with ASAN enabled already fails. This is the reason why I put up #3280 yesterday. |
|
|
||
| ions_.get(app_log()); | ||
| elec_.get(app_log()); | ||
|
|
||
| const char* jasxml = "<wavefunction name=\"psi0\" target=\"e\"> \ | ||
| <jastrow name=\"J1\" type=\"One-Body\" function=\"Bspline\" print=\"yes\" source=\"ion0\"> \ | ||
| <correlation elementType=\"H\" cusp=\"0.0\" size=\"2\" rcut=\"5.0\"> \ |
There was a problem hiding this comment.
@shivupa Missing ion group in J1 seems not fully supported. The added test was causing problems, so I add J1O parameter to make it passing for now. We definitely need to fix the case of missing group but not in this PR and also not adding two dummy parameters as the added test indicates.
There was a problem hiding this comment.
Ok I can take a look in a following PR
|
Test this please |
|
@PDoakORNL This PR so far looks good for me now. OK to merge? Trying to eliminate long standing PRs. |
Proposed changes
This PR addresses some maintenance work suggested by @ye-luo for One body jastrows. This merges the two files mentioned in the title, and uses
unique_ptr. I used #3217 as a guide.What type(s) of changes does this code introduce?
Does this introduce a breaking change?
What systems has this change been tested on?
Laptop i7-7700HQ
Checklist