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

Should torch.reshape support integer shape? #28

Closed thomasjpfan closed 1 year ago

thomasjpfan commented 1 year ago

torch.reshape requires an tuple as the shape:

import torch
import array_api_compat 

x = torch.asarray([1.0, 2.0])
xp = array_api_compat.get_namespace(x)

# works
xp.reshape(x, (-1,))

xp.reshape(x, -1)
# TypeError: reshape(): argument 'shape' (position 2) must be tuple of ints, not int

Note that integer shape works with numpy.array_api:

import numpy.array_api as np
import array_api_compat 

x = np.asarray([1.0, 2.0])
xp = array_api_compat.get_namespace(x)

xp.reshape(x, -1)
asmeurer commented 1 year ago

This looks like a bug in numpy.array_api. reshape in the spec only accepts a tuple (in all versions of the spec) https://data-apis.org/array-api/draft/API_specification/generated/array_api.reshape.html#array_api.reshape

asmeurer commented 1 year ago

Looks like broadcast_to and the axis argument of permute_dims also have this issue. We might consider changing this in the spec. Most other places that accept shape or axis accept both an int or tuple of ints.

asmeurer commented 1 year ago

Let's see what the decision here is https://github.com/data-apis/array-api/issues/608. We should probably update numpy.array_api either way.

asmeurer commented 1 year ago

Of course, we can still work around this in the wrappers here. But it wouldn't be portable to other array API compliant libraries (not that any necessarily exist). But this should be too hard to work around in upstream code either.

rgommers commented 1 year ago

I think the numpy.array_api support for ints is a bug indeed, and requiring a tuple is good practice here. It's easy to fix this up in scikit-learn, so I suggest doing it there rather than in this repo.

thomasjpfan commented 1 year ago

I'll close this issue because torch.reshape is following the spec correctly and there is nothing to change without this repo.