fix #39556 (infer_freq not working with freq="H" and DST #39644

sdementen · 2021-02-07T15:18:22Z

check that the delta are unique before checking if they are day multiples
add test with freq="H" that raises the bug
closes BUG: 'infer_freq' does not work with tz != "UTC" #39556
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry => not sure where to enter this ...

- check that the delta are unique before checking if the are day multiples - add test with freq="H" that raises the bug

…taking for delta the minimum of deltas and checking delta is not null

arw2019

Thanks @sdementen for the PR!

we'll need a whatsnew (I expect targeting 1.3)

pandas/tseries/frequencies.py

jbrockmendel · 2021-02-09T00:37:00Z

pandas/tseries/frequencies.py

@@ -239,17 +239,18 @@ def get_freq(self) -> Optional[str]:
        if not self.is_monotonic or not self.index._is_unique:
            return None

-        delta = self.deltas[0]
-        if _is_multiple(delta, _ONE_DAY):
+        delta = min(self.deltas)


i think the idea behind using deltas[0] was that deltas should be unique at this point. is that not the case?

It is not unique when the index has a business day frequency as you have deltas of 1 day or 3 days (for the weekend).
A first version of the bugfix I tried was to check first the unicity and then take deltas[0] to fix the issue with freq=H and DST but it broke the test for business days

after reading through the doc of unique_deltas I see the self.deltas area already sorted => no need to take the min.
the key is to check that delta !=0 as with freq="H" and tz with DST, the minimum delta is 0 (in local time)

sdementen · 2021-02-09T05:23:57Z

I have added the whatsnew entry

sdementen · 2021-02-09T09:12:21Z

There is some check that fails after having added the what's new entry. Any clue why?

lithomas1 · 2021-02-10T16:40:47Z

@sdementen The CI failures are due to #39688. They should be gone if you merge master.

jbrockmendel · 2021-02-10T17:20:17Z

cc @mroeschke i think this is an hour-based analogue of the problem of freq=Day vs freq=DayDST.

i.e. this fixes one problem but will introduce others. im hesitant to do that, but the long-term fix has been stuck in limbo for a while

sdementen · 2021-02-10T19:12:13Z

@sdementen The CI failures are due to #39688. They should be gone if you merge master.

Do I need to do this myself? Or will this automatically be solved once someone merge my PR into master?

arw2019 · 2021-02-10T19:16:19Z

@sdementen The CI failures are due to #39688. They should be gone if you merge master.

Do I need to do this myself? Or will this automatically be solved once someone merge my PR into master?

you wanna merge master yourself yeah

mroeschke · 2021-02-10T19:28:14Z

I haven't used infer_freq much, but if infer_freq = "infer the wall time frequency" then yes this the same issue as freq=Day vs freq=DayDST.

sdementen · 2021-02-11T17:17:25Z

merged with master, all tests pass, ready to merge ;-)

sdementen · 2021-02-22T12:12:45Z

@arw2019 @mroeschke , do I need to do something more re this PR for it to be merged ?

jreback

can u also add an explicit test that is similar to the OP

sdementen · 2021-02-22T13:37:32Z

can u also add an explicit test that is similar to the OP

The test test_infer_freq_tz_transition is similar to my OP in the issue. The bug was not revealing itself because the frequency "H" was not used (only "3H") and it is this frequency "H" that I uses in my OP.

jreback · 2021-02-22T13:40:03Z

pandas/tests/tseries/frequencies/test_inference.py

@@ -267,7 +267,7 @@ def test_infer_freq_tz(tz_naive_fixture, expected, dates):
    ],
 )
 @pytest.mark.parametrize(


this is not exactly the same as the OP (though it may have revelead the issue). this is a naive fixture (IOW the OP worked for naive & UTC) ,but NOT for other tzs.

so can also make this more comprehensive

Not sure to get your comment yet ...

When I run the test test_infer_freq_tz_transition, it runs for a lot of tzs (None, UTC, US/Eastern, Asia/Tokyo, ...), for date_pairs that cover the DST changes (Fall, Spring and no change) and for freq = "H" (a.o. as it also tests for other infra-day frequencies). The test also refers to #8772 which is the issue I rephrased with a simple example in #39556.

My OP was only one case (tz=None, UTC, CET and freq=H) amongst these cases.
What was misleading in the original test is that the base frequency "H" that triggers the issue was not covered (probably because the author thought that testing with "3H" would cover "H" + other cases.

I can add a new test but I do not see what would differ from the current one (just my date_pairs would cover a full year which is not really needed for the test and my tz would be "CET" that is not covered yet other tz with DST are covered).

The small change in the test (adding freq="H" to the frequencies to test) breaks pandas before the bugfix.

I could adapt the current comment from " # see gh-8772" to " # see gh-8772 and gh-39556" to make it clearer ?

sorry you are right, was misreading the fixture.

jreback · 2021-02-22T23:26:46Z

cc @mroeschke if any comments, merge when good

mroeschke · 2021-02-23T02:25:56Z

Thanks @sdementen

sebastiendementen added 3 commits February 7, 2021 16:14

fix pandas-dev#39556:

967f39e

- check that the delta are unique before checking if the are day multiples - add test with freq="H" that raises the bug

fix lint

551f382

when freq=="B", the deltas are not unique (1 or 3 days) => change by …

5a7bdc4

…taking for delta the minimum of deltas and checking delta is not null

arw2019 reviewed Feb 7, 2021

View reviewed changes

pandas/tseries/frequencies.py Show resolved Hide resolved

arw2019 added Datetime Datetime data dtype Timezones Timezone data dtype labels Feb 7, 2021

jbrockmendel reviewed Feb 9, 2021

View reviewed changes

add whatsnew entry

ce8e8d3

as self.deltas is already ordered, deltas[0] is the minimum delta

17e0fb8

Merge remote-tracking branch 'origin/master' into fix-infer-freq-H-DST

5102602

jreback requested changes Feb 22, 2021

View reviewed changes

jreback reviewed Feb 22, 2021

View reviewed changes

jreback added this to the 1.3 milestone Feb 22, 2021

jreback approved these changes Feb 22, 2021

View reviewed changes

mroeschke approved these changes Feb 23, 2021

View reviewed changes

mroeschke merged commit 3289f82 into pandas-dev:master Feb 23, 2021

lithomas1 mentioned this pull request Feb 23, 2021

BUG: Freq argument lost using pd.infer_freq #38243

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #39556 (infer_freq not working with freq="H" and DST #39644

fix #39556 (infer_freq not working with freq="H" and DST #39644

sdementen commented Feb 7, 2021 •

edited

Loading

arw2019 left a comment

jbrockmendel Feb 9, 2021

sdementen Feb 9, 2021 •

edited

Loading

sdementen Feb 9, 2021

sdementen commented Feb 9, 2021

sdementen commented Feb 9, 2021

lithomas1 commented Feb 10, 2021

jbrockmendel commented Feb 10, 2021

sdementen commented Feb 10, 2021

arw2019 commented Feb 10, 2021

mroeschke commented Feb 10, 2021

sdementen commented Feb 11, 2021

sdementen commented Feb 22, 2021

jreback left a comment

sdementen commented Feb 22, 2021

jreback Feb 22, 2021

sdementen Feb 22, 2021

jreback Feb 22, 2021

jreback commented Feb 22, 2021

mroeschke commented Feb 23, 2021

fix #39556 (infer_freq not working with freq="H" and DST #39644

fix #39556 (infer_freq not working with freq="H" and DST #39644

Conversation

sdementen commented Feb 7, 2021 • edited Loading

arw2019 left a comment

Choose a reason for hiding this comment

jbrockmendel Feb 9, 2021

Choose a reason for hiding this comment

sdementen Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

sdementen Feb 9, 2021

Choose a reason for hiding this comment

sdementen commented Feb 9, 2021

sdementen commented Feb 9, 2021

lithomas1 commented Feb 10, 2021

jbrockmendel commented Feb 10, 2021

sdementen commented Feb 10, 2021

arw2019 commented Feb 10, 2021

mroeschke commented Feb 10, 2021

sdementen commented Feb 11, 2021

sdementen commented Feb 22, 2021

jreback left a comment

Choose a reason for hiding this comment

sdementen commented Feb 22, 2021

jreback Feb 22, 2021

Choose a reason for hiding this comment

sdementen Feb 22, 2021

Choose a reason for hiding this comment

jreback Feb 22, 2021

Choose a reason for hiding this comment

jreback commented Feb 22, 2021

mroeschke commented Feb 23, 2021

sdementen commented Feb 7, 2021 •

edited

Loading

sdementen Feb 9, 2021 •

edited

Loading