BUG: Fix numpy boolean subtraction error in Series.diff
State: closed

A new version of the pull request (#27755), the other one was a bit behind. Original text below.

This fixes #17294, for more than three years now NumPy has not allowed the subtraction of boolean series. 

TypeError                                 Traceback (most recent call last)
<ipython-input-46-3da3b949c6bd> in <module>
      1 data = pd.Series([0,-1,-2,-3,-4,-3,-2,-1,0,-1,-1,0,-1,-2,-3,-2,0])
      2 filtered = data.between(-2,0, inclusive = True)
----> 3 filtered.diff()
      4 print(filtered)

~\AppData\Local\Continuum\anaconda3\lib\site-packages\pandas\core\series.py in diff(self, periods)
   2191         dtype: float64
   2192         """
-> 2193         result = algorithms.diff(com.values_from_object(self), periods)
   2194         return self._constructor(result, index=self.index).__finalize__(self)
   2195 

~\AppData\Local\Continuum\anaconda3\lib\site-packages\pandas\core\algorithms.py in diff(arr, n, axis)
   1817             out_arr[res_indexer] = result
   1818         else:
-> 1819             out_arr[res_indexer] = arr[res_indexer] - arr[lag_indexer]
   1820 
   1821     if is_timedelta:

TypeError: numpy boolean subtract, the `-` operator, is deprecated, use the bitwise_xor, the `^` operator, or the logical_xor function instead.

- [x] closes #17294
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Bug
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: Dtype Conversions
description: Unexpected or buggy dtype conversions

Comments:
Hello @Unprocessable! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:







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

##### Comment last updated at 2019-09-30 11:22:22 UTC
Are we in agreement with https://github.com/pandas-dev/pandas/pull/28251#discussion_r320818052? That the non-NA elements should be booleans just like NumPy's `np.diff` on bool data?
Yea I think that makes the most sense. ~~Will take up more space but let's leave optimization as a down the road exercise to implement BoolNA~~
Come on people, make up your minds. I have two versions now, the original commit is the original implementation, but working with a new numpy version, the second one also dealing with np.nan object 'boolean' arrays.

Which one should be implemented?
@TomAugspurger and @WillAyd want the second implementation
@jreback and me the first
That leaves @jbrockmendel and @simonjayhawkins
Unless it is not a democracy, then feel free to tell me which one to pick.

@Unprocessable how about waiting to update. This is slightly non-trivial as to scope.
@jreback You are right. I assumed that a bug that went unnoticed for several years did not affect the scope that much. I will wait, but I would like to have this bug fixed in the next release if possible. 
> @jreback You are right. I assumed that a bug that went unnoticed for several years did not affect the scope that much. I will wait, but I would like to have this bug fixed in the next release if possible.

thanks @Unprocessable *many* times bugs go unfixed for long periods of time. Its only when they  are being fixed things are looked at more closely.
@jreback what's your ideal behavior?

My preference is for us to implement the behavior we would desire *if* we had a nullable BoolArray. 

Do you agree that if we had BoolArray, then `Series[bool].diff() -> Series[bool]` rather than `-> Series[float]` like we do today, when there are some NAs present?
> @jreback what's your ideal behavior?
> 
> My preference is for us to implement the behavior we would desire _if_ we had a nullable BoolArray.
> 
> Do you agree that if we had BoolArray, then `Series[bool].diff() -> Series[bool]` rather than `-> Series[float]` like we do today, when there are some NAs present?

yes I am not averse to implementing this, but want to do it generically. We have *very* little object support inside that routine and that's why I think adding here now is not great.


@TomAugspurger The original code and my first commit also do that. If the dtype is a bool, it will return a bool. 

The problem arises when the Series has the object dtype due to the presence of float dtype NA's before running the diff function. 

I would say that once the NA in the Series is seen by pandas as a bool dtype, a Series of [True, False, nan] will have a bool dtype and the code should execute correctly.
> yes I am not averse to implementing this, but want to do it generically. We have very little object support inside that routine and that's why I think adding here now is not great.

Do you consider this blocked by a BoolArray implementation? We'll still need to get users migrated to that API, so people with an object-dtype ndarray of bool & nan will still hit this issue.

I think for now I'm OK with supporting on object-dtype ndarray of bool & NaN, since it'll be ~5ish LoC to support properly, and it establishes the correct behavior going forward.
There are two separate issues:
1. diff on a boolean Series currently does not work (- deprecated)
2. the current and previous implementation caused bool + NA to return floats

My goal was definitely fixing issue 1 and maybe fixing issue 2.
@TomAugspurger if I understand you correctly, you see issue 2 as more pressing, am I correct?
Fair point. Fixing 1 is probably more pressing.

But regardless of how where we fix 2, I'd like to not add tests with
"incorrect" behavior (diff returning floats) in this PR.

On Tue, Sep 10, 2019 at 9:46 AM Unprocessable <notifications@github.com>
wrote:

> There are two separate issues:
>
>    1. diff on a boolean Series currently does not work (- deprecated)
>    2. the current and previous implementation caused bool + NA to return
>    floats
>
> My goal was definitely fixing issue 1 and maybe fixing issue 2.
> @TomAugspurger <https://github.com/TomAugspurger> if I understand you
> correctly, you see issue 2 as more pressing, am I correct?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/28251?email_source=notifications&email_token=AAKAOIVJZ6V6W6V35INMD7LQI6XL5A5CNFSM4IS6G5PKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6LLKLQ#issuecomment-529970478>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOIRL6NUJ6VGD4APYKDDQI6XL5ANCNFSM4IS6G5PA>
> .
>

That I can get behind. So either we fix the second issue as well or we leave it for another bug report. Feel free to discuss.

Edit: I also noticed the original description of diff says 'analogous to s-s.shift(n)', which is neither the new, current nor old implementation for the bool dtype whether for pandas or numpy.
@TomAugspurger Do you agree that I should remove the object part, like jreback requested, so we can at least fix the first issue? I have changed the test for the nan series to test the result of an object series, so it no longer tests for incorrect behaviour.
@Unprocessable yes, that sounds best for now.

CI should be fixed if you merge master.
As far as I know, this should work. It is changed back to the same code it was several tens of commits ago. If it does not work, I will fix it later. If everything checks out, feel free to merge with master.
@TomAugspurger @jreback 
Do you currently agree that we ignore the object nan Series for now and only focus on the boolean-only series? The current commit reflects that change at the moment.
Agreed with your fix in the method, and @WillAyd's comment about the test. I'd prefer a focused, new test like

```python
def test_diff_bool(self):
        s = Series([False, True, True, False, False])
        result = s.diff()
        expected = Series([nan, True, False, True, False])
        assert_series_equal(result, expected)
```

And leaving out the object-dtype Series.diff test.
> And leaving out the object-dtype Series.diff test.

I just tested a diff with a combination of datetime values and floats, this will crash as well. As long as there is a combination of integers, floats (nan included) or booleans, the object diff will work. While testing that specific case might not be a good idea, we do not want to introduce a patch that will make booleans from a float/int object series. For that reason I think it is better to include a test for that. 

@TomAugspurger How would you test that a diff on a combined boolean and float object series results in floats instead of booleans? I am pretty certain that someone doing a diff on [0, 1.0, True] expects [nan, -1.0, 0] instead of [nan, True, False]. The test reflects that.


I'd like this diff to be as small as possible. I'd only like to fix the original issue: calling `-` on a boolean ndarray.

Let's leave the rest for another issue.
I have been fixing the tests as well as there was a need for that from you (among others). I would rather not (re)write any tests, but I get the need for them. The issue is now fixed and the boolean diff test is both parametrized and split from the rest.

> Let's leave the rest for another issue.

I agree. If anyone does not agree with how it was written, it will be possible to change it later. The code works, does what it should do and should probably be merged before 1.0.
thanks @Unprocessable 

sorry this took a while. sometimes PRs trigger a lot of discussion about the original issue (which is what happened here).
No worries, it is good to see a simple question/PR causing a review of the core code. At least it means it was due for some commenting. This is also my first contribution to an open source project, so there is that. Thanks to all for keeping pandas updated and working!