DEP: Deprecate 'generic' unit in np.timedelta64 - #29619
Conversation
| # Ignore DeprecationWarning from typing.mypy_plugin | ||
| ignore:`numpy.typing.mypy_plugin` is deprecated:DeprecationWarning | ||
| # Ignore Runtime Warning from datetime by calculating unitless value and unitful value | ||
| ignore:Casting from unitless timedelta to unitful timedelta is ambiguous.:RuntimeWarning |
There was a problem hiding this comment.
I believe that if we do this, we'd likely want at least one test case that actually makes sure that the warning is issued.
I see locally that there are 18 failures if this is removed though, and we wouldn't want 18 checks for the warning. Still, it might be nice if we could suppress most of them but enforce at least one of them.
That said, the discussion in the matching issue suggests that the decision here may be tricky, so it may be best to wait for some design feedback first before making changes.
There was a problem hiding this comment.
Thank you for the feedback!
I agree that adding a test case to check this warning makes sense.
As you suggested, I’ll wait for the design feedback before making the change.
There was a problem hiding this comment.
According to the issue discussion and the triage meeting, deprecation of the np.generic unit has been decided. I have updated the test implementation as you suggested.
I would appreciate it if you could take a look.
|
Needs a release note. |
7b3b00a to
902f4ac
Compare
93ae278 to
9e14d71
Compare
np.timedelta64 to unitful one.
@charris |
|
@jbrockmendel I wonder if you have thoughts on this. I think the idea now was to deprecate any unitless scalars except for NaT (I suppose 0 may be another plausible exception). The hope would be that we may still need NaT as a unitless scalar, but overall try to make it hard or impossible to work with this. I think the scalar trick may well be good. But we may need some deeper stuff to really disable this fully. Since I think you can still cast from int (or view), etc. (But maybe it can be a follow-up too, I am mostly curious if it seems safe to remove almost all unitless datetimes from a pandas perspective.) |
That won't cause us any problems, might even allow us to clean up some checking-for-it code. |
| sys.path.append(str(build_dir)) | ||
|
|
||
|
|
||
| @pytest.mark.filterwarnings("ignore::FutureWarning") |
There was a problem hiding this comment.
Rather than a general filter, could you assert that the warning is raised when expected
There was a problem hiding this comment.
Thanks for the comment! I've updated the test to assert that the warning is raised when expected.
pytest.mark.filterwarnings is now used only in numpy/typing/tests/test_typing.py to avoid failures when this test loads a Python script file that uses the generic unit. I can update it further if needed.
| # # m8 generic units | ||
| # (np.timedelta64(1890), | ||
| # np.timedelta64(31), | ||
| # 60), |
There was a problem hiding this comment.
Is there a test in test_deprecations that asserts that these calls now warn?
There was a problem hiding this comment.
Yes! I've implemented it as test_generic_timedelta_floor_divide in test_datetime.py.
| def test_raise_warning_for_timedelta_with_generic_unit(self, value: int): | ||
| msg = "Using 'generic' unit for NumPy timedelta is deprecated" | ||
| with pytest.warns(FutureWarning, match=msg): | ||
| _ = np.timedelta64(value) |
There was a problem hiding this comment.
I think this test these tests should be moved to test_deprecations
There was a problem hiding this comment.
I've moved the relevant tests to test_deprecations.py.
91b812e to
acbcdf2
Compare
|
It seems that some CI jobs are failing (especially those related to Python 3.11). I’m investigating it now. Update: I've resolved them. |
2a42032 to
c943f3a
Compare
|
@seberg |
|
@riku-sakamoto yes, since pandas is likely fine, I think we can give it a shot. But we need to wait another few weeks until branching unfortunatley. |
7445340 to
c43c82d
Compare
|
Thank you all for the last triage meeting.
Just to note, the CI failure does not seem to be related to this PR. |
seberg
left a comment
There was a problem hiding this comment.
Thanks, we had discussed this a few times and in general agreed to try this.
I'll ping the mailing list as well. If there is downstream fallout, we should consider reverting this. From pandas perspective this is the right step, but I am not 100% sure if it a painless one either way.
Astropy might also notice it (but they test against nighties well).
In general, removing the ability to mix integers with timedelta/datetimes seems right and this is probably the most gentle first step.
We could even re-instate specific things like arr + 1 e.g. ufuncs and comparisons that mix with integers (or just Python integers).
|
@seberg Thank you for the review and for merging this! |
|
There is some fallout here. I half think the easiest solution is to change behavior (I guess with another small release note) so that Rethinking it, there some subtleties. I.e. (That should be solvable, in practice there is still a clear path here. Just posting in case you (or someone else) is interested in it. It would be nice to solve this) |
|
Eh, bit late, but |
|
The warning for adding integers to a unit-full timedelta64 is also a bit confusing I think: It might be confusing because |
|
Hmmm, maybe we could ammend the warning message to mention it? That part could be changed, but it would remove a lot of the point, adding special paths for this feels a bit much maybe. One thing I am not sure about is, if you were to change the default to |
|
Thanks for the feedback. @seberg @jorenham I’ve opened a follow-up issue to discuss the default behavior of Regarding the warning message, I can also open a separate issue if needed. |
sure, sounds fine |
This PR deprecates the
genericunit innp.timedelta64. Using this unit can lead to unexpected behavior in some cases (see #28287 for details). Usinggenericunit now raises aDeprecationWarning.Changes
Main
DeprecationWarningwhennp.timedelta64is constructed with thegenericunit.np.onesandnp.ones_like.Test
Test updates fall into three categories.
Suppressing the warning in tests where the
genericunit seems to be used intentionally. (with pytest.warns(DeprecationWarning))Adding tests to check
genericunit's behavior. Some tests usesgenerictimedelta inpytest.mark.parametrize. In such cases, the warnings cannot be suppressed withpytest.mark.filterwarnings. Instead, this PR add additional tests to check onlygenericunit's behavior.Modifying tests to use an explicit unit instead of
generic.Future follow up items
np.timedelta64()creates agenerictimedelta with value0. We may want to change this to create a timedelta with an explicit unit (e.g.,np.timedelta64(0, 's')) instead.np.ones_likenow raisesDeprecationWarningwhen the input array is of timedelta type.Resolvednp.all_closenow raisesDeprecationWarningwhen the input arrays are timedelta type.Resolved