Numpy2 fixes #959
Numpy2 fixes #959
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #959 +/- ##
==========================================
- Coverage 93.20% 93.20% -0.01%
==========================================
Files 10 10
Lines 3767 3765 -2
==========================================
- Hits 3511 3509 -2
Misses 256 256 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@newville the proposed changes look fine but it seems to me there is some redundant stuff going on here that I would rather not add if not absolutely required. I am happy to clean up the commits and changes a bit if you agree.
lmfit/parameter.py
Outdated
| @@ -5,6 +5,8 @@ | |||
|
|
|||
| from asteval import Interpreter, get_ast_names, valid_symbol_name | |||
| from numpy import arcsin, array, cos, inf, isclose, sin, sqrt | |||
| from numpy import version as npvers | |||
| from packaging.version import Version | |||
There was a problem hiding this comment.
I would rather not add a dependency for such a simple version check. In a few places we just compare the version for SciPy just using: int(scipy_version.split('.')[1]) >= 11: any reason we can't do that here, especially since it's a short-term need anyway?
There was a problem hiding this comment.
@reneeotten Yes, I came to the same conclusion, and was just making a similar change too (npversion.version.startswith('2'))
lmfit/parameter.py
Outdated
| @@ -95,7 +109,8 @@ def update(self, other): | |||
| if not isinstance(other, Parameters): | |||
| raise ValueError(f"'{other}' is not a Parameters object") | |||
| self.add_many(*other.values()) | |||
| for sym in other._asteval.user_defined_symbols(): | |||
| usersyms = clean_np2_symbols(other._asteval.user_defined_symbols()) | |||
| for sym in usersyms: | |||
There was a problem hiding this comment.
This is fine, but I would prefer to leave the original in commented form, with a FIXME note to revert to it as soon as asteval is updated. Unless you will remember which changes to undo as I might not without looking back to the commit history ;)
azure-pipelines.yml
Outdated
| @@ -183,7 +183,7 @@ stages: | |||
| versionSpec: '$(python.version)' | |||
| displayName: 'Use Python $(python.version)' | |||
| - script: | | |||
| python -m pip install --upgrade build pip setuptools wheel | |||
| python -m pip install --upgrade build pip setuptools wheel "numpy>=2.0" | |||
There was a problem hiding this comment.
I don't understand the need for the specification of numpy here, doing an installation with pip will always install the latest version so specifying just numpy should suffice for every use-case (i.e., for Python 3.8 the latest pre-2.0 version and for all others the latest available 2.0.x version
There was a problem hiding this comment.
OK, we can revert, I just wanted to be specific. I don't think we need to worry about testing with both numpy 1.something and 2.something. I was burned recently by my own misconfiguration of testing, and wanted "numpy 2, dammit" just to make sure it would pass... but we can revert this file.
|
so with the new release of |
|
@reneeotten We could do that. I had been thinking that we should wait for a version or so before requiring "most recent asteval", just in case some packaging systems are not as aggressive at updating packages. But I could be persuaded. We could merge this, release a Numpy2-compatible version, and then immediately raise an Issue to clean this up ;) I'll do whichever of those you prefer. FWIW, I decided to tag "asteval 1.0" after years of being stable and then adding in more flexibility for namespaces a year ago. That release "worked" in the sense that within days I was contacted by a group at one of those well-known tech companies that had found some security flaws. I hope to clear those up and push 'asteval 1.0.1' in a week or two. That's mostly unrelated, but maybe an additional reason to keep |
|
In my opinion we should just require the latest "asteval" and remove all the changes here that are then bot required anymore. Downstream packagers can easily update asteval to the latest version, it has no dependencies and build very quickly. |
|
@reneeotten OK, requiring asteval>=1.0 does make this all a lot simpler. I would suggest Squash-and-Merge with a modified message, or I could close this and open a new PR with a single commit. |
|
okay, let me take a look later today and I'll squash-merge or do some git voodoo to clean it up. |
Pull-Request: lmfit/lmfit-py#959 Signed-off-by: Michał Górny <mgorny@gentoo.org>
Now require asteval>=1.0 and uncertainties>=3.2.2
- add codespell exceptions
|
@newville I clean-up the commits a bit, added the dependency version changes to the documentation and CI, and folded in two more small changes for the latest SciPy/NumPy versions and |
|
@reneeotten Thanks for cleaning that up and for the maintenance fixes too. I'll merge (not squash) this. |
|
@reneeotten ... except that I am also going to try to fix #960 before 1.3.2 -- but that should be too much of a delay. |
Description
This fixes a few problems with NumPy 2.
It fixes #958 by explicitly removing a few odd non-serializable functions if using NumPy2 and a version of asteval that still includes those. That is, this is really a problem with asteval exporting numpy functions that it shouldn't (and are deprecated and not really needed anyway). But, until that is fixed in asteval (in progress), and that version of asteval is required, it is probably safest to look for and remove those offending functions,
This adds
packagingas a dependency in order to compare versions.This also updates the "latest" variations of the tests to use "numpy>=2.0"
This does not alter the test labeled "development version", but that might be slightly dated.
Type of Changes
Tested on
Python: 3.12.4 | packaged by conda-forge | (main, Jun 17 2024, 10:11:10) [Clang 16.0.6 ]
lmfit: 1.3.1.post1+geee021f6.d20240629, scipy: 1.14.0, numpy: 2.0.0, asteval: 0.9.33, uncertainties: 3.2.1
Verification
Have you