DEPR: Remove legacy offsets by agraboso · Pull Request #13868 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- tests added / passed
- passes
git diff upstream/master | flake8 --diff - whatsnew entry
Follow-up to #13590. Remove legacy offset aliases that remained in pandas/tseries/frequencies.py: _period_alias_dictionary() and _period_alias_dict.
hmm, were these previously deprecated?
@jreback All the removed aliases raise a FutureWarning in 0.18.1 and a ValueError in master — at least when creating Period and PeriodIndex objects, as well as calling .asfreq() on them.
Period data type
pandas objects compatability with Numpy or Python functions
labels
Current coverage is 85.22% (diff: 100%)
@@ master #13868 diff @@
Files 140 140
Lines 50456 50398 -58
Methods 0 0
Messages 0 0
Branches 0 0
- Hits 43007 42950 -57
- Misses 7449 7448 -1
Partials 0 0
Powered by Codecov. Last update d4f95fd...25d932d
| @@ -683,7 +683,6 @@ cdef class _Period(object): |
|---|
| if isinstance(freq, compat.string_types): |
| freq = freq.upper() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Yes it has been missed as not tested. Thanks for the catch!
NOTE: I found a condition which doesn't show warning. I think it is rare and can't be a reason to keep the impl, but may need discussion.
pd.Period(ordinal=1, freq='SECONDLY')
# Period('1970-01-01 00:00:01', 'S')
Follow-up to GH13590. Remove legacy offset aliases that remain in
pandas/tseries/frequencies.py: _period_alias_dictionary() and
_period_alias_dict
@sinhrks pd.Period(ordinal=1, freq='SECONDLY') raises a FutureWarning in 0.18.1. I added a test of pd.Period(ordinal=1, freq='SECONDLY') (and other removed aliases) that fails on master but passes with this PR.
@agraboso You're right, confirmed using 0.18.1 to show warning.
jreback pushed a commit that referenced this pull request
- closes #13730 - [x] tests added / passed - [x] passes
git diff upstream/master | flake8 --diff- [x] whatsnew entry Essentially, makes sure anyfreqstring passed to aPeriodorPeriodIndexgoes through [_Period._maybe_convert_freq()](https://g ithub.com/pydata/pandas/blob/master/pandas/src/period.pyx#L682-L697), which callsto_offset()er/pandas/tseries/frequencies.py#L389-L451), which is where the logic for combining aliases is. All the examples in #13730 result in the correct output, and all existing tests pass. I have not written any new ones yet — I first wanted to get the opinion of the maintainers. This PR builds on #13868 (without it, some existing tests fail).
Author: agraboso agraboso@gmail.com
Closes #13874 from agraboso/fix-13730 and squashes the following commits:
49a3783 [agraboso] BUG: Fix Period and PeriodIndex support of combined alias offsets
Labels
pandas objects compatability with Numpy or Python functions
DateOffsets
Period data type