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

BUG: cp.generic is an alias for np.generic #207

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

crusaderky
Copy link
Contributor

@crusaderky crusaderky commented Nov 28, 2024

Fix bug where plain numpy objects e.g. np.int64(0) would take cupy-specific code paths.

This is a blocker for implementing at function in array-api-extra.

@crusaderky
Copy link
Contributor Author

crusaderky commented Dec 2, 2024

This PR is ready to be reviewed and merged.

@ev-br
Copy link
Contributor

ev-br commented Dec 2, 2024

What would be a scenario where cupy aliasing array scalars matters? I just tried running https://github.com/data-apis/array-api-compat/blob/main/test_cupy.sh locally, and it passes with or without.

@crusaderky
Copy link
Contributor Author

What would be a scenario where cupy aliasing array scalars matters?

Not that I'm aware of.
If there are any cases where the output of is_cupy_array(cp.somefunction(...)) needs to return True, even if the output is a np.generic, then those cases will need to be looked into.

To me this looks like a copy-paste error from is_numpy_array.

@crusaderky crusaderky closed this Dec 4, 2024
@crusaderky crusaderky reopened this Dec 4, 2024
crusaderky added a commit to crusaderky/array-api-compat that referenced this pull request Dec 6, 2024
@ev-br
Copy link
Contributor

ev-br commented Dec 12, 2024

So if there's no use case, what are the reasons for this then?

@crusaderky
Copy link
Contributor Author

So if there's no use case, what are the reasons for this then?

There is no use case that I know of where is_cupy_array(cupy.asarray([1])[0]) returning False would cause a problem - which is what this PR would cause.
But it is a big problem if is_cupy_array(numpy.asarray([1])[0]) returns True - which is what happens now in main.

@crusaderky crusaderky changed the title cp.generic is an alias for np.generic BUG: cp.generic is an alias for np.generic Dec 12, 2024
@crusaderky
Copy link
Contributor Author

crusaderky commented Dec 12, 2024

This is ready for final review and merge.

@crusaderky crusaderky closed this Dec 12, 2024
@crusaderky crusaderky reopened this Dec 12, 2024
@ev-br
Copy link
Contributor

ev-br commented Dec 12, 2024

But it is a big problem if is_cupy_array(numpy.asarray([1])[0]) returns True - which is what happens now in main.

That's bad indeed if that happens, but... does it?

In [5]: is_cupy_array(np.asarray([1])[0])
Out[5]: False

This is on main, after

$ pip install -e .
Obtaining file:///home/br/repos/array-api-compat
  Preparing metadata (setup.py) ... done
Installing collected packages: array_api_compat
  DEPRECATION: Legacy editable install of array_api_compat==1.9.1 from file:///home/br/repos/array-api-compat (setup.py develop) is deprecated. pip 25.0 will enforce this behaviour change. A possible replacement is to add a pyproject.toml or enable --use-pep517, and use setuptools >= 64. If the resulting installation is not behaving as expected, try using --config-settings editable_mode=compat. Please consult the setuptools documentation for more information. Discussion can be found at https://github.com/pypa/pip/issues/11457
  Running setup.py develop for array_api_compat
Successfully installed array_api_compat-1.9.1
(array-api-tests) br@gonzales:~/repos/array-api-compat$ git log
commit edde2e04a984b65dc299321ea5c0cda33f1f9e5d (HEAD -> main, upstream/main, upstream/HEAD)
Merge: d9df003 484f725
Author: Evgeni Burovski <[email protected]>
Date:   Thu Dec 12 18:58:34 2024 +0200

    Merge pull request #213 from ev-br/torch_allclose
    
    MAINT: avoid a DeprecationWarning from torch

What am I missing?

@crusaderky
Copy link
Contributor Author

crusaderky commented Dec 12, 2024

What am I missing?

You're missing cupy in sys.modules.

In [1]: from array_api_compat import is_cupy_array
In [2]: import numpy as np
In [3]: g = np.asarray([0])[0]
In [4]: is_cupy_array(g)
Out[4]: False
In [5]: import cupy
In [6]: is_cupy_array(g)
Out[6]: True

Copy link
Contributor

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

Ouf, yes indeed, is_cupy_array(....) is False unless cupy has been imported. So yes, a clear bug with a clear fix. Thank you @crusaderky

@ev-br ev-br merged commit bbc0e9c into data-apis:main Dec 12, 2024
83 of 84 checks passed
@crusaderky crusaderky deleted the cupy_generic branch December 12, 2024 19:44
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