Series pct_change fill_method behavior #25291

albertvillanova · 2019-02-12T22:55:12Z

closes Series pct_change fill_method behavior #25006
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2019-02-12T22:55:19Z

Hello @albertvillanova! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-30 16:38:20 UTC

codecov · 2019-02-13T06:31:34Z

Codecov Report

Merging #25291 into master will decrease coverage by 50%.
The diff coverage is 16.66%.

@@             Coverage Diff             @@
##           master   #25291       +/-   ##
===========================================
- Coverage   91.72%   41.71%   -50.01%     
===========================================
  Files         173      173               
  Lines       52831    52841       +10     
===========================================
- Hits        48457    22045    -26412     
- Misses       4374    30796    +26422

Flag	Coverage Δ
#multiple	`?`
#single	`41.71% <16.66%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/generic.py	`38.24% <16.66%> (-55.93%)`	⬇️
pandas/io/formats/latex.py	`0% <0%> (-100%)`	⬇️
pandas/core/categorical.py	`0% <0%> (-100%)`	⬇️
pandas/io/sas/sas_constants.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/plotting.py	`0% <0%> (-100%)`	⬇️
pandas/tseries/converter.py	`0% <0%> (-100%)`	⬇️
pandas/io/formats/html.py	`0% <0%> (-99.35%)`	⬇️
pandas/core/groupby/categorical.py	`0% <0%> (-95.46%)`	⬇️
pandas/io/sas/sas7bdat.py	`0% <0%> (-91.17%)`	⬇️
pandas/io/sas/sas_xport.py	`0% <0%> (-90.15%)`	⬇️
... and 130 more

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 4d44a2a...4418bf1. Read the comment docs.

codecov · 2019-02-13T06:31:34Z

Codecov Report

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

@@            Coverage Diff            @@
##             master   #25291   +/-   ##
=========================================
  Coverage          ?   91.68%           
=========================================
  Files             ?      174           
  Lines             ?    50751           
  Branches          ?        0           
=========================================
  Hits              ?    46531           
  Misses            ?     4220           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.19% <100%> (?)`
#single	`41.15% <7.14%> (?)`

Impacted Files	Coverage Δ
pandas/core/generic.py	`93.38% <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 7b25463...fd18d04. Read the comment docs.

jreback

haven't actually looked at the changes, rather some stylistic concerns

pandas/tests/frame/test_analytics.py

pandas/core/generic.py

albertvillanova · 2019-02-21T05:33:33Z

@jreback all checks have passed.

albertvillanova · 2019-02-27T07:22:15Z

@WillAyd @jschendel could you please have a look?

jreback · 2019-02-27T13:02:59Z

pandas/core/generic.py

+            raise ValueError("cannot pass both skipna and limit")
+        if skipna is None and fill_method is None and limit is None:
+            skipna = True
+        if skipna and self._typ == 'dataframe':


use isinstance here

Hmm maybe add this to the frame subclass instead? Somewhat confusing to introspect here in the shared one.

For the moment, I have added isinstance as required by @jreback. Tell me if you both think I should do otherwise.

pandas/core/generic.py

pandas/tests/frame/test_analytics.py

pandas/tests/generic/test_generic.py

jreback · 2019-02-27T13:04:55Z

pandas/tests/generic/test_generic.py

+    ])
+    def test_pct_change_skipna_raises(self, fill_method, limit):
+        # GH25006
+        if self._typ is DataFrame or self._typ is Series:


this should not be needed any longer, Panels are gone

pandas/core/generic.py

WillAyd · 2019-02-27T16:27:00Z

pandas/core/generic.py

+            raise ValueError("cannot pass both skipna and limit")
+        if skipna is None and fill_method is None and limit is None:
+            skipna = True
+        if skipna and self._typ == 'dataframe':


Hmm maybe add this to the frame subclass instead? Somewhat confusing to introspect here in the shared one.

pandas/tests/series/test_analytics.py

pandas/tests/frame/test_timeseries.py

jreback

this needs a subsection in the whatsnew to show the previous and current behavior.

pandas/core/generic.py

albertvillanova · 2019-03-16T16:56:18Z

@TomAugspurger yes, the behavior you are interested in can be achieved by setting skipna=False:

In [11]: s = pd.Series([90, 91, None, 85, None, 95, 97], index=pd.date_range('2000', periods=7))                                               

In [12]: s                                                                                                                                     
Out[12]: 
2000-01-01    90.0
2000-01-02    91.0
2000-01-03     NaN
2000-01-04    85.0
2000-01-05     NaN
2000-01-06    95.0
2000-01-07    97.0
Freq: D, dtype: float64

In [13]: s.pct_change(skipna=False)                                                                                                            
Out[13]: 
2000-01-01         NaN
2000-01-02    0.011111
2000-01-03         NaN
2000-01-04         NaN
2000-01-05         NaN
2000-01-06         NaN
2000-01-07    0.021053
Freq: D, dtype: float64

I totally agree with you that the behavior with skipna=False is much more sensible and intuitive.

jreback

i think settingt he default skipna=True is good. I believe this simplifies the checking a bit as well.

jreback · 2019-03-20T00:00:55Z

doc/source/whatsnew/v0.25.0.rst

@@ -86,6 +86,7 @@ Other API Changes
 - :class:`DatetimeTZDtype` will now standardize pytz timezones to a common timezone instance (:issue:`24713`)
 - ``Timestamp`` and ``Timedelta`` scalars now implement the :meth:`to_numpy` method as aliases to :meth:`Timestamp.to_datetime64` and :meth:`Timedelta.to_timedelta64`, respectively. (:issue:`24653`)
 - :meth:`Timestamp.strptime` will now rise a ``NotImplementedError`` (:issue:`25016`)
+- Default `skipna=True` for :meth:`Series.pct_change` and :meth:`DataFrame.pct_change` will drop NAs before calculation (:issue:`25006`)


right, maybe also say adding the skipna arg (as its not obvious that it was added in the note)

WillAyd · 2019-05-03T05:33:52Z

@albertvillanova can you merge master and address latest comments?

jreback · 2019-05-12T21:11:01Z

can you merge master

jreback

@albertvillanova small comment; pls merge master and ping on green.

jreback · 2019-05-19T19:27:14Z

doc/source/whatsnew/v0.25.0.rst

@@ -86,6 +86,7 @@ Other API Changes
 - :class:`DatetimeTZDtype` will now standardize pytz timezones to a common timezone instance (:issue:`24713`)
 - ``Timestamp`` and ``Timedelta`` scalars now implement the :meth:`to_numpy` method as aliases to :meth:`Timestamp.to_datetime64` and :meth:`Timedelta.to_timedelta64`, respectively. (:issue:`24653`)
 - :meth:`Timestamp.strptime` will now rise a ``NotImplementedError`` (:issue:`25016`)
+- Default `skipna=True` for :meth:`Series.pct_change` and :meth:`DataFrame.pct_change` will drop NAs before calculation (:issue:`25006`)


can you say that thiis is current behavior and the default is NO change.

jreback · 2019-06-03T00:00:22Z

I think only a small comment left, can you merge master and respond.

jreback · 2019-06-27T03:40:01Z

can you merge master here

WillAyd · 2019-08-26T02:43:24Z

Rebased to keep current. Let's see if we can get this one in

TomAugspurger · 2019-08-26T19:53:23Z

Seems that CI is failing (haven't looked closely).

Is the release note accurate (just an enhancement, not an API change?)

Looks like we need a docstring example with skipna=False.

WillAyd · 2019-08-30T16:38:10Z

Updated docstring, though on closer look this does change the default behavior against master:

before change:

>>> s = pd.Series([90, 91, np.nan, 85, np.nan, 95])
>>> s.pct_change()
0         NaN
1    0.011111
2    0.000000
3   -0.065934
4    0.000000
5    0.117647
dtype: float64

this branch:

>>> s = pd.Series([90, 91, np.nan, 85, np.nan, 95])
>>> s.pct_change()
0         NaN
1    0.011111
2         NaN
3   -0.065934
4         NaN
5    0.117647
dtype: float64

So I think need to be careful here

TomAugspurger · 2019-08-30T17:02:10Z

Hmm thanks for checking. This seems like something we can do in a backwards-compatible way (possibly with a deprecation).

WillAyd · 2019-09-13T01:46:31Z

I've personally put this on the back burner for now - @albertvillanova are you interested in picking back up?

WillAyd · 2019-09-20T14:42:35Z

I think this requires some effort to manage the backwards compat piece but not something I personally have time / motivation to dedicate to. Looks stale otherwise so closing, but @albertvillanova if something you'd like to pick back up please ping and we can continue on!

Albert Villanova del Moral added 2 commits February 12, 2019 23:52

Add skipna to pct_change (pandas-dev#25006)

0e4e1c2

Add tests

4be1bdc

Albert Villanova del Moral added 3 commits February 13, 2019 00:02

Fix PEP8 issues

192bded

Fix PEP8 issue

bb74285

Fix test

4418bf1

Fix test

3670ffe

jreback changed the title ~~Fix #25006~~ Series pct_change fill_method behavior Feb 13, 2019

jreback requested changes Feb 13, 2019

View reviewed changes

pandas/tests/frame/test_analytics.py Outdated Show resolved Hide resolved

pandas/tests/frame/test_analytics.py Outdated Show resolved Hide resolved

Albert Villanova del Moral added 2 commits February 13, 2019 23:38

Fix tests

8f36c7a

Fix linting

add18de

gfyoung added API Design Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Feb 16, 2019

jreback requested changes Feb 16, 2019

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

Albert Villanova del Moral added 3 commits February 18, 2019 22:03

Merge branch 'master' into fix-25006

59eab18

Set default skipna=True

279f433

Use pytest.raises

4072ca0

jreback requested changes Feb 27, 2019

View reviewed changes

WillAyd requested changes Feb 27, 2019

View reviewed changes

Albert Villanova del Moral added 4 commits March 2, 2019 08:19

Merge branch 'master' into fix-25006

a016d8a

Address requested changes

80a09c9

Fix tests passing periods as kwarg

1bf00f8

Merge branch 'master' into fix-25006

9208f61

jreback requested changes Mar 3, 2019

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

pandas/core/generic.py Outdated Show resolved Hide resolved

pandas/core/generic.py Outdated Show resolved Hide resolved

pandas/core/generic.py Outdated Show resolved Hide resolved

Albert Villanova del Moral added 2 commits March 3, 2019 09:52

Merge branch 'master' into fix-25006

fd2cdf8

Add whatsnew note

66cc4a4

Albert Villanova del Moral added 2 commits March 16, 2019 17:10

Replace None with np.nan

ed86a7b

Replace DataFrame with ABCDataFrame

a1ca0ca

Merge branch 'master' into fix-25006

efefaf6

jreback requested changes Mar 20, 2019

View reviewed changes

Merge branch 'master' into fix-25006

1e854ed

jreback approved these changes May 19, 2019

View reviewed changes

jreback added this to the 0.25.0 milestone May 19, 2019

jreback requested changes May 19, 2019

View reviewed changes

jorisvandenbossche removed this from the 0.25.0 milestone Jun 30, 2019

WillAyd added 4 commits August 25, 2019 19:38

Merge remote-tracking branch 'upstream/master' into fix-25006

84c036a

blackify

1acee7c

Changed whatsnew

764846d

Updated versionadded

7184698

WillAyd added 4 commits August 26, 2019 16:07

Signature fixup

3821857

test failure fixup

a2be8f6

Merge remote-tracking branch 'upstream/master' into fix-25006

e456c6b

docstring for skipna=False

fd18d04

WillAyd mentioned this pull request Sep 16, 2019

DataFrame.pct_change should default fill_method=None instead of 'pad' #28461

Closed

WillAyd closed this Sep 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Series pct_change fill_method behavior #25291

Series pct_change fill_method behavior #25291

albertvillanova commented Feb 12, 2019 •

edited by WillAyd

Loading

pep8speaks commented Feb 12, 2019 •

edited

Loading

codecov bot commented Feb 13, 2019

codecov bot commented Feb 13, 2019 •

edited

Loading

jreback left a comment

albertvillanova commented Feb 21, 2019

albertvillanova commented Feb 27, 2019

jreback Feb 27, 2019

WillAyd Feb 27, 2019

albertvillanova Mar 2, 2019

jreback Feb 27, 2019

WillAyd Feb 27, 2019

jreback left a comment

albertvillanova commented Mar 16, 2019

jreback left a comment

jreback Mar 20, 2019

WillAyd commented May 3, 2019

jreback commented May 12, 2019

jreback left a comment

jreback May 19, 2019

jreback commented Jun 3, 2019

jreback commented Jun 27, 2019

WillAyd commented Aug 26, 2019

TomAugspurger commented Aug 26, 2019

WillAyd commented Aug 30, 2019

TomAugspurger commented Aug 30, 2019

WillAyd commented Sep 13, 2019

WillAyd commented Sep 20, 2019

Series pct_change fill_method behavior #25291

Series pct_change fill_method behavior #25291

Conversation

albertvillanova commented Feb 12, 2019 • edited by WillAyd Loading

pep8speaks commented Feb 12, 2019 • edited Loading

Comment last updated at 2019-08-30 16:38:20 UTC

codecov bot commented Feb 13, 2019

Codecov Report

codecov bot commented Feb 13, 2019 • edited Loading

Codecov Report

jreback left a comment

Choose a reason for hiding this comment

albertvillanova commented Feb 21, 2019

albertvillanova commented Feb 27, 2019

jreback Feb 27, 2019

Choose a reason for hiding this comment

WillAyd Feb 27, 2019

Choose a reason for hiding this comment

albertvillanova Mar 2, 2019

Choose a reason for hiding this comment

jreback Feb 27, 2019

Choose a reason for hiding this comment

WillAyd Feb 27, 2019

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

albertvillanova commented Mar 16, 2019

jreback left a comment

Choose a reason for hiding this comment

jreback Mar 20, 2019

Choose a reason for hiding this comment

WillAyd commented May 3, 2019

jreback commented May 12, 2019

jreback left a comment

Choose a reason for hiding this comment

jreback May 19, 2019

Choose a reason for hiding this comment

jreback commented Jun 3, 2019

jreback commented Jun 27, 2019

WillAyd commented Aug 26, 2019

TomAugspurger commented Aug 26, 2019

WillAyd commented Aug 30, 2019

TomAugspurger commented Aug 30, 2019

WillAyd commented Sep 13, 2019

WillAyd commented Sep 20, 2019

albertvillanova commented Feb 12, 2019 •

edited by WillAyd

Loading

pep8speaks commented Feb 12, 2019 •

edited

Loading

codecov bot commented Feb 13, 2019 •

edited

Loading