-
Notifications
You must be signed in to change notification settings - Fork 11
RFC: rm __array__, add __buffer__ #115
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
base: main
Are you sure you want to change the base?
Conversation
The matching SciPy PR: scipy/scipy#22189 |
df7b4d1
to
4fcb9e6
Compare
4fcb9e6
to
a821bb9
Compare
The current behavior: On python 3.12:
On python 3.11:
|
8115901
to
ede45af
Compare
6961ee7
to
45befd6
Compare
PSA: The timeline for this PR is "land in a couple of month's time". The current thinking is that short-term we work on the 2024.12 support in version 2.3, let the dust settle, and then merge this PR. Downstream testing with |
Otherwise, on python 3.11 and below, np.array(array_api_strict_array) becomes a 0D object array.
Alternatively, we can make a 2.4 release with what is now in main (#156), and postpone the Locally, this branch causes no problems for SciPy (scipy/scipy#23144 adds small fixes from local testing), and scikit-learn/scikit-learn#31517 fixes scikit-learn. |
# This was implemented historically for compatibility, and removing it has | ||
# caused issues for some libraries (see | ||
# https://github.com/data-apis/array-api-strict/issues/67). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this comment as well or edit it to mention that it is referring to __array__
- because "this was implemented ..." will be weird. Not sure we need to keep it/combine it with the comment below for __buffer__
, so delete?
if self._device != CPU_DEVICE: | ||
raise RuntimeError(f"Can not convert array on the '{self._device}' device to a Numpy array.") | ||
return memoryview(self._array) | ||
def __release_buffer(self, buffer): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def __release_buffer(self, buffer): | |
def __release_buffer__(self, buffer): |
def __buffer__(self, flags): | ||
if self._device != CPU_DEVICE: | ||
raise RuntimeError(f"Can not convert array on the '{self._device}' device to a Numpy array.") | ||
return memoryview(self._array) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my education: why not use return self._array.__buffer__(flags)
- aka delegate to numpy for this? And the same for __release_buffer__
Try getting rid of
__array__
, replace with the buffer protocol's__buffer__
.cross-ref #67, #69
This does not seem to fix gh-102.
scipy
tests pass locally.Replacing
__array__
with__buffer__
makes code marginally simpler. Effectively bumps the python requirement to 3.12+ though.So maybe this is a temp solution until
dlpack
matures.¯\_(ツ)_/¯