DEP: Remove datetime64 deprecation warning when constructing with timezone#24193
DEP: Remove datetime64 deprecation warning when constructing with timezone#24193mattip merged 19 commits intonumpy:mainfrom
Conversation
|
Thanks, it's too bad that we didn't have one, but could you add a test to cover the path when a |
numpy/core/_add_newdocs_scalars.py
Outdated
| or datetime format. | ||
|
|
||
| When parsing a string to create a datetime object, if the string contains | ||
| a timezone, the timezone will be dropped. |
There was a problem hiding this comment.
What's the exact logic? A userwarning is given (I think it would be good to mention), we do have a Warnings heading in principle, too.
Can we explain (and check) what we actually do? And do we actually want to keep it for all of them?
- Timezones differences without the Z are simply applied
- A Z (and I guess also trailing) is simply ignored, the naive timezone is assumed to be UTC?
Is that correct?
|
Could use a release note. |
|
@seberg Looks like I have 4 tests failing because of: Is this issue stemming from something that I have done, or the build environment? |
|
Probably the CI changes. You may need to rebase off main and force push or merge main into your branch. |
|
Thanks @mattip! That fixed my issue! |
| with pytest.warns(UserWarning, match=msg): | ||
| t0 = np.datetime64('2023-06-09T12:18:40Z', 'ns') | ||
|
|
||
| t0 = np.datetime64('2023-06-09T12:18:40', 'ns') |
There was a problem hiding this comment.
The tests no longer belong in this file, which is intended to test deprecations. Are the use cases covered in other tests?
There was a problem hiding this comment.
Ah good point! Moving the tests to test_datetime.py.
From what I can tell, these tests are not covered in other places.
…ecation messages.
seberg
left a comment
There was a problem hiding this comment.
I still think it isn't really clear what is going on, I don't really recall discussing it but I guess we did and the docs are the only thing that needs updating.
I think numpy datetimes are naive but we are parsing them as if they were UTC?
I just will note that upgrading the warning might actually make it a lot more noisy in practice (because not everyone sees DeprecationWarnings).
|
|
||
| Datetime64 object should be considered to be UTC and therefore have an | ||
| offset of +0000. |
There was a problem hiding this comment.
| Datetime64 object should be considered to be UTC and therefore have an | |
| offset of +0000. | |
| For the purpose of parsing time offsets the timezone naive `datetime64` | |
| behaves as if converting to UTC. |
Maybe? I don't think the old sentence is correct, the warning is there for a reason after all.
Probably this could be part of a Notes section?
| return -1; | ||
| } | ||
| if (PyErr_WarnEx(PyExc_UserWarning, | ||
| "no explicit representation of timezones available for np.datetime64", |
There was a problem hiding this comment.
This seems not very clear to me? Maybe also mention that parsing to naive datetime but handling timezone as if we were UTC?
(If that is correct)
| "no explicit representation of timezones available for np.datetime64", | ||
| 1) < 0) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
NIT: Please undo this indent. You can over-indent the above.
|
close/reopen |
|
Be nice to finish this up. The test failure looks irrelevant and should go away if this is rebased. |
|
@rmhowe425 would you like to finish this up? If you are unsure about the rebase we can do that for you, let us know. Cheers! |
|
Working on fixing this now! |
…ecation messages.
|
@melissawm I rebased and all of my tests are passing. The number of lines contributed for my PR does not appear to be correct.....I'm hoping that this won't interfere with reviewing my PR |
|
It looks like something went wrong on your rebase, as you now have a large number of unrelated commits listed here. Do you think you can take a look to see what happened? |
|
This needs a final review. |
@seberg should we re-open this for discussion? |
rossbar
left a comment
There was a problem hiding this comment.
FWIW the changes here agree with the discussion in #23904 and the triage meeting, so +1 from me as well!
Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
|
Thanks @rmhowe425 |
This commit changes the deprecation warning for
np.datetime64to a UserWarning. Updates to the doc string fornp.datetime64are also provided to explicitly state that timezones for datetime64 objects are not available.Closes #23904