-
-
Notifications
You must be signed in to change notification settings - Fork 11.2k
DEP: Deprecate setting the shape attribute of an exact numpy array #29523
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
Looks like there's one docstring that still needs an update:
|
an array is unsafe if an array is shared, especially by multiple | ||
threads. As an alternative, you can create a new view (no copy) via | ||
`np.reshape` or `array.reshape`. | ||
|
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.
any chance you can add a more fleshed out concrete code example showing exactly what to do?
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.
I wonder if someone could write a ruff lint for this?
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.
A NPY
rule would indeed be nice. But those of us that use a type-checker should already see a deprecation message appear in their IDE.
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.
The nice thing about adding ruff rules is it can automatically fix issues for you.
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.
Can you add a ruff rule though? You have a new object, so it just isn't a safe rule to just apply (in part for the exact same reason that we want this, because it is weird to change the shape on the other side of a program).
It seems possible, but rather raw:
(I suppose you may be able to walk the I still think option iv might be nicer, just because the need for this should only arise in the subclass implementation itself and not for end-users. But happy to go with this initially (even this might be quite noisy, of course). |
For setting the shape the only complex part is in |
Matrix deprecation always waited on SciPy sparse matrix being replaced. That process finally started, but it may not be quite far enough yet to actually do it. |
Ah, sorry, I see you just had to change how the shape is handled there (i.e. that weird offender) and not actually need to get rid of it. To me that seems actually pretty nice and not too complex if it works! |
Addresses part of #28800.
Notes:
for small arrays the changes could have a minor performance impact as x.reshape(shape) returns a view (e.g. a new object). We could optimize perhaps by returning the same array if the shape is unchanged, but I am not sure this is worth it.
Setting the shape in
__array_finalize__
(e.g. here) is problematic: using reshape is not possible as we have to modify the self. This is seems rather safe (since we are still in__array_finalize__
it would be strange for other threads to already have access to the object), but we have to deal with it somehow.Some options: i) suppress the warning here ii) issue the warning only for exact np.ndarray iii) refactor the matrixlib (and others) to not set the shape iv) use in private method to set the shape.
Option i) is not very performant iii) I do not know how to do, iv) might give issues with subclasses that override setting the shape (is that possible?). So I am picking option ii) for now (same approach as in the PR where dtype setting is deprecated