BUG - remove scaling multiplier from `Period` diff result #23915

ms7463 · 2018-11-26T01:35:57Z

closes BUG: non-standard frequency Period arithmetic #23878
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2018-11-26T01:35:59Z

Hello @ArtinSarraf! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/conftest.py !
There are no PEP8 issues in the file pandas/core/arrays/datetimelike.py !
There are no PEP8 issues in the file pandas/tests/arithmetic/test_period.py !
There are no PEP8 issues in the file pandas/tests/scalar/period/test_period.py !
There are no PEP8 issues in the file pandas/tests/tseries/offsets/conftest.py !

Comment last updated on December 07, 2018 at 04:37 Hours UTC

codecov · 2018-11-26T02:32:27Z

Codecov Report

Merging #23915 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23915      +/-   ##
==========================================
- Coverage    92.2%    92.2%   -0.01%     
==========================================
  Files         162      162              
  Lines       51701    51700       -1     
==========================================
- Hits        47672    47670       -2     
- Misses       4029     4030       +1

Flag	Coverage Δ
#multiple	`90.6% <100%> (-0.01%)`	⬇️
#single	`43.02% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/datetimelike.py	`96.35% <100%> (ø)`	⬆️
pandas/core/internals/blocks.py	`93.65% <0%> (-0.07%)`	⬇️
pandas/core/indexes/base.py	`96.27% <0%> (-0.01%)`	⬇️
pandas/core/frame.py	`96.91% <0%> (ø)`	⬆️
pandas/core/generic.py	`96.65% <0%> (ø)`	⬆️
pandas/core/arrays/interval.py	`92.98% <0%> (ø)`	⬆️
pandas/core/groupby/groupby.py	`96.5% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b841374...9ed3629. Read the comment docs.

gfyoung

@ArtinSarraf : Good start! I have a couple of comments regarding the tests.

Also, don't forget to add a whatsnew for this bug.

gfyoung · 2018-11-26T04:10:32Z

pandas/tests/arithmetic/test_period.py

+    ])
+    def test_period_diff(self, freq, expected):
+        # GH 23878
+        for i in range(1, 4):


Parameterize on i as well.

gfyoung · 2018-11-26T04:10:57Z

pandas/tests/arithmetic/test_period.py

-        tm.assert_equal(result, expected)
+        # This test is broken
+        # result = to_offset('3M') + pi
+        # tm.assert_equal(result, expected)


Why are we commenting this out?

Existing tests shouldn't be broken (or xfailed) unless we have a very good reason for this...

Ah yea, sorry meant to make a comment addressing this in the PR discussion. This was failing for me on a clean checkout of the latest master code, before I made my changes. I can provide more details tomorrow.

If you could revert the change here and let CI run on it, that would be great actually, so that we can also check if it's just a local failure or a more likely implementation issue.

put it back and tests passed, looks like it was some transient local issue.

ms7463 · 2018-11-26T18:33:42Z

@gfyoung - the reason I didn’t add a whatsnew entry is that this Offset result from diffing periods is already new behavior in 0.24 so the bug had not been released yet. Should I still add an entry for it?

gfyoung · 2018-11-26T20:50:54Z

the reason I didn’t add a whatsnew entry is that this Offset result from diffing periods is already new behavior in 0.24 so the bug had not been released yet. Should I still add an entry for it?

Ah, gotcha. In that case, just add a reference to your issue number to the existing whatsnew entry instead.

This reverts commit 364d4a9.

jbrockmendel · 2018-11-27T15:01:51Z

pandas/_libs/tslibs/period.pyx

@@ -1685,7 +1685,7 @@ cdef class _Period(object):
                if other.freq != self.freq:
                    msg = _DIFFERENT_FREQ.format(self.freqstr, other.freqstr)
                    raise IncompatibleFrequency(msg)
-                return (self.ordinal - other.ordinal) * self.freq
+                return (self.ordinal - other.ordinal) * type(self.freq)()


Won't this be wrong for offsets that have relevant keywords?

You’re right. So one option would be to do

... * type(self.freq)(normalize=self.freq.normalize, **self.freq.kwds)

This way no other classes need to be modified. However, I think it might be worth adding a property to the DateOffset objet to do the above suggestion. Something like DateOffset.base?

jbrockmendel · 2018-11-27T15:02:51Z

pandas/tests/arithmetic/test_period.py

@@ -1085,3 +1085,21 @@ def test_pi_sub_period_nat(self):
        exp = pd.TimedeltaIndex([np.nan, np.nan, np.nan, np.nan], name='idx')
        tm.assert_index_equal(idx - pd.Period('NaT', freq='M'), exp)
        tm.assert_index_equal(pd.Period('NaT', freq='M') - idx, exp)
+
+
+class TestPeriodArithmetic(object):


This all belongs in pandas.tests.scalar.period. If you want to make a new file test_arithmetic.py in that directory, that'd be OK. Otherwise put in test_period.py

jbrockmendel · 2018-11-27T15:04:50Z

pandas/tests/arithmetic/test_period.py

+        (pd.offsets.Day, 214),
+        (pd.offsets.MonthEnd, 7),
+        (pd.offsets.YearEnd, 1),
+    ])


definitely needs cases with kwargs passed to offset constructors. Putting it in tests.tseries.offsets might be useful since there are test classes that construct a bunch of these

parameterized the tests on some kwargs too, only YearEnd takes any kwds besides normalize ('month'). And only non-Tick offsets (in this case MonthEnd and YearEnd) can take normalize=True.

ms7463 · 2018-12-01T15:35:27Z

@gfyoung / @jbrockmendel - any other changes to consider?

doc/source/whatsnew/v0.24.0.rst

jbrockmendel · 2018-12-02T02:16:35Z

pandas/_libs/tslibs/period.pyx

@@ -1685,7 +1685,9 @@ cdef class _Period(object):
                if other.freq != self.freq:
                    msg = _DIFFERENT_FREQ.format(self.freqstr, other.freqstr)
                    raise IncompatibleFrequency(msg)
-                return (self.ordinal - other.ordinal) * self.freq
+                base_freq = type(self.freq)(normalize=self.freq.normalize,
+                                            **self.freq.kwds)


pass n=1 explicitly here.

add a reference # GH#23915 for future readers

jbrockmendel · 2018-12-02T02:20:54Z

pandas/tests/scalar/period/test_period.py

+                expected = 0
+            else:
+                return
+        # Only non-Tick frequencies can have normalize set to True


probably cleaner to test separately

also this gets a couple of non-tick frequencies, but using the structure in tests.tseries.offsets should make it feasible to be a lot more thorough

Separated.
Only 4 of the non-tick frequencies in pd.offsets are valid Period frequencies so I explicitly parameterized those in the tests. The Tick fixture worked well though.

jbrockmendel · 2018-12-02T02:23:33Z

It looks like the same bug exists in the analogous PeriodIndex op. Want to fix it there while you're at it?

…sets

jreback · 2018-12-02T18:11:53Z

@gfyoung if you have any comments.

jreback · 2018-12-02T18:12:25Z

@jbrockmendel if you'd have a look at the tests and see if they are sufficient

jreback · 2018-12-03T13:36:53Z

@ArtinSarraf can you merge master and see if you can resolve failures

ms7463 · 2018-12-04T03:30:28Z

@jreback / @jbrockmendel - is there any way to recreate the testing envs of the automated tests. My tests run fine locally and from the failed test output its not obvious what the error is (since the repr of the result and expected are the same), I'm assuming either the normalize kwd attributes are differing somehow.

ms7463 · 2018-12-06T02:05:55Z

@jreback / @jbrockmendel

Found the issue. Looks like this is due to an existing bug with PeriodIndex (I will open a separate issue for this). See the example below.

>>> pd.PeriodIndex(['19910905'], freq=pd.offsets.YearEnd(normalize=True)).freq.normalize
True
>>> pd.PeriodIndex(['19910905'], freq=pd.offsets.YearEnd(normalize=False)).freq.normalize
True

Restart the process

>>> pd.PeriodIndex(['19910905'], freq=pd.offsets.YearEnd(normalize=False)).freq.normalize
False
>>> pd.PeriodIndex(['19910905'], freq=pd.offsets.YearEnd(normalize=True)).freq.normalize
False

Looks like the normalize option gets cached for PeriodIndex somehow. This causes my tests to fail because I iterate through normalize = True | False.

I will remove the normalize parameterization from the tests for now.

jbrockmendel · 2018-12-06T02:21:21Z

Looks like the normalize option gets cached for PeriodIndex somehow

Good catch. Best guess is it is in pandas.tseries.frequencies

ms7463 · 2018-12-06T03:33:56Z

@jbrockmendel - looks like the only failing test in the pandas-dev.pandas tests are linting errors now due to importing the tick_classes fixture (since it's not in the discovery path for those tests).

from pandas.tests.tseries.offsets.conftest import tick_classes

Is there another way to discover this fixture? I could move the test, but I think it makes sense to keep it grouped where it is.

jreback · 2018-12-06T12:34:27Z

pandas/tests/arithmetic/test_period.py

@@ -16,6 +16,7 @@
 from pandas.core import ops
 from pandas import Period, PeriodIndex, period_range, Series
 from pandas.tseries.frequencies import to_offset


so we can't do this, instead move the tick_classes fixture to pandas/conftest.py. I think everything should still work

ms7463 · 2018-12-07T05:23:27Z

@jreback / @jbrockmendel - anything else to consider?

jreback · 2018-12-07T12:58:06Z

@ArtinSarraf lgtm. can you add a whatsnew note. ping on green.

@gfyoung good?

ms7463 · 2018-12-08T23:43:08Z

@jreback - tests are all clean.

jreback · 2018-12-09T14:02:21Z

thanks @ArtinSarraf

…#23915)

sds9995 added 2 commits November 25, 2018 20:26

BUG/TST - do not multiply period diff result by freq scaling factor

377f6d1

TST - reference issue number in test

b9fe48a

CLN - pep8 adherence

c262453

ms7463 mentioned this pull request Nov 26, 2018

BUG: non-standard frequency Period arithmetic #23878

Closed

gfyoung added Bug Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Period Period data type labels Nov 26, 2018

gfyoung requested changes Nov 26, 2018

View reviewed changes

pandas-dev deleted a comment from mroeschke Nov 26, 2018

sds9995 added 4 commits November 26, 2018 21:16

DOC - reference issue in whatsnew

364d4a9

Revert "DOC - reference issue in whatsnew"

713484c

This reverts commit 364d4a9.

TST - parameterize test

d0a5afe

DOC - reference issue in whatsnew

7ebe407

jbrockmendel reviewed Nov 27, 2018

View reviewed changes

sds9995 added 2 commits November 27, 2018 18:36

BUG - account for freq kwds

e8b4c4a

TST - move non standard freq period diff test and test various keywords

9ad13d8

gfyoung reviewed Dec 2, 2018

View reviewed changes

doc/source/whatsnew/v0.24.0.rst Outdated Show resolved Hide resolved

jbrockmendel reviewed Dec 2, 2018

View reviewed changes

sds9995 added 2 commits December 1, 2018 23:30

BUG/CLN - fix periodindex/array diff, and provide base method for off…

a38ba5a

…sets

TST/DOC - split tick and offset tests and fix whatsnew

cd7bb21

jreback added this to the 0.24.0 milestone Dec 2, 2018

DOC - additional explanation of code

3206144

jreback removed this from the 0.24.0 milestone Dec 3, 2018

Merge branch 'master' into bug/period_diff

4b83c3a

sds9995 added 2 commits December 5, 2018 19:17

Merge branch 'master' into bug/period_diff

1db7bb0

CLN - reorganize test

c9a83d6

TST - update tests to account for existing bug

7c4e3e6

jreback requested changes Dec 6, 2018

View reviewed changes

sds9995 added 2 commits December 6, 2018 21:38

TST - move tick fixture to pandas/conftest

c128a1f

CLN - fix linting error

e6d35e6

jreback added this to the 0.24.0 milestone Dec 7, 2018

sds9995 added 3 commits December 7, 2018 18:01

DOC - add more detail about this fixs behavior in the whatsnew

ae189ea

DOC - fix issue reference

8aaf19e

DOC - fix spacing

9ed3629

jreback approved these changes Dec 9, 2018

View reviewed changes

jreback merged commit 8dc22d8 into pandas-dev:master Dec 9, 2018

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG - remove scaling multiplier from Period diff result (pandas-dev…

b7b03f7

…#23915)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG - remove scaling multiplier from Period diff result (pandas-dev…

090b886

…#23915)

xiaohuanlin mentioned this pull request Jan 11, 2024

BUG: .to_json() of periode_range gives OverflowError: Maximum recursion level reached #55490

Open

3 tasks

BUG - remove scaling multiplier from Period diff result #23915

BUG - remove scaling multiplier from Period diff result #23915

Conversation

ms7463 commented Nov 26, 2018

pep8speaks commented Nov 26, 2018 • edited Loading

Comment last updated on December 07, 2018 at 04:37 Hours UTC

codecov bot commented Nov 26, 2018 • edited Loading

Codecov Report

gfyoung left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms7463 commented Nov 26, 2018

gfyoung commented Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

ms7463 Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

ms7463 Nov 28, 2018 • edited Loading

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

ms7463 commented Dec 1, 2018

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

jbrockmendel commented Dec 2, 2018

jreback commented Dec 2, 2018

jreback commented Dec 2, 2018

jreback commented Dec 3, 2018

ms7463 commented Dec 4, 2018

ms7463 commented Dec 6, 2018 • edited Loading

jbrockmendel commented Dec 6, 2018

ms7463 commented Dec 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms7463 commented Dec 7, 2018

jreback commented Dec 7, 2018

ms7463 commented Dec 8, 2018

jreback commented Dec 9, 2018

BUG - remove scaling multiplier from `Period` diff result #23915

BUG - remove scaling multiplier from `Period` diff result #23915

pep8speaks commented Nov 26, 2018 •

edited

Loading

codecov bot commented Nov 26, 2018 •

edited

Loading

gfyoung left a comment •

edited

Loading

gfyoung Nov 26, 2018 •

edited

Loading

gfyoung Nov 26, 2018 •

edited

Loading

gfyoung commented Nov 26, 2018 •

edited

Loading

ms7463 Nov 27, 2018 •

edited

Loading

ms7463 Nov 28, 2018 •

edited

Loading

ms7463 commented Dec 6, 2018 •

edited

Loading