MAINT: Update masked function signatures for NumPy 2.4 #19096
Conversation
This commit addresses positional-only argument changes in NumPy 2.4 and handles the removal of the 'style' parameter in array2string. Functions updated include array2string, array_str, ediff1d, in1d, isin, and setdiff1d.
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
Too much changes. Please look at past PRs. |
|
I understand where you're coming from @pllim , but a bit of meta-context seems needed: I've been talking with @ReemHamraz about how she could get into contributing to astropy. I suggested she took a shot at #19093 and gave her a couple hints how she might do that. This PR is her attempt, but I haven't reviewed it in any aspect yet, so I do expect her work might not be perfect at first, which of course is perfectly okay. |
|
(nevermind it was in draft state already) |
neutrinoceros
left a comment
neutrinoceros
left a comment
There was a problem hiding this comment.
Additionally, this PR updates function signatures to include the positional-only argument marker (/) where required for parity with NumPy 2.4.
I think this is out of scope in this PR and should be conducted separately. Making each PR as narrowly scoped as possible is extremely beneficial in the long term if for any reason a regression is appears to be attached to a specific PR, so we can easily understand what went wrong and make reverting easy. It's also very helpful in reviews: streamlined changes are much easier to understand and tend to be approved more promptly.
The approach seems sound but I did indeed find a handful of out-of-scope changes here. Most of them seem correct but they should still get their own PRs. We'll work that out. I'll now look into CI failures and try to grasp what's not working yet.
| def _get_np_func_name(func): | ||
| """ | ||
| Safely get the name of a numpy function. | ||
| If the function is already a string, return it. | ||
| """ | ||
| if isinstance(func, str): | ||
| return func | ||
| return getattr(func, "__name__", str(func)) |
There was a problem hiding this comment.
I think it would be preferable to only use static lookups (i.e. func.__name__) and only allow one type (functions) as inputs. This will make callsite much easier to reason about.
Additionally, it might not be sufficient to retrieve the name of a function, as we sometimes also need to carry the name of it's parent(s) namespaces. Not sure how this applies yet, but that's something to keep in mind.
| "iscomplexobj", "isrealobj", | ||
| "shares_memory", "may_share_memory", | ||
| "apply_along_axis", "take_along_axis", "put_along_axis", | ||
| "linalg.cond", "linalg.multi_dot", |
There was a problem hiding this comment.
a lot of churn, but for the record, this is exactly what I proposed we do.
| return func | ||
| return getattr(func, "__name__", str(func)) | ||
|
|
||
| def _interpret_tol(tol, unit): |
There was a problem hiding this comment.
if I'm following correctly, this is a pre-existing function but you moved its definition ? If I'm correct, please revert this bit and simply import from where it's defined.
| # currently return NotImplementedError. | ||
| # TODO: move latter to UNSUPPORTED? Would raise TypeError instead. | ||
| SUBCLASS_SAFE_FUNCTIONS |= {np.ediff1d} | ||
| SUBCLASS_SAFE_FUNCTIONS |= {"ediff1d", "interp", "interp2d", "interp3d"} |
There was a problem hiding this comment.
this might be correct, but it's still out of scope here. SUBCLASS_SAFE_FUNCTIONS is nice because it's less work to support, but we still need minimal tests for each function that lands here, so we should just leave it be for this PR.
| # If no 'helps' provided, use the name of the function itself | ||
| helps = (f.__name__,) |
There was a problem hiding this comment.
this looks incorrect (and possibly accidental ?)
| @function_helper(helps="linalg.pinv") | ||
| def pinv(a, rcond=1e-15, *args, **kwargs): | ||
| rcond = _interpret_tol(rcond, a.unit) | ||
| rcond = _interpret_tol(rcond, dimensionless_unscaled) |
There was a problem hiding this comment.
I'm not sure what this is about, but I have a hunch that it's (you guessed it) out of scope
| try: | ||
| target_unit = next(iter(arr.unit.values())) | ||
| except (StopIteration, AttributeError): | ||
| # Fallback if the unit isn't structured or is empty | ||
| target_unit = arr.unit |
There was a problem hiding this comment.
looks like a bugfix, but it needs its own test(s) -> separate PR
| UFUNC_HELPERS[np_umath._ones_like] = helper__ones_like | ||
| UFUNC_HELPERS[np.modf] = helper_modf | ||
| UFUNC_HELPERS[np.frexp] = helper_frexp | ||
| UFUNC_HELPERS[_get_np_func_name(np.sqrt)] = helper_sqrt |
There was a problem hiding this comment.
wouldn't this be exactly equivalent (and simpler ?) ?
| UFUNC_HELPERS[_get_np_func_name(np.sqrt)] = helper_sqrt | |
| UFUNC_HELPERS["sqrt"] = helper_sqrt |
| from astropy.units import StructuredUnit | ||
| if isinstance(unit, StructuredUnit): | ||
| from astropy.units.structured import StructuredQuantity | ||
| return StructuredQuantity, True | ||
|
|
||
|
|
||
| from astropy.units import LogUnit | ||
| if isinstance(unit, LogUnit): | ||
| from astropy.units import LogQuantity | ||
| return LogQuantity, True |
There was a problem hiding this comment.
not sure where that's coming from, but the docstring indicates that changes on the return value should be done as overrides in subclasses. What motivated this change ?
| if not NUMPY_LT_2_4: | ||
| @dispatched_function | ||
| def setdiff1d(ar1, ar2, /, assume_unique=False): | ||
| return _setdiff1d_impl(ar1, ar2, assume_unique) | ||
| else: | ||
| @dispatched_function | ||
| def setdiff1d(ar1, ar2, assume_unique=False): | ||
| return _setdiff1d_impl(ar1, ar2, assume_unique) |
There was a problem hiding this comment.
separate PR again :)
okay at least this is easy to reproduce. A general advice here would be to run tests locally before you push to CI so you can iterate quickly on trivial mistakes like this one. |
3 participants
MAINT: Update function helpers for NumPy 2.4 parity and import resilience
Description
This pull request addresses compatibility issues with the upcoming NumPy 2.4 release and refactors the internal dispatching registry in
astropy.unitsto be more resilient.Previously,
FUNCTION_HELPERSused NumPy function objects as dictionary keys. This caused import-time failures if a function was removed or renamed in a newer version of NumPy. By switching to string-based keys, we ensure thatastropy.unitsremains importable even if the underlying NumPy environment changes. Additionally, this PR updates function signatures to include the positional-only argument marker (/) where required for parity with NumPy 2.4.Summary of Changes:
astropy.unitsFUNCTION_HELPERSinquantity_helper/function_helpers.pyto use function names (strings) as keys (e.g.,np.sin->"sin").Quantity.__array_function__inquantity.pyto perform lookups usingfunction.__name__.sum,mean,std) inquantity.pyto include the positional-only/marker.astropy.utils.maskedarray2string,array_str,ediff1d,in1d,isin, andsetdiff1dinfunction_helpers.py._implfunctions to reduce logic duplication across NumPy version branches.NUMPY_LT_2_4constant incore.pyfor cleaner conditional logic.Scope of Future Changes:
This PR focuses on the most critical function helpers. Further changes might be needed in
astropy/utils/masked/structured.pyonce the CI results identify any remaining signature mismatches in structured array handling.Fixes #19093