Skip to content

BUG - remove scaling multiplier from Period diff result #23915

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.

Sign up for GitHub

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 23 commits into from
Dec 9, 2018
Merged

BUG - remove scaling multiplier from Period diff result #23915

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
377f6d1
BUG/TST - do not multiply period diff result by freq scaling factor
sds9995 Nov 26, 2018
b9fe48a
TST - reference issue number in test
sds9995 Nov 26, 2018
c262453
CLN - pep8 adherence
sds9995 Nov 26, 2018
364d4a9
DOC - reference issue in whatsnew
sds9995 Nov 27, 2018
713484c
Revert "DOC - reference issue in whatsnew"
sds9995 Nov 27, 2018
d0a5afe
TST - parameterize test
sds9995 Nov 27, 2018
7ebe407
DOC - reference issue in whatsnew
sds9995 Nov 27, 2018
e8b4c4a
BUG - account for freq kwds
sds9995 Nov 27, 2018
9ad13d8
TST - move non standard freq period diff test and test various keywords
sds9995 Nov 28, 2018
a38ba5a
BUG/CLN - fix periodindex/array diff, and provide base method for off…
sds9995 Dec 2, 2018
cd7bb21
TST/DOC - split tick and offset tests and fix whatsnew
sds9995 Dec 2, 2018
9a369a6
TST - add periodindex diff fix tests
sds9995 Dec 2, 2018
c8f36b9
DOC - add docstrings and GH references
sds9995 Dec 2, 2018
3206144
DOC - additional explanation of code
sds9995 Dec 2, 2018
4b83c3a
Merge branch 'master' into bug/period_diff
sds9995 Dec 4, 2018
1db7bb0
Merge branch 'master' into bug/period_diff
sds9995 Dec 6, 2018
c9a83d6
CLN - reorganize test
sds9995 Dec 6, 2018
7c4e3e6
TST - update tests to account for existing bug
sds9995 Dec 6, 2018
c128a1f
TST - move tick fixture to pandas/conftest
sds9995 Dec 7, 2018
e6d35e6
CLN - fix linting error
sds9995 Dec 7, 2018
ae189ea
DOC - add more detail about this fixs behavior in the whatsnew
sds9995 Dec 7, 2018
8aaf19e
DOC - fix issue reference
sds9995 Dec 7, 2018
9ed3629
DOC - fix spacing
sds9995 Dec 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pandas/_libs/tslibs/period.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ms7463 ms7463 Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@ms7463 ms7463 Nov 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

fixed

elif getattr(other, '_typ', None) == 'periodindex':
# GH#21314 PeriodIndex - Period returns an object-index
# of DateOffset objects, for which we cannot use __neg__
Expand Down
23 changes: 21 additions & 2 deletions pandas/tests/arithmetic/test_period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,8 +608,9 @@ def test_pi_add_offset_n_gt1_not_divisible(self, box_with_array):
result = pi + to_offset('3M')
tm.assert_equal(result, expected)

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

@gfyoung gfyoung Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why are we commenting this out?

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

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

Copy link
Member

@gfyoung gfyoung Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


# ---------------------------------------------------------------
# __add__/__sub__ with integer arrays
Expand Down Expand Up @@ -1085,3 +1086,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):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved


@pytest.mark.parametrize('freq,expected', [
(pd.offsets.Second, 18489600),
(pd.offsets.Minute, 308160),
(pd.offsets.Hour, 5136),
(pd.offsets.Day, 214),
(pd.offsets.MonthEnd, 7),
(pd.offsets.YearEnd, 1),
])
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

def test_period_diff(self, freq, expected):
# GH 23878
for i in range(1, 4):
Copy link
Member

Choose a reason for hiding this comment

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

Parameterize on i as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

p1 = pd.Period('19910905', freq=freq(i))
p2 = pd.Period('19920406', freq=freq(i))
assert (p2 - p1) == freq(expected)