Fix spline jastrow

[ye-luo]

Proposed changes

Review after #4827
Fix https://cdash.qmcpack.org/CDash/testDetails.php?test=28505916&build=446393
It was caused by NaN due to out-of-bound access when accessing 1D spline coefficients in Jastrow.

What type(s) of changes does this code introduce?

• Bugfix

Does this introduce a breaking change?

• No

What systems has this change been tested on?

bora

Checklist

• Yes. This PR is up to date with current the current state of 'develop'
• Yes. Code added or changed in the PR has been clang-formatted

[prckent]

Interesting!

Please give more of the backstory here: what is causing the invalid access, and how come we did not notice the problem before now? How could we avoid such things in future/catch other similar issues that might be lurking in the code today?

[ye-luo]

A simple example.
When there are 11 knots for cutoff=20. Each segment is 2.0.
There are 13 spline coefficients.
For a give distance r, the integer part is i and the residual is t after breaking up r/segment.
To calculate interpolation, we access coefs[i] to coefs[i+3] so the max allow i is 9.
for a distance 19.99999 very close to 20.
We calculate r/segment and it gives i = 10 and thus t = 0. coefs[i+3] becomes out-of-bound access.
The fix should be using i=9 and t=1.0-epsilon done via the getSplneBound.

[prckent]

How could we avoid such things in future/catch other similar issues that might be lurking in the code today?

I am thinking about options to catch these, both in the code we already have, and in future:

1. A sanitizer or similar debug flavored run for one of the mid-sized molecular or solid-state performance tests would have a statistical chance of catching this. Not 100% reliable but maybe worth a try.

2. Some unit tests that effectively plot the wavefunction component values would have caught this: for a given jastrow, compute the value at zero distance and then in small steps up to some distance beyond the cutoff e.g. do up to a few * Wigner Seitz radius for safety, and take small enough steps to have triggered the above problem.

Your thoughts?

[ye-luo]

1. The failure happened on short test. So worth running ASAN on tests longer than determinsitic.
2. For feature using splines, batter to test features at corner cases.

[PDoakORNL]

2 is better but,

Any function that converts from a float type to an index type and doesn't have a unit test that checks the out of bounds edge cases is a completely unsafe choice by both the reviewer and the author. The sort of thing that gets blue collar folks fired on the spot. Like ignoring LOTO or running CNC machines with the safetyshields removed.

[PDoakORNL]

The other problem here was that this logic was hiding in a too large function so it escaped consideration for unit testing. If the behavior of a code fragment is important enough it should get factored out into something testable.

[prckent]

Seems that a minimal test improvement would be evaluating a r=-delta, 0,delta, cutoff-delta, cutoff, cutoff+delta. Where delta = a few epsilon. I note that the "API" for the spline functor and nearby Jastrow factors does not say what the expectations are for the inputs. After this testing is done we can make an issue/task for someone else. Besides this we also need some extensive asan runs. The issue is then noticing the asan failure amidst statistical failures (== motivation to improve the statistical checks). Presumably this has been hitting production runs, although in very subtle ways to not be caught by now.

[prckent]

I made an issue so we can merge this fix as is.

The merge-base changed after approval.

The merge-base changed after approval.

[prckent]

Test this please