data-apis / array-api-compat

Compatibility layer for common array libraries to support the Array API
https://data-apis.org/array-api-compat/
MIT License
69 stars 22 forks source link

(Temporarily) add `.device` attribute to Numpy scalars #159

Closed betatim closed 1 month ago

betatim commented 2 months ago

Basically https://github.com/numpy/numpy/issues/26850

Maybe this is something that is in scope for array-api-compat? At least temporarily so that we can use Numpy 2 without waiting for a new release.

The issue also seems clearer for array-api-compat, I think. When using the Numpy wrapped namespace (via get_namespace(..., use_compat=True) it should behave like specified in the standard. In this casexp.sumshould return something that has a.device` attribute. Whether it is an array or a scalar (which is a 0dim array).

What do people think?

rgommers commented 2 months ago

I think we should prioritize fixing this in NumPy. It's not fixable in array-api-compat; adding or wrapping functions is easy, but methods on the array object can't be monkey patched in.

betatim commented 2 months ago

Ok. I'm happy to see it fixed in Numpy.


What I was hoping for was that we can use array-api-compat to fix things faster than the next Numpy/jax/PyTorch/etc release and/or deal with making a bug like this "vanish", so that as a consuming library we don't need to deal with "well, in numpy 2.0 we can't do this, but in numpy 2.1 we can and should".

I think this particular case is an easy one to deal with in an array consuming library, so this is more of a forward looking though.

asmeurer commented 2 months ago

array-api-compat doesn't try to wrap the underlying array object, so we cannot change the attributes on NumPy scalars. See https://data-apis.org/array-api-compat/#scope and https://data-apis.org/array-api-compat/dev/special-considerations.html.

However, we can change sum so that it returns a 0-D array instead of a scalar. If you feel this is important this would be a trivial change to make.

At any rate, array-api-compat already contains a device() helper function to work around libraries that don't have a device attribute.

rgommers commented 2 months ago

However, we can change sum so that it returns a 0-D array instead of a scalar. If you feel this is important this would be a trivial change to make.

That would probably break something else - for example, there are tests in scipy.stats to verify that for numpy arrays the output is indeed a scalar rather than a 0-D array.

The fix for NumPy is already in progress and the next release isn't too far off, so I'd recommend not spending extra effort here.

betatim commented 1 month ago

Not doing anything is fine for me.

For scikit-learn we will special case based on the Numpy version in the unit tests and try and work on adding a "nightly" CI to increase the chances of finding things like this before a release happens.

lucascolley commented 1 month ago

For scikit-learn we will special case based on the Numpy version in the unit tests

Why can't you use https://data-apis.org/array-api-compat/helper-functions.html#array_api_compat.device ?

betatim commented 1 month ago

Good point.

mdhaber commented 2 weeks ago

That would probably break something else - for example, there are tests in scipy.stats to verify that for numpy arrays the output is indeed a scalar rather than a 0-D array.

Thanks for thinking of that! But I was actually just about to submit an issue proposing this sort of change. If it were possible to make this change and also make array magic methods more resistant to returning scalars (e.g. adding two 0-D arrays returns a 0-D array, indexing [()] preserves a 0-D array, breathing gently on on a 0-D array doesn't make it a scalar, etc.) I think this would really help the state of things. SciPy's array API testing functions are currently in flux because of the scalar/0-d array inconsistency. If this change were made, I think we could pretty easily make all array-API compatible SciPy functions return 0-d arrays where appropriate (when SCIPY_ARRAY_API=1) and test accordingly.