Skip to content

DOC: timezone warning for dates beyond TODAY #34100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 21, 2020
Merged

DOC: timezone warning for dates beyond TODAY #34100

merged 4 commits into from
Aug 21, 2020

Conversation

joooeey
Copy link
Contributor

@joooeey joooeey commented May 10, 2020

introducing a suggestion discussed in PR #33863 :
Added a warning in the user guide that timezone conversion on future dates is inherently unreliable.

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

introducing a suggestion discussed in PR #33863 :
Added a warning in the user guide that timezone conversion on future dates is inherently unreliable.
@@ -2314,13 +2314,24 @@ you can use the ``tz_convert`` method.
Instead, the datetime needs to be localized using the ``localize`` method
on the ``pytz`` time zone object.

.. warning::

Be aware that for times in the future, correct conversion between time zones
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of adding a new warning box, the last sentence of the warning box below should be Be aware that for times in the future, correct conversion between time zones (and UTC) cannot be guaranteed by any time zone library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel strongly that this should be a separate warning. These are two different issues (even though the solution is basically the same), and one sentence could easily be overlooked in the big 2038 warning. Here's my rationale as I said int he discussion at #33863 :

Okay. I'm convinced now. We should warn about converting (any) future time. I still think this warning shouldn't be an afterthought in the 2038 warning text. It should be its own warning box. That gives it the prominence it deserves and ensures the warning remains if the 2038 problem is resolved in the downstream libraries and that box is removed.

In addition, the example given in the warning below only concerns the 2038 problem. How is anyone reading the docs supposed to know that the example only concerns the first part of the warning (the 2038 problem) but not the last sentence. It's just confusing that way.

What do you think about adding the warning I proposed but only with the first sentence?

Copy link
Member

Choose a reason for hiding this comment

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

Okay maybe you can combine this warning and the 2038 warning into 1 warning, highlighting your point first and the 2038 issue as a ramification.

Just want to reduce the amount of warnings in this section since there's already a lot of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is a longer warning better than two warnings? Of the five warnings here, the first and second warnings about differing timezone definitions between libraries and library versions respectively are also not merged into one. For good reason, I would say. Keeping five separate warnings makes the structure quickest to comprehend and hence the user guide easiest to read, which I think is the ultimate goal. That's why there should be a separate, succinct warning about time zone conversions for future dates.

Copy link
Member

Choose a reason for hiding this comment

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

I would agree this can at least be shortened a bit. We don't need to single out Morocco

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just committed a change to make it shorter but CI / Web and Docs throws: AttributeError: module 'numpy.random' has no attribute 'BitGenerator'. I have no idea what that means.

@jreback jreback added Docs Timezones Timezone data dtype labels May 12, 2020
@jreback jreback added this to the 1.1 milestone May 12, 2020
@jreback
Copy link
Contributor

jreback commented Jun 20, 2020

what is the status here?

@jreback jreback removed this from the 1.1 milestone Jun 20, 2020
@mroeschke
Copy link
Member

LGTM. Just looks like master needs to be merged in

@joooeey
Copy link
Contributor Author

joooeey commented Jun 21, 2020

what is the status here?

I don't have a working dev environment and no time to set it up. I also tried to figure out if this can be done on Github only (without using my local machine) but couldn't find a way to merge master into my GH repository online. Feel free to take over the PR and fix the one CI error it still throws. Or I'll get to it when I get to it -- that is sometime this year.

@joooeey
Copy link
Contributor Author

joooeey commented Jul 12, 2020

I merged in master but there are again CI failures unrelated to the docs. Why is it so hard to get a simple doc change in?

@MarcoGorelli
Copy link
Member

LGTM. Just looks like master needs to be merged in

Have merged master, CI passes now - still happy with it @mroeschke ?

@MarcoGorelli MarcoGorelli added this to the 1.2 milestone Aug 21, 2020
@MarcoGorelli MarcoGorelli merged commit 8f79543 into pandas-dev:master Aug 21, 2020
@MarcoGorelli
Copy link
Member

Thanks @joooeey !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Timezones Timezone data dtype
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants