Fix indexing inside SplineX2X when outputing orbitals fewer than it holds#4871
Conversation
passes with my proposed fix
|
The current units tests that are failing, are actually failing as expected. This is due to other spline implementations needing the same fixes. I'll work on those next and get the test passing |
|
As is, the unit tests are modified to allow for the (requested number of orbitals) < (the number of orbitals defined in the SPOSet). The current unit test in test_hybridrep only checks SplineC2C and SplineR2R. These modifications to the unit test fail with an out of bounds access without the changes to SplineC2C and SplineR2R. The various assign_x functions in the splines now guard against out of bounds access for the requested number of orbitals, and then the tests pass The diamond2x1x1 test can be modified in the same way to test SplineC2R, but I'm still taking a look at that. For now, I think we could just merge this, and do subsequent PRs on SplineC2R and SplineC2ROMPTarget/SplineC2COMPTarget if those need it |
| // protect last | ||
| last = last > kPoints.size() ? kPoints.size() : last; | ||
| const size_t last_real = std::min(kPoints.size(), psi.size()); | ||
| last = last > last_real ? last_real : last; |
There was a problem hiding this comment.
Should be called last_cplx as the last complex output orbital.
| { | ||
| // protect last | ||
| last = last > kPoints.size() ? kPoints.size() : last; | ||
| const size_t last_real = std::min(kPoints.size(), psi.size()); |
| const ST* restrict g2 = myG.data(2); | ||
|
|
||
| const size_t N = last_spo - first_spo; | ||
| const size_t last_real = last_spo > psi.size() ? psi.size() : last_spo; |
| { | ||
| // protect last | ||
| last = last > kPoints.size() ? kPoints.size() : last; | ||
| const int last_real = std::min(kPoints.size(), psi.size()); |
|
Test this please |
|
Test this please |
|
Well the V100 Clang offload tests were failing, which makes me think the tests I modified actually end up going through SplineC2COMPTarget, so i need to include those fixes in this PR as well. So I went ahead and fixed SplineC2R and modified a test for that as well since it seems all the Spline classes need a fix |
|
Ok, I think all tests should pass now |
|
|
||
| const size_t N = last_spo - first_spo; | ||
| const size_t last_cplx = last_spo > psi.size() ? psi.size() : last_spo; | ||
| const size_t N = last_cplx - first_spo; |
There was a problem hiding this comment.
This is the fix for the previous CI failure.
|
Will approve and merge once the CI update is merged. |
b66b7cc to
78b6ee1
Compare
ye-luo
changed the title
|
Test this please |
3 participants
Please review the developer documentation
on the wiki of this project that contains help and requirements.
Proposed changes
Found a segfault when trying to use hybridrep in orbital optimization. The source of the problem is that when doing orbital optimization, the underlying orbital set includes unoccupied orbitals. However, if we are calling the evaluations from inside a DiracDeterminant, the ValueVector psi size is ways set to the number of occupied orbitals, not the full size of the SPOSet. The fix here allows for proper indexing into psi, and this is actually producing a successful hyrbid rep optimization.
The spline classes do not properly guard against accessing out of bounds for the ValueVector psi, and I expect this is more general problem for all SPOSets.
TO DO:
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?
M1 mac
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.