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.

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
Show file tree
Hide file tree
Changes from 9 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 doc/source/whatsnew/v0.24.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1056,7 +1056,7 @@ from :class:`Period`, :class:`PeriodIndex`, and in some cases
:class:`Timestamp`, :class:`DatetimeIndex` and :class:`TimedeltaIndex`.

This usage is now deprecated. Instead add or subtract integer multiples of
the object's ``freq`` attribute (:issue:`21939`)
the object's ``freq`` attribute (:issue:`21939`) / (:issue:`23878`)

Previous Behavior:

Expand Down
4 changes: 3 additions & 1 deletion pandas/_libs/tslibs/period.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

pass n=1 explicitly here.

add a reference # GH#23915 for future readers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return (self.ordinal - other.ordinal) * base_freq
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
32 changes: 32 additions & 0 deletions pandas/tests/scalar/period/test_period.py
Original file line number Diff line number Diff line change
Expand Up @@ -1106,6 +1106,38 @@ def test_sub(self):
with pytest.raises(period.IncompatibleFrequency, match=msg):
per1 - Period('2011-02', freq='M')

@pytest.mark.parametrize('kwds', [
{},
{'normalize': True},
{'normalize': False},
{'normalize': True, 'month': 4},
{'normalize': False, 'month': 4},
])
@pytest.mark.parametrize('n', [1, 2, 3, 4])
@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),
])
def test_sub_non_standard_freq(self, freq, expected, n, kwds):
# GH 23878
# Only kwd allowed in period compatible freqs is 'month' in `YearEnd`
if 'month' in kwds:
if freq is pd.offsets.YearEnd:
expected = 0
else:
return
# Only non-Tick frequencies can have normalize set to True
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if pd.tseries.offsets.Tick in freq.__bases__ and kwds.get('normalize'):
return

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

def test_add_offset(self):
# freq is DateOffset
for freq in ['A', '2A', '3A']:
Expand Down