DEP: Deprecate 'generic' unit in np.timedelta64 and np.datetime64 #29619
Conversation
pytest.ini
Outdated
| # 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. |
numpy/_core/tests/test_cython.py
Outdated
| 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.
numpy/_core/tests/test_datetime.py
Outdated
| # # 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.
numpy/_core/tests/test_datetime.py
Outdated
| 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. |
|
@seberg |
…4` to unitful one. This is a temporary solution to handle numpy#28287. Rasing `RuntimeWarning` encourages users to pay attention to the behavior.
….datetime64` The ``generic`` unit in `numpy.timedelta64` and `numpy.datetime64` is now deprecated. Using it will raise a ``FutureWarning``. (See numpy#28287)
* Asserting warnings with the warns function¶ * Some tests are moved to `test_deprecations.py`
c943f3a to
b9853f7
Compare
b9853f7 to
8f1daf2
Compare
|
@seberg I've updated the PR to raise a warning for unitless |
(Updated from previous PR description.)
This PR deprecates the
genericunit innp.timedelta64andnp.datetime64. Using this unit can lead to unexpected behavior in some cases (see #28287 for details). Usinggenericunit now raises aDeprecationWarningFutureWarningChanges
Main
DeprecationWarningFutureWarningnp.timedelta64ornp.datetime64is constructed with thegenericunit.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 Work
These works are not included in this PR and I am planning to do them in future PRs. But, if you think they should be included in this PR, please let me know. I am happy to do so.
NaTis allowed withgenericunit. We may want to disallow this in future.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 raisesFutureWarningwhen the input array is of timedelta type.1value with specific unit.np.all_closenow raisesFutureWarningwhen the input arrays are of datetime or timedelta type.atolis not allowed to betimedelta64because it is supposed to be printed withgformat. ("{atol:g}"in the code.)