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

Fix nearest neighbor index errors/Remove SciPy upper limit #2475

Merged
merged 2 commits into from Jan 23, 2024

Conversation

JaydenR2305
Copy link
Member

@JaydenR2305 JaydenR2305 commented Jan 23, 2024

An update to SciPy 1.12.0 conducted in #2472 revealed breaking changes made to SciPy's NearestNDInterpolator.

The problematic lines can be seen here:

@cached_property
def _nearest_neighbor_interpolator(self):
"""
Creates a nearest neighbor interpolator object for this grid, which can
then be called repeatedly.
"""
indgrid = np.arange(self.grid.shape[0])
return interp.NearestNDInterpolator(self.grid.to(u.m).value, indgrid)

The issue is that SciPy implicitly converts the grid indices (indgrid) to floats in the interpolation step. This is problematic because the return from this interpolation is used to index the quantity array:

i = self._nearest_neighbor_interpolator(pos)
vals = self._interp_quantities[i, :]

I propose we move the quantities array directly to the instantiation of the NearestNDInterpolator object, as seen in this PR:

@cached_property
def _nearest_neighbor_interpolator(self):
"""
Creates a nearest neighbor interpolator object for this grid, which can
then be called repeatedly.
"""
return interp.NearestNDInterpolator(
self.grid.to(u.m).value, self._interp_quantities
)

Resolves #2474

@JaydenR2305 JaydenR2305 requested review from namurphy and a team as code owners January 23, 2024 14:46
@github-actions github-actions bot added plasmapy.plasma Related to the plasmapy.plasma subpackage packaging Related to packaging or distribution labels Jan 23, 2024
Copy link

Thank you for submitting a pull request (PR) to PlasmaPy! ✨ The future of the project depends on contributors like you, so we deeply appreciate it! 🌱

Our contributor guide has information on:

The bottom of this page shows several checks that are run for every PR. Don't worry if something broke! We break stuff all the time. 😺 Click on "Details" to learn why a check didn't pass. Please also feel free to ask for help. We do that all the time as well. 🌸 You can find us in our chat room or weekly community meeting & office hours. Here are some tips:

  • Try fixing CI / Python 3.11 test failures first.
  • Most pre-commit.ci - pr failures can be automagically fixed by commenting pre-commit.ci autofix below, followed by a git pull to bring the changes back to your computer. Please also see our pre-commit troubleshooting guide.
  • If pre-commit.ci - pr says that a function is too long or complex, try breaking up that function into multiple short functions that each do one thing. See also these tips on writing clean scientific software.
  • If the CI / Documentation check ends with a cryptic error message, check out our documentation troubleshooting guide.
  • For a documentation preview, click on Details next to docs/readthedocs.org:plasmapy.

If this PR is marked as ready for review, someone should stop by to provide a code review and offer suggestions soon. ✅ If you don't get a review within a few days, please feel free to send us a reminder.

Please also use SI units within PlasmaPy, except when there is strong justification otherwise or in some examples.

We thank you once again!

@JaydenR2305 JaydenR2305 changed the title Fix Nearest Neighbor Indexing Errors Fix nearest neighbor indexing errors/Remove SciPy upper limit Jan 23, 2024
@JaydenR2305 JaydenR2305 changed the title Fix nearest neighbor indexing errors/Remove SciPy upper limit Fix nearest neighbor index errors/Remove SciPy upper limit Jan 23, 2024
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d23a9a1) 96.93% compared to head (42f86e4) 96.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2475      +/-   ##
==========================================
- Coverage   96.93%   96.93%   -0.01%     
==========================================
  Files         104      104              
  Lines        9165     9163       -2     
==========================================
- Hits         8884     8882       -2     
  Misses        281      281              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JaydenR2305
Copy link
Member Author

JaydenR2305 commented Jan 23, 2024

@pheuer wanted me to make sure that we aren't doing multiple interpolations due to the nature of providing a multidimensional array for the data values in the construction of the NearestNDInterpolator.

Here is the relevant code:
https://github.com/scipy/scipy/blob/4edfcaa3ce8a387450b6efce968572def71be089/scipy/interpolate/_ndgriddata.py#L144-L166

The underlying type/shape of the provided data values is actually irrelevant until it comes to populating the interp_values array-- at which point it is necessary to cast the array to the proper type. This isn't necessary in our case because we still use float64 as opposed to complex numbers (or integers!) It was possible to provide an array of strings in previous versions and still get the expected behavior.

@pheuer
Copy link
Member

pheuer commented Jan 23, 2024

The underlying type/shape of the provided data values is actually irrelevant until it comes to populating the interp_values array (at which point it is necessary to cast the array to the proper type, which isn't necessary in our case because we still use float64 as opposed to complex numbers.)

Sounds good, thanks for checking this - once you add a changelog entry I think we can merge this PR

@JaydenR2305
Copy link
Member Author

oops forgot thanks for reminding me

@pheuer pheuer merged commit 45e6f56 into PlasmaPy:main Jan 23, 2024
16 checks passed
@pheuer
Copy link
Member

pheuer commented Jan 23, 2024

Thanks @StanczakDominik !

@namurphy
Copy link
Member

Thank you for fixing and reviewing this so quickly!

@JaydenR2305 JaydenR2305 deleted the fix-grid-nn-indexing branch January 23, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packaging Related to packaging or distribution plasmapy.plasma Related to the plasmapy.plasma subpackage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove upper limit on version of SciPy and fix resulting test failures
4 participants