pct change bug issue 21200
State: closed

closes #21200
closes #21621
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This addresses [#21200](https://github.com/pandas-dev/pandas/issues/21200?_pjax=%23js-repo-pjax-container) . When there are different groups in a dataframe, by using groupby it is expected that the pct_change function be applied on each group. However, combining groupby with pct_change does not produce the correct result.

**Explanation:**
Currently the groupby method in the pandas series and pandas dataframe pct change method can implement a vectorized solution, rather than calling apply, if certain conditions are met. For the pandas series method, the vectorized solution is the only option.
This is certainly inappropriate in cases where the groupby object is non-monotonic in its group order. To solve this I've added a check for monotonicity in both the series and dataframe implementation, as well as adding the opportunity to call apply for the series method.
In addition, I have augmented the UT to accept a parameter that can shuffle the dataframe, in order to ensure that the correct calculation occurs.

**Concern**
One concern I have is that depending on whether the apply or vectorized solution is used within the pct change method (e.g. depending on whether the groupby object is monotonic or not), the result returned to the user may have a different index structure. While this was the case prior to the PR, It's not clear to me if (1) this is an acceptable design within the pandas infrastructure, and (2) whether or not this is within the scope of a single PR that was originally opened to address a very specific bug. 

As this is my first pandas PR, I would certainly appreciate feedback, and will incorporate any constructive feedback into future issues.

Labels:
name: Bug
name: Groupby

Comments:
Hello @SimonAlecks! Thanks for updating the PR.

 - There are no PEP8 issues in the file [`pandas/core/groupby/generic.py`](https://github.com/SimonAlecks/pandas/blob/f0a9c6d825721224ddbcf4b89ec6ab8410e98cf1/pandas/core/groupby/generic.py) !

 - There are no PEP8 issues in the file [`pandas/core/groupby/groupby.py`](https://github.com/SimonAlecks/pandas/blob/f0a9c6d825721224ddbcf4b89ec6ab8410e98cf1/pandas/core/groupby/groupby.py) !

 - There are no PEP8 issues in the file [`pandas/tests/groupby/test_transform.py`](https://github.com/SimonAlecks/pandas/blob/f0a9c6d825721224ddbcf4b89ec6ab8410e98cf1/pandas/tests/groupby/test_transform.py) !



##### Comment last updated on November 27, 2018 at 06:03 Hours UTC
Just eyeballing this I do not like that the implementation is going to vary depending on the monotonicity of the group as that has relatively large performance implications that the standard user will not understand nor should they care to

Is there any way to preserve the vectorized solution and come to the same result?
@WillAyd It's hard for me to think of a scenario where the vectorized solution would work if the data is not in order with respect to groups, although perhaps the monotonicity requirement is overkill? I'm new to Pandas source code, so I'll investigate the existing helper functions that may preserve the intended logic but lessen the requirements. 

I'm also a little concerned though that the vectorized solution may be getting the first element of each group wrong, if it's calculating its percentage change with respect to the last element of the previous group. I'll check this when I get home. 

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21235?src=pr&el=h1) Report
> Merging [#21235](https://codecov.io/gh/pandas-dev/pandas/pull/21235?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/89f044175801b8a3c468138f454d51eec849eddb?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/21235/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/21235?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #21235      +/-   ##
==========================================
+ Coverage   92.21%   92.21%   +<.01%     
==========================================
  Files         162      162              
  Lines       51763    51768       +5     
==========================================
+ Hits        47733    47740       +7     
+ Misses       4030     4028       -2
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.61% <100%> (ø)` | :arrow_up: |
| #single | `43% <0%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/21235?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/21235/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `96.67% <100%> (+0.16%)` | :arrow_up: |
| [pandas/core/groupby/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/21235/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9nZW5lcmljLnB5) | `87.12% <100%> (+0.04%)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/21235/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `87.41% <0%> (-0.1%)` | :arrow_down: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/21235/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `93.09% <0%> (+0.47%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21235?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21235?src=pr&el=footer). Last update [89f0441...01d705f](https://codecov.io/gh/pandas-dev/pandas/pull/21235?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

After changing the UT to have a value in the 0th mock array element, the UT begins to fail. The non-apply based vectorized solution doesn't seem to work, as it calculates a pct diff between the first element of a group, and the last element of the preceding group.

It seems to me the simplest solution to solve this issue is to remove the potential to use a vectorized solution. In addition to solving the original issue, it also solves the other issue I discovered above, as well as the additional bonus of letting the user expect the same index structure returned whether or not the vectorized style was called or not (granted, this could easily be fixed).

I suppose we could try preserving the vectorized solution, then writing a little algo to find the first element of each group and setting it to nan. I have no prior knowledge as to whether this would improve performance vs. calling apply. My intuition says at this point you'd be better off just using apply in the first place, but I could very well be wrong.

I also left my updated UT in, as it would catch these cases in case they occurred again, or if a vectorized solution was reimplemented. 

Any thoughts @WillAyd ?

PS: Does anyone have a link they can share with me on how to troubleshoot/debug the continuous-integration/travis-ci/pr failure?


Just looking at this can't you just changed the following line:

https://github.com/pandas-dev/pandas/blob/c85ab083919b59ce84c220d5baf7d34ff4a0bcf2/pandas/core/groupby/groupby.py#L2081

To be `self.shift` instead of `filled.shift`? I think that emulates exactly what the user is doing in the example cited so may just be a typo in the first place. That would preserve the performance and be much preferable if it works
In my most recent commit I tried to preserve vectorization, while also not making costly calls to is_monotonic, and preventing group contamination within the vectorization (e.g. first element of group B being divided by an element of group A).

I did this by only allowing the code to attempt vectorization if it can confirm that the series is monotonic, *and* if the truth value of whether or not it is monotonic already exists in the cache. If these conditions are met, it will do a vectorized pct change operation, and then a vectorized operation to set any values that shouldn't exist to np.nan (e.g. for shift=1 the first element of a group would be divided by the last element of the prior group, but this is an invalid value).

I expect this is trying to be too fancy given the problem and my knowledge of pandas source, so I'm prepared to nix it if this is the case, and just keep it simple and/or try to implement it in cython. Please just let me know.  Else I will extend it to the dataframe method as well. 

Hi William,
I tried what you suggested -- but I don't _think_ it makes sense. I think the current logic is correct in so far as shifted should simply be the filled series that is shifted once. Another way to say it, is changing the code to your suggestion wouldn't address the issue that non-monotonic group ordering is handled incorrectly, and that even if it were monotonic, the first element of a group may have a pct change with respect to the last element of the previous group.




For the reasons I mentioned above a vectorized solution will return incorrect values for pct_change under a set of conditions. I attempted to solve these conditions in the CR above, which William pointed out was too convoluted (I agree). 

The additional change you suggested (self.shift instead of filled.shift) does not work either for the reasons I mentioned above. The new test I wrote would fail under that change.

My suggestion is to update the test, remove the vectorized solution, and create a new issue to implement this in cython, which I'm interested in doing. 
Not sure what you mean by the benchmarks being blacklisted but for illustration purposes here is how your latest compares compares to master:

```bash
       before           after         ratio
     [415012f4]       [eaede342]
+         571±1μs          975±4ms  1708.33  groupby.GroupByMethods.time_dtype_as_group('float', 'pct_change', 'transformation')
+         572±2μs        974±0.6ms  1702.23  groupby.GroupByMethods.time_dtype_as_group('float', 'pct_change', 'direct')
+         592±3μs          635±8ms  1072.93  groupby.GroupByMethods.time_dtype_as_group('int', 'pct_change', 'transformation')
+         606±8μs          628±3ms  1036.87  groupby.GroupByMethods.time_dtype_as_group('int', 'pct_change', 'direct')
+        559±10μs         514±10ms   920.59  groupby.GroupByMethods.time_dtype_as_field('float', 'pct_change', 'direct')
+       529±0.9μs          481±5ms   908.79  groupby.GroupByMethods.time_dtype_as_field('float', 'pct_change', 'transformation')
+         603±8μs         460±20ms   762.64  groupby.GroupByMethods.time_dtype_as_field('int', 'pct_change', 'direct')
+         593±1μs          445±3ms   750.02  groupby.GroupByMethods.time_dtype_as_field('int', 'pct_change', 'transformation')

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
```

As you can see (and I think have already recognized) this isn't a great solution and kills performance.

> The additional change you suggested (self.shift instead of filled.shift) does not work either for the reasons I mentioned above. The new test I wrote would fail under that change.

I'm not entirely clear on what your reasons are. It's not a requirement that grouped elements be monotonic in any of the Cythonized code. 

If you are getting an error on the test can you be explicit about *what* exactly that error is? IIRC correctly with the last set of tests what I suggested worked fine. I have a hunch that there may be confusion about the test as it was before (i.e. explicitly passing `None` would fail, but that was also expected). I could be wrong but in any case clarifying the failures you are seeing would be very helpful.

Final note - thanks for helping on this. I hope you are taking this feedback constructively and not in any negative fashion (I believe you are but just want to be explicit). Understanding how / when to interface with the Cythonized code is one of the more complicated aspects of this package
I'm very grateful for the feedback, and I definitely appreciate your time. I apologize for the delay since the last commit. I've been working on understanding the code more clearly.

In this situation I've attempted to solve the problem by incorporating your prior feedback. My previous posts were incorrect, and you correctly pointed out to me how the shift operator works.

In this latest CR I've attempted to fix the problem in line with the spirit of the original implementation. The first step is I added an explicit catch for a fill_method, which did not exist before.

I initially attempted to implement your 1-line suggestion, but I noticed that I couldn't simply call self.shift(), as it first needed to have the proper fill method. However, the fill method returns the obj attribute, not the groupby object. 

In order to remedy this, without creating a function that changed the state of the code, I used the following strategy:
https://github.com/SimonAlecks/pandas/blob/64097ccd308db909dc99cbb88eeb6206beedb820/pandas/core/groupby/groupby.py#L2079
I first recreate a new groupby object, but I solve the fill problem as part of this group. I remember I then had to reset the cache in order for some of the meta-data to align properly. I convinced myself this was necessary at the time, but I didn't write down why, so I'm going to go revisit that now to remind myself that I did this for a good reason. More generally, I could see it being the case that this solution is a hack or not idiomatic within pandas. If so, could you please let me know the correct way to achieve this same result?

Once this is done, the result is relatively self-explanatory. and is pretty similar to the original code.

Lastly, I ran an ASV benchmark locally, and the performance seemed similar, but I need to spend some more time teaching myself how the benchmarking system and code works, as it's possible I made an error.
Incorporated most requested changes, with a few clarifying questions asked on a few of them. Thanks for your patience! 
@SimonAlecks do you believe this change would also solve #21621?1 If so, could you add a test cases for that issue?
Since you mentioned that this PR fixes #21621, can you go ahead and add a test for that issue?
@mroeschke I think the test I already wrote (well, augmented), should already cover it. It's essentially the exact problem I have attempted to solve here. If you disagree can you enumerate on the point? I don't mind adding a new test if I'm missing something, but it's not clear to me it's necessary. 
Hmm OK thanks. So I see the difference now between the two sets of code we have. Here's a very small example to illustrate:

```python
df = pd.DataFrame([['a', 1], ['a', np.nan], ['a', 2]], columns=['key', 'val'])
```

Your code yields the following:
```python
In [4]: df.groupby('key').pct_change()
Out[4]: 
   val
0  NaN
1  0.0
2  1.0
```

whereas my branch would do as follows:
```python
In [4]: df.groupby('key').pct_change()
Out[4]: 
   val
0  NaN
1  0.0
2  NaN
```

There difference is whether or not the fill method is getting applied to both the numerator and denominator or just the denominator.

Your's is preferable behavior though the code should still be simplified. Can you see if there's a way to still emulate your behavior borrowing some of my code? It may just need a small tweak, but doing that would be preferable to the copy and private method calls you are currently making
> Hmm OK thanks. So I see the difference now between the two sets of code we have. Here's a very small example to illustrate:

I cleaned up the code a fair amount. Let me know if anything is still unpalatable or not in line with what you were thinking. 

Thanks for the updates @SimonAlecks . Note that we've reorganized some of the modules here so you'll want to merge in master on your next PR and resolve any merge conflicts
@SimonAlecks can you merge master?
Closing as stale. Ping if you'd like to pick this one back up
Hi William,

I did let it go stale -- but I would like to finish this if that's okay.

Thanks,
Simon


On Thu, Nov 22, 2018 at 7:27 PM William Ayd <notifications@github.com>
wrote:

> Closed #21235 <https://github.com/pandas-dev/pandas/pull/21235>.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/21235#event-1983392340>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AIchu0hgrwHc856BiiUxyBC_CprJR0j-ks5ux2s1gaJpZM4UQqbk>
> .
>

Thanks! Ping if you need more help or clarification 
Thanks for your thoughtful response. Using self.grouper.labels seems to be exactly what I was looking for -- thanks for pointing that out! In adding these changes to series and dataframe groupby, I also noticed that the vectorized operation would break for certain usages of frequency. Specifically, the code would *break* on the [following line](
https://github.com/pandas-dev/pandas/pull/21235/commits/5d3fc2dd6752a04ddeb56989d397ea9ee85c8228#diff-bfee1ba9e7cb79839776fac1a57ed940R1231), when an additional index was added. In addition, the shifted/filled series would have mismatched datetime indices, which was causing other issues. As a result I added a conditional to simply call the normal pct change in that case, as well as an additional test check.

Looking a lot better. Can you provide a concrete example of where `freq` is breaking things? Really want to avoid using an anonymous func as it kills performance.

FYI you also have what I assume to be a stylistic error causing the Travis failure. Make sure to run `ci/code_checks.sh` locally to see what may be causing it
Hi William,

To start with I'll mock data and call the generic groupby pct change as it exists in the latest commit to see the intended behavior -- under the assumption that calling the anonymous function returns the intended behavior, and the vectorized operation replicates this same output, only faster. I will then go through the function and point out where I see issues, and articulate why I believe this to be the case. 

> #### Code Sample
> 
> ```python
> >>>import pandas as pd
> 
> >>> idx = pd.DatetimeIndex(start='2017-01-01', end='2017-01-03', freq='D')
> >>> df = pd.DataFrame({
> >>>    'a': [4.0405, 4.0963, 4.3149],
> >>>    'b': [1.7246, 1.7482, 1.8519],
> >>>    'c': [804.74, 810.01, 860.13],
> >>>    'grp': ['a', 'a', 'a']},
> >>>    index=idx)
> >>> expected = df.groupby('g')['a'].pct_change(freq='D')
> >>> print(expected)```

**Output**
>    Out[26]: 
>     2017-01-01         NaN
>     2017-01-02    0.013810
>     2017-01-03    0.053365
>     Freq: D, Name: a, dtype: float64
> 

The next step is to comment out the following chunk, and explore why this causes the tests to fail, using the same mocked data as above:
https://github.com/pandas-dev/pandas/blob/5d3fc2dd6752a04ddeb56989d397ea9ee85c8228/pandas/core/groupby/generic.py#L1225-L1228

When doing this, I encounter troubles here:
https://github.com/pandas-dev/pandas/blob/5d3fc2dd6752a04ddeb56989d397ea9ee85c8228/pandas/core/groupby/generic.py#L1231

I notice that when this line is called with a non-none value of freq (e.g 'D'), it returns a dataframe with an added named index dimension, turning it into a multi-index. I'm not sure if this is intended behavior or not, but even it it isn't, it feels out of scope, as a result I'm weary of going down the rabbit hole of trying to change this, at least not without encouragement. 

Below are examples generated at this breakpoint with and without a frequency argument, to demonstrate the above point: 

> #### Code Sample
> ```python
> 
> >>> fill_grp.shift(periods=periods, freq='D')

>      Out[11]: 
>      0   2017-01-02    4.0405
>          2017-01-03    4.0963
>          2017-01-04    4.3149
>          Name: a, dtype: float64

> ```python
> >>> fill_grp.shift(periods=periods, freq=freq).index

>      Out[15]: 
>      MultiIndex(levels=[[0], [2017-01-02 00:00:00, 2017-01-04 00:00:00, 2017-01-05 00:00:00]],
>      labels=[[0, 0, 0], [0, 1, 2]])

> #### Code Sample
> ```python
> >>> fill_grp.shift(periods=periods, freq=None)

>     Out[12]: 
>      2017-01-01       NaN
>      2017-01-02    4.0405
>      2017-01-03    4.0963
>      Freq: D, Name: a, dtype: float64
> 

Assuming I were to fix this multi-index issue by just dropping it within the function (although I'm skeptical this solution would generalize):

> ```python
> >>> shifted.reset_index(level=0, drop=True, inplace=True)
> ```
 
There is another difference, where the time-index structure does not match the expected behavior above. Although I suspect this could be solved by reindexing on the original self.obj.index.
> #### Code Sample
> ```python
> >>> out = (filled / shifted) - 1
> >>> print(out)
**output**
>     Out[27]: 
>     2017-01-01         NaN
>     2017-01-02    0.013810
>     2017-01-03    0.053365
>     2017-01-04         NaN
>     Freq: D, Name: a, dtype: float64

I don't mind spending a few hours or more diving deeper into the code to see if I can solve the issue of the line returning a multi-index, and then reindexing the final line of (filled / shifted) -1. But before I give that a shot, I wanted to lay out my understanding of the problem, and see if you have any suggestions or concerns.

(As a side note, I welcome any meta-feedback on my articulation of the problem, or how I can be more clear).



Thanks for the thorough example and understood. I opened #23918 for that as a separate issue.

Instead of falling back to the lambda could we inspect `shifted` when `periods` is non-zero and a `freq` is specified and simply drop the first index level? If we add a comment referencing the other issue I think it's clearer that that code is a workaround and should be adjusted accordingly whenever the other item gets fixed
No problem, thanks for your response.

My first attempt at implementing your suggestion had some unexpected outcomes. 

After this line I added this:
https://github.com/pandas-dev/pandas/blob/5d3fc2dd6752a04ddeb56989d397ea9ee85c8228/pandas/core/groupby/generic.py#L1231

> ```python
> >>> shifted = fill_grp.shift(periods=periods, freq=freq)
> >>> if isinstance(shifted.index, MultiIndex):
> >>>         shifted.reset_index(level=0, drop=True, inplace=True)

From here, using the test data, we can notice:
> ```python
> >>> print(filled)

> Out[28]: 
2010-01-01     3.0
2010-01-02     3.0
2010-01-03     NaN
2010-01-04     NaN
2010-01-05     1.0
2010-01-06     2.0
2010-01-07     4.0
2010-01-08    10.0
2010-01-09    10.0
2010-01-10     4.0
2010-01-01     3.0
2010-01-02     3.0
2010-01-03     NaN
2010-01-04     NaN
2010-01-05     1.0
2010-01-06     2.0
2010-01-07     4.0
2010-01-08    10.0
2010-01-09    10.0
2010-01-10     4.0
Name: vals, dtype: float64

> ```python
> >>> print(shifted)

> Out[29]: 
2010-01-02     3.0
2010-01-03     3.0
2010-01-04     NaN
2010-01-05     NaN
2010-01-06     1.0
2010-01-07     2.0
2010-01-08     4.0
2010-01-09    10.0
2010-01-10    10.0
2010-01-11     4.0
2010-01-02     3.0
2010-01-03     3.0
2010-01-04     NaN
2010-01-05     NaN
2010-01-06     1.0
2010-01-07     2.0
2010-01-08     4.0
2010-01-09    10.0
2010-01-10    10.0
2010-01-11     4.0
Name: vals, dtype: float64

> ```python
> >>> print((filled / shifted) - 1)

> (filled / shifted) - 1
Out[30]: 
2010-01-01    NaN
2010-01-01    NaN
2010-01-02    0.0
2010-01-02    0.0
2010-01-02    0.0
2010-01-02    0.0
2010-01-03    NaN
2010-01-03    NaN
2010-01-03    NaN
2010-01-03    NaN
2010-01-04    NaN
2010-01-04    NaN
2010-01-04    NaN
2010-01-04    NaN
2010-01-05    NaN
2010-01-05    NaN
2010-01-05    NaN
2010-01-05    NaN
2010-01-06    1.0
2010-01-06    1.0
2010-01-06    1.0
2010-01-06    1.0
2010-01-07    1.0
2010-01-07    1.0
2010-01-07    1.0
2010-01-07    1.0
2010-01-08    1.5
2010-01-08    1.5
2010-01-08    1.5
2010-01-08    1.5
2010-01-09    0.0
2010-01-09    0.0
2010-01-09    0.0
2010-01-09    0.0
2010-01-10   -0.6
2010-01-10   -0.6
2010-01-10   -0.6
2010-01-10   -0.6
2010-01-11    NaN
2010-01-11    NaN
Name: vals, dtype: float64


I suspect I could solve this with a little investigation of the root cause for the index doubling, which seems to be due to having duplicates of dates in each series, and potentially trying to modify the index structure. In fact, I noticed this same issue would occur in the canonical pct_change method if there are duplicate date values in the series. 

Do you think it's worth trying to solve this issue?
No - the more I think about it the referenced issue is going to cause misalignment of values so division probably won’t work. As you have it is probably the best we can do here. 

Can you add a TODO comment referencing the issue and fix up the Travis failure (it’s coming from ci/code_checks.sh)
Added the whatsnew. I checked for previous examples in the docs to follow style guidelines, but please let me know if it's too verbose or I made a stylistic mistake, and I'll quickly fix it. Thanks.
thanks @SimonAlecks 
@jreback and @WillAyd thanks so much for your patience on my first PR! 
Any suggestions for the next bug I tackle? I'd like to learn the index structure. (I'm perfectly capable of finding a bug to work on on my own, just curious if anything in particular that you have a preference for).