Warn when creating `Period` with a string that includes timezone information #47990

joooeey · 2022-08-06T13:42:20Z

closes BUG: PeriodIndex looses tz information without emitting a warning that the timezone is dropped. #47005
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions. -- no new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

Note on the new `DateTimeWarning`

I added the DateTimeWarning for this specific case and the warnings raised in #22549 because the commonly used UserWarning is often not appropriate. There are lots of conceivable situations where this warning may be raised due to design choices in Pandas as opposed to user error. DateTimeWarning inherits from UserWarning to remain backwards compatible. This trail of thought applies to much of the datetime functionality in Pandas but that's beyond the scope of this PR.

joooeey · 2022-08-06T13:52:03Z

pandas/tests/indexes/period/test_period_range.py

-    @pytest.mark.parametrize("freq", ["D", "W", "M", "Q", "A"])
+    @pytest.mark.parametrize("freq", ["D", "M", "Q", "A"])


The "W" here tests for functionality that is not properly implemented in Pandas. Namely creating a Period from a string representing a week such as "2017-01-23/2017-01-29". This only works by chance and doesn't work for weeks in the 60s, 70s, 80s, 90s of any century or any year from 2360 onwards. This is because dateutil.parser.parse interprets the second year as hours and minutes.

This is a bit sad because the given string is Pandas' string representation of a week period and it would be nice to have bi-directionality.

Could you address this in a separate PR just to keep this PR tightly scoped?

I could but doing so would stall this PR until the "W" PR is merged. The two test cases I changed raise the warning introduced in this PR. How should we proceed?

Not sure I fully understand where and how a "W" frequency in particular is impacted by this new warning.

The two tests that I propose to change create a Period, convert it to a str, and then try to create a Period again from the string while also telling the constructor what the freq is. Such a functionality isn't deliberately implemented anywhere for "W", the code path goes to dateutil.parser.parse which accepts these "W" period strings only if the year can be interpreted as a time. I've explained this in the comment above and also in #48000 - there's a section on weeks.

Sure, this appears to be an issue, but I don't understand why these "W" changes need to made in the same PR as adding the warning for Periods with timezones since they seem independent. Having them as separate PRs makes tracking issues easier in the long run.

This only applies to "W" because it has to do with what the string representation of a week period is and how it's parsed. Some part of that week string is interpreted as a timezone. If I remember correctly, it's the hyphen and the day (-29 in the example). dateutil.parser.parse reads that as a timezone offset of negative 29 hours. Converting back to a Period will throw the warning introduced in this PR.

Ah okay thanks. That explanation makes sense.

But it would be good to still keep testing "W" even if it works by chance. (but maybe include a comment why it works by chance)

And ignore the warning in the test or what? The CI requires that the test suite doesn't throw warnings, no? It sounds like implementing that would make the test a little complex. I'd have to special-case the "W", so the warning is only ignored there and not for the other frequencies.

In general, what's the point of testing for undocumented & broken functionality like this? Do we have reason to believe that library users are relying on parsing these kind of strings? To me the conversion from Period to String and back seems like a hack to write those tests quickly. Is this something we need to support going forward?

You can use pytest.param("W", marks=pytest.mark.filterwarnings(...)) in the parameterize.

Generally, we have seen all sorts of undocumented use cases and have gotten issues when those behaviors "broke". I imagine this should be supported eventually.

jreback

we
don't need a special warning here
just use the standard UserWarning

joooeey · 2022-08-06T14:19:18Z

we don't need a special warning here just use the standard UserWarning

Done.

I was just thinking it's not good UX if we raise UserWarning for things that are not necessarily mistakes by the user which I've seen a few examples. But then there's not really a better Warning in the built-in Exceptions except perhaps Warning.

…in matplotlib.

pandas/_libs/tslibs/period.pyx

joooeey · 2022-08-23T14:48:55Z

@jreback can this be merged? I implemented your requested change the day I opened the merge request.

jreback · 2022-08-23T15:05:43Z

doc/source/whatsnew/v1.4.4.rst

@@ -43,6 +43,7 @@ Bug fixes
 - Bug in :meth:`DataFrameGroupBy.value_counts` where ``subset`` had no effect (:issue:`44267`)
 - Bug in :meth:`loc.__getitem__` with a list of keys causing an internal inconsistency that could lead to a disconnect between ``frame.at[x, y]`` vs ``frame[y].loc[x]`` (:issue:`22372`)
 - Bug in the :meth:`Series.dt.strftime` accessor return a float instead of object dtype Series for all-NaT input, which also causes a spurious deprecation warning (:issue:`45858`)
+- :class:`Period` now raises a warning when created with data that contains timezone information. This is necessary because :class:`Period`, :class:`PeriodArray` and :class:`PeriodIndex` do not support timezones and hence drop any timezone information used when creating them. (:issue:`47005`)


move to 1.5 this is not a change we would backport

move to 1.5 this is not a change we would backport

I don't understand how the versioning works. Should this go into 1.5.3 or 1.6.0? I put it in v1.5.3.rst for now.

mroeschke

Would prefer the 'W' changes to be done in a separate PR to keep this one tightly scoped

#47990 (comment)

github-actions · 2022-09-29T00:12:18Z

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

joooeey · 2022-09-29T07:45:39Z

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

I'm still interested in working on this but I'm swamped with other things right now. I'll get to it later this year.

MarcoGorelli · 2022-12-08T09:44:09Z

Hi @joooeey - is this still active? Shall we close for now and can revisit when you have time?

joooeey · 2022-12-09T07:44:52Z

Hi @MarcoGorelli! I might have time to do all the changes from my side this weekend or on Tuesday. If I don't get to it, I'll close the PR next week (for the time being).

I couldn't pin down why the tests think, the stacklevel is wrong. In the stock python interpreter, the warning message refers to the correct line.

mroeschke · 2023-01-16T19:39:52Z

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.

jbrockmendel · 2023-03-04T22:12:33Z

I think the "W" parts of this have recently been fixed separately, would be good to see this revived. @joooeey up for it?

joooeey · 2023-03-06T08:14:31Z

In principle, I'm up for it. I've kept this on my backlog all this time. But I can't give a timeline.

joooeey added 3 commits August 5, 2022 20:19

added tests

49ea3fb

insert warning

d949d60

test for Period itself added.

cd421e4

joooeey commented Aug 6, 2022

View reviewed changes

jreback requested changes Aug 6, 2022

View reviewed changes

Replace DateTimeWarning by UserWarning

5d99f75

Removed overlooked DateTimeWarning from testing.rst.

8de04d1

joooeey marked this pull request as draft August 6, 2022 15:07

joooeey added 3 commits August 6, 2022 21:44

Added warning for Period creation with timestamp. Suppressed warning …

a067f84

…in matplotlib.

Suppress warning when indexing dataframe with string datetimes.

57d8883

Merge branch 'main' into tz-warning

fe9222e

joooeey marked this pull request as ready for review August 6, 2022 22:53

joooeey requested a review from jreback August 7, 2022 08:08

joooeey mentioned this pull request Aug 7, 2022

ENH: improve Period parsing #48000

Open

3 tasks

mroeschke reviewed Aug 8, 2022

View reviewed changes

pandas/_libs/tslibs/period.pyx Outdated Show resolved Hide resolved

mroeschke added Period Period data type Warnings Warnings that appear or should be added to pandas labels Aug 8, 2022

joooeey added 2 commits August 9, 2022 18:53

warn with f-string instead of %

b9e119e

Merge branch 'main' into tz-warning

466ae26

jreback requested changes Aug 23, 2022

View reviewed changes

mroeschke requested changes Aug 23, 2022

View reviewed changes

github-actions bot added the Stale label Sep 29, 2022

joooeey added 2 commits December 13, 2022 21:07

Merge branch 'main' into tz-warning

372e9e4

move changelog to correct .rst (part 1)

edf7de3

joooeey added 4 commits December 13, 2022 21:24

move whatsnew bullet to v1.5.3.rst

4d8ee9b

re-introduce test for week period

ca24736

Fix broken merge (missing import)

d398265

black

84621a0

MarcoGorelli self-requested a review December 13, 2022 21:16

joooeey added 5 commits December 13, 2022 23:11

increment warning stacklevel

55fbbab

revert stacklevel change

441d17c

remove stacklevel check

abf431e

I couldn't pin down why the tests think, the stacklevel is wrong. In the stock python interpreter, the warning message refers to the correct line.

re-include stacklevel check

cecc738

Add comment with reference to GitHub issue.

b5a2b8e

joooeey marked this pull request as draft December 18, 2022 20:45

mroeschke closed this Jan 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when creating `Period` with a string that includes timezone information #47990

Warn when creating `Period` with a string that includes timezone information #47990

joooeey commented Aug 6, 2022 •

edited

Loading

joooeey Aug 6, 2022

mroeschke Aug 8, 2022

joooeey Aug 24, 2022

mroeschke Aug 24, 2022

joooeey Aug 25, 2022

mroeschke Aug 25, 2022

joooeey Aug 26, 2022

mroeschke Aug 26, 2022

joooeey Aug 27, 2022 •

edited

Loading

mroeschke Aug 29, 2022

jreback left a comment

joooeey commented Aug 6, 2022

joooeey commented Aug 23, 2022

jreback Aug 23, 2022

joooeey Dec 13, 2022

mroeschke left a comment

github-actions bot commented Sep 29, 2022

joooeey commented Sep 29, 2022

MarcoGorelli commented Dec 8, 2022

joooeey commented Dec 9, 2022

mroeschke commented Jan 16, 2023

jbrockmendel commented Mar 4, 2023

joooeey commented Mar 6, 2023

		@pytest.mark.parametrize("freq", ["D", "W", "M", "Q", "A"])
		@pytest.mark.parametrize("freq", ["D", "M", "Q", "A"])

Warn when creating Period with a string that includes timezone information #47990

Warn when creating Period with a string that includes timezone information #47990

Conversation

joooeey commented Aug 6, 2022 • edited Loading

Note on the new DateTimeWarning

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joooeey Aug 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

joooeey commented Aug 6, 2022

joooeey commented Aug 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroeschke left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 29, 2022

joooeey commented Sep 29, 2022

MarcoGorelli commented Dec 8, 2022

joooeey commented Dec 9, 2022

mroeschke commented Jan 16, 2023

jbrockmendel commented Mar 4, 2023

joooeey commented Mar 6, 2023

Warn when creating `Period` with a string that includes timezone information #47990

Warn when creating `Period` with a string that includes timezone information #47990

joooeey commented Aug 6, 2022 •

edited

Loading

Note on the new `DateTimeWarning`

joooeey Aug 27, 2022 •

edited

Loading