ENH: Upgrade Array API version to 2024.12 - #28615
charris merged 5 commits into
Conversation
Sorry, something went wrong.
jorenham
left a comment
There was a problem hiding this comment.
This might be out of scope, but the the 2024.12 api added a dtype kwarg to fft.[r]fftfreq (data-apis/array-api#885), which we currently don't have.
The __array_namespace_info__.capabilities() function should also additionally return "max dimensions" (https://data-apis.org/array-api/latest/API_specification/generated/array_api.info.capabilities.html)
Sorry, something went wrong.
|
Right, I can xfail new features, and we can consider them separately. |
Sorry, something went wrong.
|
22 failed, including special test cases, I guess this is due to some changes in Array API test suite itself. |
Sorry, something went wrong.
|
Now should be good to review! |
Sorry, something went wrong.
| if (descr == NULL) { | ||
| return NULL; | ||
| } | ||
| return PyArray_Scalar(&count, descr, NULL); |
There was a problem hiding this comment.
This is a more significant change than it may look, since it has serious impact on promotion for count_nonzero without an axis.
(I.e. code like arr.sum() / count_nonzero(arr) can behave differently.)
Maybe we can do it, but we should discuss it briefly/add a release note for visibility.
But I am tempted to fix it in the array api tests to say that it is completely fine to return an integer for count_nonzero(arr, axis=None).
(I think an integer return is just better for NumPy users, the argument against it is only that we can also return arrays of course, for which there is no equivalent behavior obviously.)
Sorry, something went wrong.
There was a problem hiding this comment.
Sure! I can just skip this test or we can keep this change, I'm Ok with both. I added it to today's triage meeting for broader discussion. I can't attend myself today so just ping me if anything was decided.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks, I just checked and NumPy doesn't really run into this (I suppose we don't really have code paths that never pass an axis, so have to provision anyway).
Still think we should at least mention it in a release note as a subtle change, though.
Sorry, something went wrong.
There was a problem hiding this comment.
Sure, I added a release note.
Right now I'm also in favor of this change - returning a NumPy scalar when axis=None makes it coherent when axis is passed and the result is 0-d. Right now we have:
In [1]: np.count_nonzero(np.array([1,0,3,1]))
Out[1]: 3
In [2]: np.count_nonzero(np.array([1,0,3,1]), axis=0)
Out[2]: np.int64(3)
In [3]: np.count_nonzero(np.array([[1,0,3,1]]), axis=(0,1))
Out[3]: np.int64(3)After this change it's also np.int64(3) for axis=None.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah, I understand that consistency is better with the change. But in contexts where axis is always None, the integer return is more useful and changing it can change results, because:
arr = np.linspace(0, 100, 10000, dtype=np.float32) res = arr / np.count_nonzero(arr)
will change from being a float32 result to a float64 one.
Not that I suspect this to be seen often. skimage has a function that will return a float64 rather than a Python float with this change for example, I am sure that usually doesn't matter.
Sorry, something went wrong.
This comment was marked as resolved.
|
@jorenham Right, thanks! Updated. |
Sorry, something went wrong.
This comment has been minimized.
There was a problem hiding this comment.
Stubs look good now 👌🏻
The mypy_primer diff shows the effect that this count_nonzero change would have on the mypy output of 22 downstream libraries, specifically:
- https://github.com/pandas-dev/pandas
- https://github.com/optuna/optuna
- https://github.com/hydpy-dev/hydpy
- https://github.com/Toufool/AutoSplit
- https://github.com/colour-science/colour
- https://github.com/apache/spark
- https://github.com/enthought/comtypes
- https://github.com/JohannesBuchner/imagehash
- https://github.com/pandas-dev/pandas-stubs
- https://github.com/scikit-learn/scikit-learn
- https://github.com/dedupeio/dedupe
- https://github.com/google/jax
- https://github.com/yurijmikhalevich/rclip
- https://github.com/bokeh/bokeh
- https://github.com/scipy/scipy
- https://github.com/static-frame/static-frame
- https://github.com/astropenguin/xarray-dataclasses
- https://github.com/pydata/xarray
- https://github.com/scipy/scipy-stubs
- https://github.com/arviz-devs/arviz
- https://github.com/pyodide/pyodide
- https://github.com/artigraph/artigraph
This change will apparently affect 3 of them, although I'm not sure what conclusion to draw from that.
So just to be clear: My ✅ only applies to the typing side of things, as I'm not sure how to judge this int vs intp dance-off.
Sorry, something went wrong.
|
One other small deviation in the 2024.12 array api spec:
Don't know if numpy wants to follow the spec? Either way, data-apis/array-api-compat#317 adds a workaround to array-api-compat. |
Sorry, something went wrong.
In this case, it seems OK to just allow the -1 (with a ..versionchanged directive), since it matches argsort/argpartition. In general, I would love to clarify if others consider it now OK if array-api-compat diverges indefinitely from NumPy or not. |
Sorry, something went wrong.
FWIW take_along_axis already allows -1, so if we don't want to change the default then no action is required here: In [60]: a = np.arange(16).reshape((4,4))
In [61]: np.take_along_axis(a, np.array([[1, 2]]), -1)
Out[61]:
array([[ 1, 2],
[ 5, 6],
[ 9, 10],
[13, 14]]) |
Sorry, something went wrong.
This comment has been minimized.
| @@ -0,0 +1,2 @@ | |||
| * NumPy's ``__array_api_version__`` was upgraded from ``2023.12`` to ``2024.12``. | |||
| * `numpy.count_nonzero` for ``axis=None`` now returns a scalar instead of a Python integer. | |||
There was a problem hiding this comment.
| * `numpy.count_nonzero` for ``axis=None`` now returns a scalar instead of a Python integer. | |
| * `numpy.count_nonzero` for ``axis=None`` (default) now returns a NumPy scalar instead of a Python integer. |
Anyway it's probably OK, but larger than it looks. So I would like someone else to sign off on this subtle change before merging it. Maybe @mhvk?
(I don't want to argue that the old thing is better, just the very subtle change that I can see being useful in practice, unfortunately.)
(I guess the typing diff shows similar places to skimage, where the result might be subtly slightly worse.)
EDIT: See also data-apis/array-api#932
Sorry, something went wrong.
There was a problem hiding this comment.
I added the release note tweak.
Sorry, something went wrong.
|
Re: count_nonzero, array-api-compat carries a workaround currently, https://github.com/data-apis/array-api-compat/blob/main/array_api_compat/numpy/_aliases.py#L128. |
Sorry, something went wrong.
This comment has been minimized.
mhvk
left a comment
There was a problem hiding this comment.
I think on balance it is fine to stick to the array API and return a numpy scalar for count_nonzero. I certainly can see how it is nice that the resulting dtype is not going to depend on what axis is.
I think we might as well adjust take_along_axis to get a default axis=-1 (with a version changed label).
Sorry, something went wrong.
I agree, but I can imagine that it could break a lot of code if we'd change that without announcing it beforehand. On the other hand, deprecating calls without an explicit axis would also be pretty annoying. |
Sorry, something went wrong.
|
Let's address/discuss take_along_axis default value separately then. For 2024.12 we have Array API test job passing in this PR (and we don't skip take_along_axis tests). So I assume we've reached an agreement to accept the count_nonzero change. Then we can have 2024.12 support as it is here - let's merge it. Any objections? |
Sorry, something went wrong.
@jorenham it just introduces a default, turning an error (nothing passed) into a success that is normally fine to just do. (I don't care, if anyone prefers no-default, I doubt anyone will care to just not ask for a default in Array API either -- it's fine to skip the default, more so there than in NumPy.) It's good (and OK) to split out these discussion either way of course! |
Sorry, something went wrong.
Great; that makes things a lot easier |
Sorry, something went wrong.
|
As noted by @seberg, there is no real problem with adding a default for take_along_axis - there is none currently. So I think we might as well do it... |
Sorry, something went wrong.
This is because it is untested in array-api -tests ATM: |
Sorry, something went wrong.
That's right, I added it in the latest commit! |
Sorry, something went wrong.
This comment has been minimized.
|
I resolved conflicts and rebased the branch. Once CI is green I'm going to merge this PR. Any objections? |
Sorry, something went wrong.
|
TBH, I would prefer you don't self merge but rather ask someone else to merge. Or at least give a few days notice on anything non-trivial. |
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Diff from mypy_primer, showing the effect of this PR on type check results on a corpus of open source code: imagehash (https://github.com/JohannesBuchner/imagehash)
+ imagehash/__init__.py:112: error: Incompatible return value type (got "signedinteger[_32Bit | _64Bit]", expected "int") [return-value]
optuna (https://github.com/optuna/optuna)
+ optuna/study/_multi_objective.py:104: error: Incompatible types in assignment (expression has type "signedinteger[_32Bit | _64Bit]", variable has type "int | None") [assignment]
+ optuna/study/_multi_objective.py:111: error: Incompatible types in assignment (expression has type "signedinteger[_32Bit | _64Bit]", variable has type "int | None") [assignment]
+ optuna/_gp/optim_mixed.py:307: error: No overload variant of "min" matches argument types "int", "signedinteger[_32Bit | _64Bit]" [call-overload]
+ optuna/_gp/optim_mixed.py:307: note: Possible overload variants:
+ optuna/_gp/optim_mixed.py:307: note: def [SupportsRichComparisonT: SupportsDunderLT[Any] | SupportsDunderGT[Any]] min(SupportsRichComparisonT, SupportsRichComparisonT, /, *_args: SupportsRichComparisonT, key: None = ...) -> SupportsRichComparisonT
+ optuna/_gp/optim_mixed.py:307: note: def [_T] min(_T, _T, /, *_args: _T, key: Callable[[_T], SupportsDunderLT[Any] | SupportsDunderGT[Any]]) -> _T
+ optuna/_gp/optim_mixed.py:307: note: def [SupportsRichComparisonT: SupportsDunderLT[Any] | SupportsDunderGT[Any]] min(Iterable[SupportsRichComparisonT], /, *, key: None = ...) -> SupportsRichComparisonT
+ optuna/_gp/optim_mixed.py:307: note: def [_T] min(Iterable[_T], /, *, key: Callable[[_T], SupportsDunderLT[Any] | SupportsDunderGT[Any]]) -> _T
+ optuna/_gp/optim_mixed.py:307: note: def [SupportsRichComparisonT: SupportsDunderLT[Any] | SupportsDunderGT[Any], _T] min(Iterable[SupportsRichComparisonT], /, *, key: None = ..., default: _T) -> SupportsRichComparisonT | _T
+ optuna/_gp/optim_mixed.py:307: note: def [_T1, _T2] min(Iterable[_T1], /, *, key: Callable[[_T1], SupportsDunderLT[Any] | SupportsDunderGT[Any]], default: _T2) -> _T1 | _T2
spark (https://github.com/apache/spark)
+ python/pyspark/ml/linalg/__init__.py:344: error: Incompatible return value type (got "signedinteger[_32Bit | _64Bit]", expected "int") [return-value]
+ python/pyspark/ml/linalg/__init__.py:643: error: Incompatible return value type (got "signedinteger[_32Bit | _64Bit]", expected "int") [return-value]
+ python/pyspark/mllib/linalg/__init__.py:397: error: Incompatible return value type (got "signedinteger[_32Bit | _64Bit]", expected "int") [return-value]
+ python/pyspark/mllib/linalg/__init__.py:692: error: Incompatible return value type (got "signedinteger[_32Bit | _64Bit]", expected "int") [return-value]
|
Sorry, something went wrong.
Upgrades Array API Standard version to 2024.12 (and Array API test suite).
Let's see how many failures we get when moving to 2024.12.