ENH: Add support to sort timedelta64 NaT to end of the array
#15068
Conversation
tylerjereddy
left a comment
tylerjereddy
left a comment
There was a problem hiding this comment.
This looks sensible---relatively easy to review given similarity to #12658 I think. I added a comment about the tests, but could always be a follow-up refinement thing.
numpy/core/tests/test_datetime.py
Outdated
|
|
||
| @pytest.mark.parametrize("size", [ | ||
| 3, 21, 217, 1000]) | ||
| def test_timedelta64_nat_argsort_stability(self, size): |
There was a problem hiding this comment.
Because this test and the one below are effectively copy/paste with M8 -> m8 from the first PR, we could also consider parametrizing over those two dtypes to reduce duplication. Maybe I just get carried away with the parameters though :) It would take a little more work, so not sure on the tradeoff value for maintainability.
There was a problem hiding this comment.
Done! Looks lot cleaner now. Thanks!
charris
changed the title
NaT to end of the arrayNaT to end of the array
|
Needs a release note. |
numpy/core/tests/test_datetime.py
Outdated
| (np.array([[51, -220, 'NaT'], | ||
| [-17, 'NaT', -90]], dtype='m8[us]'), | ||
| np.array([[-220, 51, 'NaT'], | ||
| [-90, -17, 'NaT']], dtype='m8[us]')), |
There was a problem hiding this comment.
Since this only uses ints and NaT strings. You could use two paramterizations: One for the dtype (maybe without the unit) and one for the list. And then move the np.array call into the test itself. That way you have half the code with the same result.
There was a problem hiding this comment.
Done. Thanks! 👍
|
Should this get a 1.18 milestone or is the "backport candidate" enough to make sure it is included in the 1.18 release? |
|
I use the |
|
Thanks, for jumping and fixing it quickly! |
…gh-15068) This work is a follow up of numpygh-12658. As requested in numpygh-15063, add NaT sort support for timedelta64 datatypes also. Fixes numpygh-15063
charris
added
01 - Enhancement
and removed
09 - Backport-Candidate
In numpy#12658 and numpy#15068 the sort ordering for datetime and timedelta was changed so that NaT sorts to the end, but the internal compare array function was not updated. Fixes numpy#19574.
…edelta In numpy#12658 and numpy#15068 the sort ordering for datetime and timedelta was changed so that NaT sorts to the end, but the internal compare array function was not updated. Fixes numpy#19574.
…edelta In numpy#12658 and numpy#15068 the sort ordering for datetime and timedelta was changed so that NaT sorts to the end, but the internal compare array function was not updated. Fixes numpy#19574.
…edelta In numpy#12658 and numpy#15068 the sort ordering for datetime and timedelta was changed so that NaT sorts to the end, but the internal compare array function was not updated. Fixes numpy#19574.
5 participants
Fixes #15063
This work is a follow up of #12658.
As requested in #15063, add NaT sort support for
timedelta64datatypes also.