10000 BUG: Array API `reshape` does not conform to standard · Issue #23410 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: Array API reshape does not conform to standard #23410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
agoose77 opened this issue Mar 16, 2023 · 7 comments
Closed

BUG: Array API reshape does not conform to standard #23410

agoose77 opened this issue Mar 16, 2023 · 7 comments
Labels
00 - Bug component: numpy.array_api
Milestone
1.25.0 release

Comments

@agoose77
Copy link

Describe the issue:

The reshape manipulation function in the new Array API takes a copy argument that we don't replicate in NumPy.

I think this is unintentional

Reproduce the code example:

import numpy.array_api as nx

array = nx.arange(10, dtype=nx.int64)
other = nx.reshape(array, (2, 5), copy=True)

array[:5] = -1
print(other)

Error message:

/tmp/test.py:1: UserWarning: The numpy.array_api submodule is still experimental. See NEP 47.
  import numpy.array_api as nx
Traceback (most recent call last):
  File "/tmp/test.py", line 4, in <module>
    other = nx.reshape(array, (2, 5), copy=True)
TypeError: reshape() got an unexpected keyword argument 'copy'

Runtime information:

1.23.5
3.10.9 | packaged by conda-forge | (main, Feb  2 2023, 20:20:04) [GCC 11.3.0]

Context for the issue:

No response
@agoose77
Copy link
Author

@asmeurer :) I can't make a PR just yet, but here's the issue :)

@asmeurer
Copy link
Member

Yes, anything in the 2021.12 standard should be implemented in np.array_api. This was just missed.

@bwalshe
Copy link
Contributor
bwalshe commented Apr 18, 2023

I had a quick go at cleaning this up, but I am a little unclear what the semantics of "copy" means here. Does it just refer to the underlying array data, or does it refer to the array object? Like if copy=False or None does that mean that the shape of the input array object should be mutated?

I have an initial commit here. It looks like there were a few missing unit tests for the array manipulation functions, so I added them. I only tested for things that were different between Numpy and the Array API, as it looks like that is what is being done elsewhere. Is that alright.

Also, it looks like copying isn't fully implemented for asarray either. I assume it would be best to fix that separately to fixing this?

@agoose77
Copy link
Author

@bwalshe conventionally copy only refers to array data. I don't think there's any statement regarding the array object itself. I'd assume this is free to e.g. return the same object if e.g. nothing needs to change. However, I would expect copy=False to always return a new object if the shape does change; we don't want to mutate the array.

@thomasjpfan thomasjpfan mentioned this issue May 5, 2023
Merged
7 tasks
@asmeurer
Copy link
Member

That's maybe something that could be clarified in the standard, but copy should refer to the data. There's no real requirements, as far as the array API spec goes, about the actual array container object.

In numpy.array_api in particular, there's two objects, the wrapper Array object and the underlying ndarray. I think there might already be some places where we create a new wrapper object even if we don't have to. I would go with whatever implementation is best for performance, i.e., avoid creating a new object if it is unnecessary.

I have an initial commit here. It looks like there were a few missing unit tests for the array manipulation functions, so I added them. I only tested for things that were different between Numpy and the Array API, as it looks like that is what is being done elsewhere. Is that alright.

The test suite in numpy.array_api only focuses on things that aren't already tested by array-api-tests. This primarily means strictness things (i.e., mostly exceptions), because the spec doesn't require strictness so the array-api test suite doesn't check for it, but we do want numpy.array_api to be strict/minimal. Your tests look fine to me. The actual reshape tests themselves aren't strictly needed if you consider array-api-tests, but all the raises checks are things that aren't checked by the test suite.

Can you open a PR with your fix?

bwalshe added a commit to bwalshe/numpy that referenced this issue May 22, 2023
@bwalshe
BUG: add copy parameter for api.reshape function
fc6e53e 
Thhis adds a parameter to api.reshape to specify if data
should be copied. This parameter is required so that
api.reshape conforms to the standard. See numpy#23410
bwalshe added a commit to bwalshe/numpy that referenced this issue May 22, 2023
@bwalshe
BUG: add copy parameter for api.reshape function
c19e84e 
This adds a parameter to api.reshape to specify if data
should be copied. This parameter is required so that
api.reshape conforms to the standard. See numpy#23410
@bwalshe bwalshe mentioned this issue May 22, 2023
Merged
@bwalshe
Copy link
Contributor
bwalshe commented May 22, 2023

@asmeurer I feel this fix is still a wip, and may need some changes to the C code to get working the way I would like. I have not had a chance to look into this in a while, but I have opened a PR with the changes I have made so far. I might need a bit of guidance to get this closed.

charris pushed a commit to charris/numpy that referenced this issue Jun 15, 2023
@bwalshe @charris
BUG: add copy parameter for api.reshape function
f0a84d1 
This adds a parameter to api.reshape to specify if data
should be copied. This parameter is required so that
api.reshape conforms to the standard. See numpy#23410
@charris charris mentioned this issue Jun 15, 2023
Merged
@rgommers
Copy link
Member

It looks like this was fixed in gh-23789 for 1.26.x and backported for 1.25.0 in gh-23957, so I'll close the issue. Thanks all!

@rgommers rgommers added this to the 1.25.0 release milestone Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
00 - Bug component: numpy.array_api
Projects
None yet
Milestone
1.25.0 release
Development

No branches or pull requests
4 participants
@asmeurer @rgommers @agoose77 @bwalshe
0