Skip to content
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

Assembly getParamOfZFunction Interpolation Warning #1997

Open
wcscherer opened this issue Nov 1, 2024 · 4 comments
Open

Assembly getParamOfZFunction Interpolation Warning #1997

wcscherer opened this issue Nov 1, 2024 · 4 comments
Labels
feature request Smaller user request low priority Style points and non-features

Comments

@wcscherer
Copy link
Contributor

wcscherer commented Nov 1, 2024

The assembly method getParamOfZFunction interpolates a param axially to find it at any value of elevation z by using the scipy.interpolate.interp1d method. There are two issues with the implementation of this assembly method:

  1. It was found in the unit tests of the armi/physics/neutronics/globalFlux/globalFluxInterface.DoseResultsMapper after it was moved out of ARMI in Removing DoseResultsMapper #1952, that if an assembly only returns one z location to interpolate over, the scipy.interpolate.interp1d method will return a Nan value and a Runtime warning stating an invalid value was encountered. This behavior is undesirable as the associated unit tests still passed but with the warnings. The error was resolved by using a test reactor with more than one z location for interpolation. It would be beneficial to include logic in the getParamOfZFunction to check for more than one z location in the assembly to interpolate over.
    image

  2. The scipy.interpolate.interp1d method is now an unmaintained legacy method: https://docs.scipy.org/doc/scipy/reference/generated/scipy.interpolate.interp1d.html . The docs recommend updating any use of interp1d with numpy.interp, so getParamOfZFunction should be updated to use the current recommended interpolation method. It's important to note that numpy.interp doesn't even provide a warning if a single value is passed for interpolation, meaning the check proposed in the first point would still apply to using this interpolation method.

@john-science john-science added feature request Smaller user request low priority Style points and non-features labels Nov 1, 2024
@john-science
Copy link
Member

For (1), we already test getParamOfZFunction with an Assembly that is 3 blocks tall:

def test_getParamValuesAtZ(self):
# single value param
for b, temp in zip(self.assembly, [80, 85, 90]):
b.p.percentBu = temp
percentBuDef = b.p.paramDefs["percentBu"]
originalLoc = percentBuDef.location
try:
self.assertAlmostEqual(
87.5, self.assembly.getParamValuesAtZ("percentBu", 20.0)
)
percentBuDef.location = parameters.ParamLocation.BOTTOM
self.assertAlmostEqual(
82.5,
self.assembly.getParamValuesAtZ("percentBu", 5.0, fillValue="extend"),
)
percentBuDef.location = parameters.ParamLocation.TOP
self.assertAlmostEqual(
82.5, self.assembly.getParamValuesAtZ("percentBu", 15.0)

That test says it tests getParamValuesAtZ(), but that method is just a one-line pass-through to getParamOfZFunction().

@john-science
Copy link
Member

I can look at updating our usage of the scipy API to remove the warnings. Sure.

If it's just an API change, it's easy to handle. If it comes with numerical differences, it will take much longer to make the change.

#fingers-crossed

@wcscherer
Copy link
Contributor Author

wcscherer commented Nov 1, 2024

Thanks John. The issue was the offending test in question used the smallest test reactor of one assembly made of one block. When passed into the interpolator, it "failed" to interpolate over one z location and returned Nan with a warning.

Apparently, this method used to fail when passed an array of one value to interpolate over but that functionality was changed a decade ago to only warn the user. The warnings were noticed in the unit tests, even though the tests passed.

The original writers of the test should have known to use a larger test reactor, but neither the assembly method nor the interpolator gave them the information to know that unless they dug into the warnings - which they didn't. It would be helpful to have some guardrails around these kind of methods to avoid similar errors in the future.

This is a nice to have, not urgent.

@john-science
Copy link
Member

Ouch.

Yeah, switching from scipy.interp1d() to numpy.interp() is not a drop-in replacement. The NumPy version is much simpler, and more limited, and doesn't have features from the SciPy version that we actually use:

interpolater = interpolate.interp1d(
z,
values,
kind=interpType,
fill_value=fillValue,
assume_sorted=True,
bounds_error=boundsError,

The NumPy version is much simpler:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request low priority Style points and non-features
Projects
None yet
Development

No branches or pull requests

2 participants