DEP: Deprecate .T property for non-2dim arrays and scalars#28678
Conversation
mtsokol
added
03 - Maintenance
40 - array API standard
mtsokol
force-pushed
the
T-non-2dim-depr
branch
from
7772cdc to
80bf35a
Compare
mtsokol
force-pushed
the
T-non-2dim-depr
branch
2 times, most recently
from
621e40a to
0163784
Compare
mtsokol
force-pushed
the
T-non-2dim-depr
branch
from
0163784 to
76387e3
Compare
That's right - done! |
|
To get docs build green we need matplotlib/matplotlib#29896 in first (and matplotlib bugfix release) |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
Given the number of places the tests needed to be updated, I bet this will be a noisy deprecation. Maybe worth pinging the mailing list?
| "In the future `.T` property will be supported for " | ||
| "2-dim arrays only. Received %d-dim array. Either " | ||
| "`np.permute_dims(arr, range(arr.ndim)[::-1])` " | ||
| "(compatible with the Array API) or `arr.transpose()` " |
There was a problem hiding this comment.
I would delete the note and at least re-order this. It nudges users to something less convenient for what seems very little reason to me.
A user who needs array API is a library author and will already not use .T here anyway.
It may be good to note that .mT exists to swap the last two axes only (If just to increase awareness).
There was a problem hiding this comment.
And what if someone uses .T to reverse axes for ndim>2 cases anyway? I can remove Array API suggestion but how about keeping arr.transpose() for people who want to retain existing behavior.
There was a problem hiding this comment.
I think that's what Sebastian is saying - to leave arr.transpose() or at least mention it first and mention .mT as well.
There was a problem hiding this comment.
I feel it currently makes it seem that arr.transpose() is somehow a not the preferred solution, but for those users who run into it it will almost certainly be the preferred one.
There was a problem hiding this comment.
Thanks for an explanation! I updated the message as you suggested.
|
|
||
| def test_deprecated_T_non_2dim(): | ||
| # Deprecated in Numpy 2.3, 2025-04 | ||
| with pytest.warns(UserWarning, match="In the future `.T` property for " |
There was a problem hiding this comment.
Please use assert_deprecated as all other tests in this file. That ensures that the error path is also tested, even if that is rather trivial here.
There was a problem hiding this comment.
I didn't use assert_deprecated because it's missing match= parameter to actually match the warning message, but I can use it instead.
There was a problem hiding this comment.
because it's missing match= parameter to actually match the warning message
Seems reasonable to add it, assuming that's easy.
There was a problem hiding this comment.
IIRC it does, but takes the message from the class, but not sure. And yeah, I don't actually care about using it but I do care abot a habit of testing the error path.
There was a problem hiding this comment.
because it's missing match= parameter to actually match the warning message
Seems reasonable to add it, assuming that's easy.
I like this idea - added msg_patterns argument to assert_deprecated.
| { | ||
| int ndim = PyArray_NDIM(self); | ||
| if (ndim != 2) { | ||
| if (PyErr_WarnFormat(PyExc_UserWarning, 1, |
There was a problem hiding this comment.
Why a UserWarning? Please use a DeprecationWarning, or actually, use the DEPRECATE macro.
We could use a visible deprecation warning if we want to inform new users who accidentally get it wrong. But only if we take the trouble to check that very few libraries use this (i.e. there are few places in existing code where DeprecationWarning would be hidden, but VisibleDeprecationWarning wouldn't be).
There was a problem hiding this comment.
Given the number of places the tests needed to be updated, I bet this will be a noisy deprecation.
So, it must clearly be a DeprecationWarning. This should be taken very slowly, i.e. in a year or so a VisibleDeprecationWarning.
There was a problem hiding this comment.
Done - for array scalars in scalartypes.c.src I used DEPRECATE macro, but here I kept PyErr_WarnFormat, now with a DeprecationWarning, because it allows formatting contrary to DEPRECATE.
|
@seberg Yesterday new matplotlib version got released. Docs build is passing so this PR can go in as well now. |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
I didn't look carefully at the code but the release notes and deprecation warnings look good to me (after applying some minor grammar fixes).
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Code looks fine, although I don't like the test code addition. It seems unnecessary and thus also unnecessarily complex.
Why not just make two classes and use the already existing mechanism or just use the fact that it is an re matching already probably (we don't need to test that it's the scalar message says scalar)?
| if re.match(pattern, msg) is None: | ||
| raise AssertionError( | ||
| "expected %s warning message pattern but got: %s" % | ||
| (pattern, msg)) |
There was a problem hiding this comment.
This doesn't look nice to me. We already have message = on the class, and that works just fine, it is used as message=self.message below.
It's fair that passing a pattern is maybe nicer (especially with pytest.warns existing now). But we should avoid unnecessary duplication.
(And since we don't need the tuple of patterns)
There was a problem hiding this comment.
I gave it another try and indeed msg_patterns wasn't needed after all - I removed it.
Instead I created two separate classes with different message regexes.
| for warning in self.log: | ||
| if msg_patterns is not None: | ||
| pattern = (msg_patterns if isinstance(msg_patterns, str) else | ||
| msg_patterns[num_found]) |
There was a problem hiding this comment.
Also, you don't use this and the num_found was a work-around for identical ones mostly, IIRC?
There was a problem hiding this comment.
Right, I got rid of the custom msg_patterns logic after all.
seberg
changed the title
.T property for non-2dim arrays and scalars.T property for non-2dim arrays and scalars
|
Needs rebase. |
Done! |
|
Thanks @mtsokol . |
|
This broke SciPy CI badly, on the last day before creating the release branch. Hence I'll revert this straight away - let's find a better time to reintroduce this, and ideally after assessing/mitigating downstream damage. |
|
Reverted in gh-28990. If the 2.3.x branch is close, this should probably wait. We can't merge something like this without doing what we did for 2.0 I think - first testing with SciPy, scikit-learn, Pandas, Matplotlib at least, and addressing the issues there. That will also show whether the impact is actually fine or not. |
|
I only adjusted matplotlib out of these four. Let's then move this change to 2.4.0 and in the meantime I'll make sure the rest of them are also ready. |
7 participants
As the title says, this PR deprecates
.Tproperty for non-2dim arrays and scalars, in order to be compatible with the Array API: link.