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.

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

Usm ndrray array protocol must raise #1964

Merged
merged 4 commits into from
Jan 15, 2025
Merged

Usm ndrray array protocol must raise #1964

merged 4 commits into from
Jan 15, 2025

Conversation

oleksandr-pavlyk
Copy link
Collaborator

Implement usm_ndarray.__array__ that raises TypeError to disallow implicit conversion of usm_ndarray to numpy.ndarray with dtype=object.

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to an issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • Have you checked performance impact of proposed changes?
  • Have you added documentation for your changes, if necessary?
  • Have you added your changes to the changelog?
  • If this PR is a work in progress, are you opening the PR as a draft?

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 13, 2025 •
edited
Loading

Deleted rendered PR docs from intelpython.github.com/dpctl, latest should be updated shortly. 🤞

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_434 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@coveralls
Copy link
Collaborator

coveralls commented Jan 13, 2025 •
edited
Loading

Coverage Status

coverage: 87.683% (+0.002%) from 87.681%
when pulling 8393a75 on usm_ndrray-array-protocol-must-raise
into 0f3536b on master.

@oleksandr-pavlyk oleksandr-pavlyk mentioned this pull request Jan 13, 2025
Merged
8 tasks
@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_435 ran successfully.
Passed: 895
Failed: 1
Skipped: 118

@ndgrigorian
Copy link
Collaborator

Should we define copy and dtype parameters?

NumPy claims it will raise warnings otherwise, but of course, this is just here to raise an error—not sure if it will be a problem or not.

@oleksandr-pavlyk
Copy link
Collaborator Author

@ndgrigorian Could you please cite where sych claim is made? I looked at https://numpy.org/doc/stable/reference/generated/numpy.asarray.html and could not find it

@oleksandr-pavlyk
Copy link
Collaborator Author

Should we define copy and dtype parameters?

If you mean in the test, than we could, but it should not make a difference.

@ndgrigorian
Copy link
Collaborator

ndgrigorian commented Jan 14, 2025 •
edited
Loading

@ndgrigorian Could you please cite where sych claim is made? I looked at https://numpy.org/doc/stable/reference/generated/numpy.asarray.html and could not find it

https://numpy.org/doc/2.1/user/basics.interoperability.html#dunder-array-interface
It is mentioned here

The signature of the method should be __array__(self, dtype=None, copy=None). If a passed dtype isn’t None and different than the object’s data type, a casting should happen to a specified type. If copy is None, a copy should be made only if dtype argument enforces it. For copy=True, a copy should always be made, where copy=False should raise an exception if a copy is needed.

If a class implements the old signature __array__(self), for np.array(a) a warning will be raised saying that dtype and copy arguments are missing.

@oleksandr-pavlyk oleksandr-pavlyk force-pushed the usm_ndrray-array-protocol-must-raise branch from 8571abc to f27d38f Compare January 14, 2025 11:29
@oleksandr-pavlyk
Copy link
Collaborator Author

I have changed __array__ from being a property to being a method with the signature NumPy expects. Thank you for catching this error @ndgrigorian

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_435 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_436 ran successfully.
Passed: 895
Failed: 1
Skipped: 118

@oleksandr-pavlyk
Implement usm_ndarray.__arrray__ method
81db5e5 
The property raises TypeError informing user that
implicit conversions from usm_ndarray to numpy.ndarray
are not permitted.

Explicit conversion using `dpctl.tensor.asnumpy` is
advised.

Disallowing implicit conversion prevents `np.asarray(usm_ar)`
from creating an array of 0D usm_ndarray instances, because
using it is very costly due to multitude of short-array transfers
from GPU to host.
@oleksandr-pavlyk
Add test that np.asarray(usm_ar) raises TypeError
d34bad9
@oleksandr-pavlyk
Record addition of __array__ method change in changelog
b6e3afe
@oleksandr-pavlyk
Make copy argument of __array__ method keyword-only
8393a75
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the usm_ndrray-array-protocol-must-raise branch from a9385af to 8393a75 Compare January 14, 2025 16:04
@github-actions GitHub Actions
Copy link

Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_440 ran successfully.
Passed: 894
Failed: 2
Skipped: 118

ndgrigorian
ndgrigorian approved these changes Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@ndgrigorian ndgrigorian left a comment

Choose a reason for hiding this comment

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

LGTM!

@oleksandr-pavlyk oleksandr-pavlyk merged commit 6f02ec3 into master Jan 15, 2025
58 of 63 checks passed
@oleksandr-pavlyk oleksandr-pavlyk deleted the usm_ndrray-array-protocol-must-raise branch January 15, 2025 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonwolfy antonwolfy antonwolfy left review comments

@ndgrigorian ndgrigorian ndgrigorian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oleksandr-pavlyk @coveralls @ndgrigorian @antonwolfy