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

Updating to Polynomial #206

Merged
merged 9 commits into from
Jun 28, 2024
Merged

Updating to Polynomial #206

merged 9 commits into from
Jun 28, 2024

Conversation

TheSkyentist
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

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 TheSkyentist reopened this Mar 1, 2024
@TheSkyentist TheSkyentist force-pushed the update-poly branch 6 times, most recently from 0948b71 to 81cd40e Compare March 7, 2024 09:21
@TheSkyentist TheSkyentist marked this pull request as draft March 12, 2024 17:31
@TheSkyentist TheSkyentist force-pushed the update-poly branch 2 times, most recently from 5218e28 to 2a59a01 Compare May 31, 2024 13:01
@TheSkyentist TheSkyentist marked this pull request as ready for review May 31, 2024 14:37
@TheSkyentist
Copy link
Contributor Author

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

@gbrammer
Copy link
Owner

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
Copy link
Contributor Author

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.

Updated setup.cfg to make NumPy requirement explicit for Polynomial Package.
@TheSkyentist
Copy link
Contributor Author

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
Copy link
Owner

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

Specifically for compatibility with scipy.integrate functions.
@TheSkyentist
Copy link
Contributor Author

Scipy 1.6 introduced the new scipy.integrate.

@gbrammer gbrammer merged commit 210e5ac into gbrammer:master Jun 28, 2024
9 checks passed
@gbrammer
Copy link
Owner

Merged! Thanks again, @TheSkyentist.

@TheSkyentist TheSkyentist deleted the update-poly branch June 28, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants