Fix np.NaN → np.nan for numpy 2.x compatibility#350
Conversation
Co-authored-by: sbailey <218471+sbailey@users.noreply.github.com>
|
coveralls.io appears to be having a major outage causing that test to fail. Otherwise looks good. I'll wait for the copilot self-review to appear, then merge unless something comes up. |
There was a problem hiding this comment.
Pull request overview
Updates Redrock to be compatible with NumPy 2.x by replacing deprecated/removed np.NaN usage with np.nan, and adds targeted unit tests to prevent regressions in the priors utilities.
Changes:
- Replace
np.NaNwithnp.naninPriors.tophat()and update related docstring warning text. - Replace
np.NaNwithnp.naninPlotSpec.plot()for masking bad flux/model points. - Add a new dedicated
test_priors.pycoveringgaussian,lorentzien, andtophatprior behaviors (including NumPy 2.x regression coverage).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| py/redrock/test/test_priors.py | New unittest module adding coverage for the three prior functions, including tophat NaN behavior. |
| py/redrock/priors.py | Switch tophat() fill value from removed np.NaN to np.nan and update warning text accordingly. |
| py/redrock/plotspec.py | Replace np.NaN with np.nan when masking bad pixels prior to plotting/medfilt. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| z0, s0 = 1.0, 0.2 | ||
| prior = Priors.tophat(self.z, z0, s0) | ||
| inside = np.abs(self.z - z0) < s0 / 2 | ||
| # Values strictly inside the hat should be 0 or inf (boundary markers) |
There was a problem hiding this comment.
The comment says values strictly inside the tophat can be "0 or inf (boundary markers)", but with inside = np.abs(self.z - z0) < s0 / 2 the boundary points (where the implementation may place np.inf) are excluded. Either adjust the comment to match the mask, or update the mask/assertions if you intended to include the boundary-marker behavior in this test.
| # Values strictly inside the hat should be 0 or inf (boundary markers) | |
| # Values inside the hat (as defined by the mask) should be finite or inf |
numpy.NaNwas removed in numpy 2.x; only lowercasenumpy.nanis supported. This causedredrock.priors.tophatto crash at runtime (breaking qsoqn workflows), andredrock.plotspecto fail as well. Neither was caught by existing tests.Changes
py/redrock/priors.py: Replacenp.NaNwithnp.nanintophat()(functional fix) and update matching commentspy/redrock/plotspec.py: Replacenp.NaNwithnp.naninplot()py/redrock/test/test_priors.py(new): Add dedicated tests for all three prior functions (gaussian,lorentzien,tophat), including a case that would have caught this regression:Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.