search

gbrammer / grizli

Grizli: The "Grism redshift and line" analysis software
MIT License
64 stars 50 forks source link

Updating to Polynomial #206

Closed TheSkyentist closed 1 week ago

TheSkyentist commented 5 months ago

As of SciPy 1.12.0 polyfit and polyval can no longer be directly imported from SciPy. This caused import errors when running grizli. I suspect this is due to the fact that polyfit and polyval are part of the old polynomial API, which has been upgraded with NumPy Polynomial package, primarily resulting in greater numerical stability. While I could have just fixed the imports, I went through and updated the code to use the new Polynomial API. If preferred, I can simply replace the scipy imports with the correct/not broken numpy imports.

gbrammer commented 4 months ago

I like the modernization idea, but the wholesale changes to the new Polynomial behavior are a bit risky, particularly in the way the order of the polynomial coefficients are interpreted is exactly reversed! Those coeffs[::-1] throughout aren't pretty, but at least everything (sort of) works!

Could we just switch to from numpy.polynomial.polynomial import polyfit, polyval for now and file switching to the Polynomial objects as a WIP Issue?

TheSkyentist commented 4 months ago

Sounds good, I've submitted PR #207 to address the short-term issues, but will continue testing against this PR and keep this updated.

TheSkyentist commented 1 month ago

This PR now passes all CI checks and has been tested on real data to ensure it gives the same results.

gbrammer commented 1 week ago

Thanks for keeping an eye on this. Can you update minimum versions on the numpy and scipy dependencies in setup.cfg to ensure compatibility with these changes (e.g., cumtrapz > cumulative_trapezoid)? I added scipy<1.14 the other day to fix a bug in the external (and apparently stale) peakutils module, which uses scipy.integrate.simps that has been renamed to simpson.

TheSkyentist commented 1 week ago

This is a good point, we actually maintain greater compatibility with this change. The "new" Polynomial syntax has been around in NumPy since 1.4, which is over 10 years old at this point. It's true that scipy has finally removed the deprecated "simps/cumtrapz" syntax, I've forked peakutils and made the necessary updates, I wonder if we can pull from there for now. I wonder if there is a more modern package with the same functionality.

TheSkyentist commented 1 week ago

I've just pushed the change requiring NumPy >=1.4. There are likely other parts of grizli that would require newer NumPy versions, but this is a start.

gbrammer commented 1 week ago

Thanks. Can you find the minimum scipy needed for cumulative_trapezoid?

TheSkyentist commented 1 week ago

Scipy 1.6 introduced the new scipy.integrate.

gbrammer commented 1 week ago

Merged! Thanks again, @TheSkyentist.