Skip to content
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.

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

Re-introduce __array__ with a warning #91

Closed
wants to merge 1 commit into from

Conversation

asmeurer
Copy link
Member

@asmeurer asmeurer commented Nov 8, 2024

#67

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.

@asmeurer
Copy link
Member Author

asmeurer commented Nov 8, 2024

@rgommers mentioned this might still be too hard for SciPy in #67 so waiting for feedback here CC @mdhaber @lucascolley

@mdhaber
Copy link

mdhaber commented Nov 8, 2024

I don't know whether silencing the warning is easier than fixing it. @lucascolley do you think we will be able to get scipy/scipy#21835 working before we need the features from array_api_strict 2.1+?

@lucascolley
Copy link
Contributor

I don't think I have the capacity to push it through immediately, but maybe someone can follow through on the idea.

asmeurer added a commit to asmeurer/array-api-strict that referenced this pull request Nov 8, 2024
Removing it caused issues for SciPy
(data-apis#67). I have left the
flag in to make it easy to remove it in the future.

I also considered raising a warning in __array__, but this is also difficult
to handle data-apis#91
@asmeurer asmeurer mentioned this pull request Nov 8, 2024
@asmeurer
Copy link
Member Author

asmeurer commented Nov 8, 2024

OK, I'll just re-enable it completely for now #92. We can continue the discussion about it at #67. If there are any changes that need to be made for either the standard or NumPy we should discuss those too.

@lucascolley
Copy link
Contributor

FWIW I do think it should be removed. But giving consumer libraries some time to adapt will be helpful.

@mdhaber
Copy link

mdhaber commented Nov 8, 2024

I'm happy with this being re-enabled while we figure out how this should be done. Maybe it could be controlled by an environment variable so we can test with _allow_array = False once we have converted to from_dlpack?

@asmeurer
Copy link
Member Author

asmeurer commented Nov 8, 2024

What would help is some clarity on what the problems are. Is there something that should be added to the standard and/or some changes that should be made to NumPy to make this easier?

I do know that for array-api-strict itself I found that I had to do some workarounds to make asarray([<list of array_api_strict arrays>]) work, because we reuse the np.asarray logic for that (I'm not keen on re-implementing the full list-of-lists logic here, and NumPy doesn't provide any useful helpers). But I don't know if this is the sort of problem that is affecting you, or if the problems are something else.

@lucascolley
Copy link
Contributor

lucascolley commented Nov 8, 2024

In SciPy the problem is that we punted on using DLPack by just using np.asarray where needed and adding skips for GPU backends. Now that there is a CPU backend which doesn't work with np.asarray (array-api-strict), we're seeing the consequences. We do need to move to DLPack, but it will take a bit of time.

@mdhaber
Copy link

mdhaber commented Nov 8, 2024

Is there something that should be added to the standard and/or some changes that should be made to NumPy to make this easier?

We'll probably figure this out when we're making the change to from_dlpack. It just wasn't done before because we were supposed to use np.asarray to convert to NumPy according to our development documentation.

@asmeurer
Copy link
Member Author

Going to discuss __array__ a little more in the meeting Thursday. For now, it sounds like a warning is no more helpful than an error, so I'm going to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants