Address more comments#22
Conversation
|
cleared about half of the comments, more to follow. |
| # array created by xp.zeros is non-writeable for dask | ||
| # and jax |
There was a problem hiding this comment.
Not true. All arrays are non-writeabla for jax. All arrays are writeable for dask.
There was a problem hiding this comment.
r.e. writeability, I am not referring to the mutability of the original dask array, but the numpy array created from the dask array with np.asarray.
MRE
import dask.array as da
a = da.zeros(10)
a_np = np.asarray(a)
a_np[1] = 10
Traceback (most recent call last):
File "<python-input-4>", line 1, in <module>
a_np[1] = 10
~~~~^^^
ValueError: assignment destination is read-only
I'll remove the jax comment (and I'll raise an issue on scipy to disallow the output keyword for jax since it looks like there's no way to make that work).
There was a problem hiding this comment.
Yes, that's because the chunks of a are broadcasted, and so is a_np unless it's a concatenation of chunks.
>>> da.zeros(10).__array__().strides
(0,)
>>> da.zeros(10, chunk=5).__array__().strides
()Broadcasted arrays are read-only as the whole thing is only 8 bytes in size.
There was a problem hiding this comment.
The whole idea of having an output= parameter makes no sense if you are going to convert it to numpy.
This makes sense:
def f(x: Array, output: Array) -> None:
output[:] = x * 2This does not:
def f(x: Array, output: Array) -> None:
x = np.asarray(x)
output = np.asarray(output)
output[:] = x * 2In the second example, even without the broadcasting issue you're facing, the output item that the user is holding will remain completely blank, unless np.asarray returns a view of the original memory. which, for dask, can never be.
There was a problem hiding this comment.
Note that np.asarray(output, copy=False) will not crash, while it should. This is a bug in Dask.
There was a problem hiding this comment.
Yeah, makes sense.
I'll put in a separate PR to the main scipy repo disabling the output kwarg for jax (to keep the diff down here).
| # This output array is read-only for dask and jax | ||
| # TODO: investigate why for dask? |
Co-authored-by: Guido Imperiale <crusaderky@gmail.com>
scipy/ndimage/tests/test_filters.py
Outdated
|
|
||
| @skip_xp_backends("jax.numpy", reason="output array is read-only.") | ||
| @skip_xp_backends("dask.array", reason="output array is read-only.") | ||
| @skip_xp_backends("dask.array", reason="output kw doesn't make sense for dask") |
There was a problem hiding this comment.
A fully Array API compliant function would be able to suppor the output= kwarg with dask.
The problem is that you're calling np.asarray(x) will always return a buffer that is not shared with the input parameter.
Please change reason to "converts dask output array to numpy"
Co-authored-by: Guido Imperiale <crusaderky@gmail.com>
|
Thanks for the review |
Reference issue
What does this implement/fix?
Additional information