Skip to content
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

Jump to bottom

DEPR: Remove legacy offsets #13868

Closed
wants to merge 1 commit into from
Closed

DEPR: Remove legacy offsets #13868

wants to merge 1 commit into from

Conversation

agraboso
Copy link
Contributor

@agraboso agraboso commented Aug 1, 2016 •
edited
Loading

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

@jreback
Copy link
Contributor

jreback commented Aug 1, 2016

@sinhrks

hmm, were these previously deprecated?

@agraboso
Copy link
Contributor Author

agraboso commented Aug 1, 2016 •
edited
Loading

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

@jreback jreback added Period Period data type Compat pandas objects compatability with Numpy or Python functions labels Aug 1, 2016
@jreback jreback added this to the 0.19.0 milestone Aug 1, 2016
@jreback
Copy link
Contributor

jreback commented Aug 1, 2016

@agraboso ok, guess weren't tested!

@sinhrks ?

@agraboso
Copy link
Contributor Author

agraboso commented Aug 1, 2016 •
edited
Loading

The are a few tests — see, for example,

But I don't know if there may be some edge cases in which the removed aliases could be accepted.

@codecov-io
Copy link

codecov-io commented Aug 1, 2016 •
edited
Loading

Current coverage is 85.22% (diff: 100%)

Merging #13868 into master will decrease coverage by 0.01%

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

@agraboso agraboso mentioned this pull request Aug 1, 2016
Closed
4 tasks
sinhrks
sinhrks reviewed Aug 1, 2016
View reviewed changes
pandas/src/period.pyx
@@ -683,7 +683,6 @@ cdef class _Period(object):

if isinstance(freq, compat.string_types):
freq = freq.upper()
Copy link
Member

Choose a reason for hiding this comment

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

can we remove .upper also, because it is handled in get_offset (called from to_offset)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@sinhrks
Copy link
Member

sinhrks commented Aug 1, 2016

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')

@sinhrks sinhrks added the Frequency DateOffsets label Aug 1, 2016
@agraboso
DEPR: Remove legacy offsets
25d932d 
Follow-up to GH13590. Remove legacy offset aliases that remain in
`pandas/tseries/frequencies.py`: `_period_alias_dictionary()` and
`_period_alias_dict`
@agraboso
Copy link
Contributor Author

agraboso commented Aug 2, 2016 •
edited
Loading

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

@sinhrks
Copy link
Member

sinhrks commented Aug 2, 2016

@agraboso You're right, confirmed using 0.18.1 to show warning.

@jreback jreback closed this in 1f55e91 Aug 2, 2016
@jreback
Copy link
Contributor

jreback commented Aug 2, 2016

thanks!

@agraboso agraboso deleted the follow-13590 branch August 2, 2016 13:11
jreback pushed a commit that referenced this pull request Aug 8, 2016
@agraboso @jreback
BUG: Fix Period and PeriodIndex support of combined offsets aliases
81819b7 
 - [x] closes #13730   - [x] tests added / passed   - [x] passes ``git
diff upstream/master | flake8 --diff``   - [x] whatsnew entry
Essentially, makes sure any `freq` string passed to a `Period` or
`PeriodIndex` goes through [`_Period._maybe_convert_freq()`](https://g
ithub.com/pydata/pandas/blob/master/pandas/src/period.pyx#L682-L697),
which calls [`to_offset()`](https://github.com/pydata/pandas/blob/mast
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Compat pandas objects compatability with Numpy or Python functions Frequency DateOffsets Period Period data type
Projects
None yet
Milestone
0.19.0
Development

Successfully merging this pull request may close these issues.
4 participants
@agraboso @jreback @codecov-io @sinhrks