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.

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

API/DEPR: 'periods' argument instead of 'n' for PeriodIndex.shift() #22912

Merged
merged 5 commits into from Oct 8, 2018

Conversation

Projects
None yet
4 participants
@arminv
Copy link
Contributor

commented Sep 30, 2018

  • closes #22458
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

In order to be consistent with Index.shift & Series.shift & DatetimeIndex.shift, n argument was deprecated in favor of periods.

In [2]: idx = pd.PeriodIndex(freq='A', start='1/1/2001', end='12/1/2009')

In [3]: idx
Out[3]: 
PeriodIndex(['2001', '2002', '2003', '2004', '2005', '2006', '2007', '2008',
             '2009'],
            dtype='period[A-DEC]', freq='A-DEC')

In [4]: idx.shift(1)
Out[4]: 
PeriodIndex(['2002', '2003', '2004', '2005', '2006', '2007', '2008', '2009',
             '2010'],
            dtype='period[A-DEC]', freq='A-DEC')

In [5]: idx.shift(periods=1)
Out[5]: 
PeriodIndex(['2002', '2003', '2004', '2005', '2006', '2007', '2008', '2009',
             '2010'],
            dtype='period[A-DEC]', freq='A-DEC')

In [6]: idx.shift(n=1)
//anaconda/envs/pandas_dev/bin/ipython:1: FutureWarning: the 'n' keyword is deprecated, use 'periods' instead
  #!//anaconda/envs/pandas_dev/bin/python
Out[6]: 
PeriodIndex(['2002', '2003', '2004', '2005', '2006', '2007', '2008', '2009',
             '2010'],
            dtype='period[A-DEC]', freq='A-DEC')
@pep8speaks

This comment has been minimized.

Copy link

commented Sep 30, 2018

Hello @arminv! Thanks for updating the PR.

Comment last updated on October 08, 2018 at 18:08 Hours UTC
@codecov

This comment has been minimized.

Copy link

commented Sep 30, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@e510b1a). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #22912   +/-   ##
=========================================
  Coverage          ?   92.19%           
=========================================
  Files             ?      169           
  Lines             ?    50879           
  Branches          ?        0           
=========================================
  Hits              ?    46910           
  Misses            ?     3969           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.61% <100%> (?)
#single 42.32% <66.66%> (?)
Impacted Files Coverage Δ
pandas/core/arrays/datetimelike.py 95.56% <ø> (ø)
pandas/core/generic.py 96.65% <ø> (ø)
pandas/core/arrays/period.py 92.04% <100%> (ø)

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 e510b1a...d5586e4. Read the comment docs.

@arminv

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2018

@TomAugspurger can you review please?

@TomAugspurger TomAugspurger added this to the 0.24.0 milestone Oct 1, 2018

@TomAugspurger
Copy link
Contributor

left a comment

LGTM.

I didn't closely follow the issue and other discussion, but I just want to double-check that we really want to do this. DatetimeIndex / PeriodIndex.shift are fundamentally different from Series.shift, (shifting values vs. position). Still, I think changing the argument name is still for the best.

cc @jorisvandenbossche

See Also
--------
Index.shift : Shift values of Index.

This comment has been minimized.

Copy link
@TomAugspurger

TomAugspurger Oct 1, 2018

Contributor

I think remove this. Index.shift isn't implemented, it's just for the date-like subclasses.

This comment has been minimized.

Copy link
@jreback

jreback Oct 7, 2018

Contributor

can you update this @arminv

@jreback

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

this is ok, we have an inconsistency in named args now.

@jreback
Copy link
Contributor

left a comment

ping on green.

@@ -573,7 +573,7 @@ Deprecations
- :meth:`Series.str.cat` has deprecated using arbitrary list-likes *within* list-likes. A list-like container may still contain
many ``Series``, ``Index`` or 1-dimensional ``np.ndarray``, or alternatively, only scalar values. (:issue:`21950`)
- :meth:`FrozenNDArray.searchsorted` has deprecated the ``v`` parameter in favor of ``value`` (:issue:`14645`)
- :func:`DatetimeIndex.shift` now accepts ``periods`` argument instead of ``n`` for consistency with :func:`Index.shift` and :func:`Series.shift`. Using ``n`` throws a deprecation warning (:issue:`22458`)
- :func:`DatetimeIndex.shift` and :func:`PeriodIndex.shift` now accept ``periods`` argument instead of ``n`` for consistency with :func:`Index.shift` and :func:`Series.shift`. Using ``n`` throws a deprecation warning (:issue:`22458`)

This comment has been minimized.

Copy link
@jreback

jreback Oct 7, 2018

Contributor

can you add this issue as well here (this PR number is fine)

@jreback

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2018

pls rebase on master as well

arminv added some commits Oct 8, 2018

@TomAugspurger
Copy link
Contributor

left a comment

Sorry about the merge conflict. This looks good.

@arminv

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

@jreback all green.
@TomAugspurger thanks for reviewing.

@TomAugspurger TomAugspurger merged commit f5f6eca into pandas-dev:master Oct 8, 2018

6 checks passed

ci/circleci: py27_compat Your tests passed on CircleCI!
Details
ci/circleci: py35_ascii Your tests passed on CircleCI!
Details
ci/circleci: py36_locale Your tests passed on CircleCI!
Details
ci/circleci: py36_locale_slow Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20181008.47 succeeded
Details
@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

Thanks!

@arminv arminv deleted the arminv:periods_kwarg branch Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.