Skip to content

DEP: Remove datetime64 deprecation warning when constructing with timezone#24193

Merged
mattip merged 19 commits intonumpy:mainfrom
rmhowe425:dev/datetime64-depr_warning
Feb 7, 2024
Merged

DEP: Remove datetime64 deprecation warning when constructing with timezone#24193
mattip merged 19 commits intonumpy:mainfrom
rmhowe425:dev/datetime64-depr_warning

Conversation

@rmhowe425
Copy link
Contributor

This commit changes the deprecation warning for np.datetime64 to a UserWarning. Updates to the doc string for np.datetime64 are also provided to explicitly state that timezones for datetime64 objects are not available.

Closes #23904

@rmhowe425 rmhowe425 changed the title ENH: datetime64: Remove deprecation warning when constructing with timezone ENH: Remove datetime64 deprecation warning when constructing with timezone Jul 17, 2023
@seberg
Copy link
Member

seberg commented Jul 17, 2023

Thanks, it's too bad that we didn't have one, but could you add a test to cover the path when a datetime.datetime object includes timezone information? (C-code coverage says that this path is never hit.)

or datetime format.

When parsing a string to create a datetime object, if the string contains
a timezone, the timezone will be dropped.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@charris charris changed the title ENH: Remove datetime64 deprecation warning when constructing with timezone DEP: Remove datetime64 deprecation warning when constructing with timezone Jul 22, 2023
@charris
Copy link
Member

charris commented Jul 22, 2023

Could use a release note.

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Jul 22, 2023
@charris charris added this to the 1.25.2 release milestone Jul 25, 2023
@rmhowe425 rmhowe425 requested a review from seberg July 30, 2023 00:19
@rmhowe425
Copy link
Contributor Author

@seberg Looks like I have 4 tests failing because of: ERROR: Problem encountered: No BLAS library detected!.

Is this issue stemming from something that I have done, or the build environment?

@mattip
Copy link
Member

mattip commented Jul 30, 2023

Probably the CI changes. You may need to rebase off main and force push or merge main into your branch.

@rmhowe425
Copy link
Contributor Author

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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests no longer belong in this file, which is intended to test deprecations. Are the use cases covered in other tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point! Moving the tests to test_datetime.py.

From what I can tell, these tests are not covered in other places.

@rmhowe425 rmhowe425 requested a review from mattip July 30, 2023 18:18
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +290 to +292

Datetime64 object should be considered to be UTC and therefore have an
offset of +0000.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Please undo this indent. You can over-indent the above.

@charris
Copy link
Member

charris commented Sep 2, 2023

close/reopen

@charris charris closed this Sep 2, 2023
@charris charris reopened this Sep 2, 2023
@charris
Copy link
Member

charris commented Sep 5, 2023

Be nice to finish this up. The test failure looks irrelevant and should go away if this is rebased.

@charris charris removed this from the 1.26.0 release milestone Sep 16, 2023
@melissawm
Copy link
Member

@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!

@rmhowe425
Copy link
Contributor Author

Working on fixing this now!

@rmhowe425 rmhowe425 requested a review from seberg November 26, 2023 19:21
@rmhowe425
Copy link
Contributor Author

@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

@melissawm
Copy link
Member

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?

@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Feb 5, 2024
@charris charris removed this from the 1.26.4 release milestone Feb 5, 2024
@charris
Copy link
Member

charris commented Feb 5, 2024

This needs a final review.

@charris charris added this to the 2.0.0 release milestone Feb 5, 2024
Copy link
Member

@mattip mattip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@mattip
Copy link
Member

mattip commented Feb 5, 2024

I don't really recall discussing it but ...

@seberg should we re-open this for discussion?

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the changes here agree with the discussion in #23904 and the triage meeting, so +1 from me as well!

@mattip mattip merged commit a315987 into numpy:main Feb 7, 2024
@mattip
Copy link
Member

mattip commented Feb 7, 2024

Thanks @rmhowe425

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

ENH: datetime64: Remove deprecation warning when constructing with timezone

6 participants