BUG,MAINT: Fix __array__ bugs and simplify code #26142
Conversation
seberg
force-pushed
the
fixup-array-copy
branch
from
ab4e511
to
3aa976b
Compare
This fixes that the UserWarning is incorrect, it needs to be a DeprecationWarning (it does not related to end-users). Instead, include the information about `copy=None` into the error message. It also fixes a bug: If `copy=` kwarg is unsupported we cannot guarantee no-copy, so we should raise, or did we discuss to "just warn"? That is also a latent bug: When `copy=True` and the fallback path is taken, then we must make a copy to be on the safe side.
seberg
force-pushed
the
fixup-array-copy
branch
from
3aa976b
to
2c08c3f
Compare
numpy/_core/src/multiarray/ctors.c
Outdated
| "Unable to avoid copy while creating an array as requested.\n" | ||
| "If using `np.array(obj, copy=False)` use `np.asarray(obj)` " | ||
| "or `copy=None` to allow NumPy to make the copy.\n" | ||
| "This changed in NumPy 2. The suggested fix works on all versions."); |
There was a problem hiding this comment.
This is not quite right, copy=None does not work on numpy 1.x:
In [2]: np.array(1, copy=None)
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
Cell In [2], line 1
----> 1 np.array(1, copy=None)
ValueError: NoneType copy mode not allowed.
There was a problem hiding this comment.
🤯, I am really surprised that this doesn't work! @mtsokol what would be the suggestion then, just np.asarray? It means that forwarding a kwarg like:
def myfunc(arr, copy=None):
return np.array(arr, copy=copy)
has no trivial cross-NumPy solution!?
There was a problem hiding this comment.
Right, for NumPy 1.x and 2.0 np.asarray(arr) is necessary. In Pandas in copy arg tests we did (pandas-dev/pandas#57172):
copy_false = None if np_version_gt2 else False
There was a problem hiding this comment.
I removed the copy=None suggestion and added a half sentence for now. But maybe we need to mention that you may need copy_if_needed = None if np_version_gt2 else False to do this right?
There was a problem hiding this comment.
Indeed, in astropy we do the same: COPY_IF_NEEDED = False if NUMPY_LT_2_0 else None
Since that was just copied from scipy, I think it makes sense to include it in the error message...
There was a problem hiding this comment.
I am wondering if it makes more sense to put a link to the migration guide when it comes beyond pointing to np.asarray()?
There was a problem hiding this comment.
Yes, putting a link makes sense!
There was a problem hiding this comment.
Tweaked it a bit, the link is unfortunately long, but probably just shouldn't worry about it :). Better too verbose than confusing!
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
But need to reduce the line length of one of the added lines... |
|
TBH, tempted to ignore the linter for once for a slightly long line (maybe we really need to start using auto-reformatting). |
|
Thanks Sebastian. I tend to be a bit lax on linting test code. |
This fixes that the UserWarning is incorrect, it needs to be a DeprecationWarning (it does not related to end-users). Instead, include the information about
copy=Noneinto the error message.It also fixes a bug: If
copy=kwarg is unsupported we cannot guarantee no-copy, so we should raise, or did we discuss to "just warn"?That is also a latent bug: When
copy=Trueand the fallback path is taken, then we must make a copy to be on the safe side.Fixes a couple of smaller error checking bugs. Vectorcall allows for simpler code, I think. The speed improvement is only around 10%, since there are other larger overheads anyway.
Or am I forgetting a discussion for why
UserWarningmakes sense @ngoldbaum and @mtsokol?