Repository navigation
DEP: Deprecate setting the dtype of an exact numpy.ndarray - #29244
eendebakpt wants to merge 14 commits into
Conversation
3ad7ba0 to
401cbca
Compare
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
I had some old comments and thought I'd submit the review.
What I think I should ponder a bit is how much we need this "allow subclasses" thing. It just doesn't feel very neat TBH, even using _set_dtype() semi-publically a lot would seem nicer, but not sure that is feasible either.
| threads. As an alternative, you can create a new view (no copy) via: | ||
| * `np.lib.stride_tricks.strided_window_view` if applicable, | ||
| * `np.lib.stride_tricks.as_strided` for the general case, | ||
| * or the `np.ndarray` constructor (``buffer`` is the original array) for a light-weight version. |
There was a problem hiding this comment.
You should be using arr.view() for this, that is what it is for!
|
|
||
| #if !defined(PYPY_VERSION) | ||
| // On PyPy we skip the warning because we cannot rely on reference counts | ||
| // to be replaced with PyUnstable_Object_IsUniquelyReferenced https://github.com/python/cpython/pull/133144 |
There was a problem hiding this comment.
We are already using this in check_unique_temporary based on the Python version, would be nice to unify it.
(Maybe also with resize.)
There was a problem hiding this comment.
I looked at check_unique_temporary and the goal is difference, so unification does not seem worthwhile at this stage.
Here we want to check we have a guaranteed non-unique temporary. I did refactor a bit.
| assert arr.strides == (0,) | ||
| with pytest.raises(ValueError): | ||
| arr.dtype = "i1" | ||
| with warnings.catch_warnings(): # gh-28901 |
There was a problem hiding this comment.
Please just use pytest.warns(). That way you don't keep it around forever.
There was a problem hiding this comment.
Using pytest.warns makes the test fail on pypy (there no deprecation warning is generated). If you want, I can skip this test on pypy instead of using the catch_warnings, but it seems have to pick the lesser of two evils.
| # The route via `__interface__` does not preserve structured | ||
| # dtypes. Since dtype should remain unchanged, we set it explicitly. | ||
| array.dtype = x.dtype | ||
| array._set_dtype(x.dtype) |
There was a problem hiding this comment.
Do you even still need this here? At this point, it's a base class and you can just assign?
There was a problem hiding this comment.
The array is indeed a base class due to the asarray. But if I assign to dtype here it will generate a deprecation warning.
Maybe we can combine it with the line above as
array = np.asarray(DummyArray(interface, base=x), dtype=dtype)
But I am not sure this covers all the corner cases.
I would prefer to leave this as is for now to keep this PR as safe and simple as possible. At some later point we will probably have to tackle this one (and some others).
Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
| if p.ls[-1] == 'simplefilter' or p.ls[-1] == 'filterwarnings': | ||
| if getattr(node.args[0], "value", None) == "ignore": | ||
| if node.args and getattr(node.args[0], "value", None) == "ignore": |
There was a problem hiding this comment.
I know it's very much out of scope, so please ignore my neurotic rant, but this could be so much prettier with a match / case 😅
I created #29575 with the |
|
Closing in favor of #29575 |
Partly addresses #28800. A continuation of #28901.
We only deprecate setting the dtype of an exact array. For masked and record arrays it will take some more refactoring to avoid setting the dtype. We skip warnings for pypy as we cannot rely on reference counting to avoid warnings when the dtype is changed via
array.view(new_dtype).