ENH: rm __array__, add __buffer__#115
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 |
d5397d6 to
12fe35d
Compare
|
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. |
array_api_strict/_array_object.py
Outdated
| # 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.
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?
array_api_strict/_array_object.py
Outdated
| 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.
For my education: why not use return self._array.__buffer__(flags) - aka delegate to numpy for this? And the same for __release_buffer__
There was a problem hiding this comment.
This is a great idea, actually. I'm going to make this change and rerun scipy and scikit-learn tests just to be sure.
The __release_buffer__ story is a bit mysterious:
In [11]: np.asarray(xp.arange(3))
Out[11]: array([0, 1, 2])
In [12]: np.sin(xp.arange(3))
Exception ignored in: Array([0, 1, 2], dtype=array_api_strict.int64)
Traceback (most recent call last):
File "/home/br/repos/array-api-strict/array_api_strict/_array_object.py", line 173, in __release_buffer__
self._array.__release_buffer__()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'numpy.ndarray' object has no attribute '__release_buffer__'
Out[12]: array([0. , 0.84147098, 0.90929743])
In [13]: np.__version__
Out[13]: '1.26.4'
Same behavior on numpy-dev, so it's not something that got fixed in the meantime. OTOH, calling numpy ufuncs on array-api-strict objects is not really correct without an explicit prior np.asarray call, so probably it's not something we need to worry about.
There was a problem hiding this comment.
Per discussion with @seberg on numpy's slack,
Normally all you need is
def __buffer__(self, flags):
return self.data.__buffer__(flags)
and nothing else, in particular no__release_buffer__
this PR now does precisely that: use Tim's suggestion for self._array.__buffer__(flags) and does not define __release_buffer__ at all.
SciPy and scikit-learn tests still pass locally.
On python 3.12 and above, delegate to numpy's __buffer__ On earlier python's raise an error and ask the user to updgrade. Otherwise, on python 3.11 and below, np.array(array_api_strict_array) becomes a 0D object array. Co-authored-by: Tim Head <betatim@gmail.com>
either way sounds fine to me. 2 separate releases is obviously 'nicer' but happy to trust your judgement as things seem promisingly green! |
|
Okay, let's give it a go. array-api-tests pass, scipy and scikit-learn tests pass locally, too. The implementation matches the expert recommendations from Tim and Sebastian. Merging now, thanks to all reviewers! |
Try getting rid of
__array__, replace with the buffer protocol's__buffer__.cross-ref #67, #69
This does not seem to fix gh-102.
scipytests 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
dlpackmatures.¯\_(ツ)_/¯