PERF: masked ops for reductions (sum)
State: closed

The current `nanops` has quite some complexity that is not needed for the masked arrays. This is a small proof of concept to have separate implementations for our nullable masked arrays, taking the sum case (and still ignoring the additional kwargs).

This is also quite a bit faster. On master:

```
In [1]: a = np.array([1, 2, 3, 4, 5, 6, 7, 8, 9, np.nan]*1000)  

In [2]: s1 = pd.Series(a, dtype="Int64")

In [3]: s2 = pd.Series(a, dtype="float64") 

In [4]: %timeit s1.sum()   
79.1 µs ± 2.39 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

In [5]: %timeit s2.sum() 
79.1 µs ± 1.48 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)
```

the nullable Int64 basically does the same as the nanops implementation for float. 
With this PR:

```
In [4]: %timeit s1.sum()  
21.5 µs ± 69.9 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

In [5]: %timeit s2.sum() 
79.8 µs ± 1.03 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)
```

(when using bigger arrays, the speed-up becomes less big in relative factor. Here it's almost 4x with 10k elements, but with 1M it's aound 2x).

I think it would be interesting to gradually implement some of the ops specifically for the nullable masked arrays. Personally I think it will be easier to do this in new functions than trying to fit it into the existing nanops function.



Labels:
name: Performance
description: Memory or execution speed performance
name: NA - MaskedArrays
description: Related to pd.NA and nullable extension arrays

Comments:
Looks promising
I updated this to finalize the "sum" case (basically added `min_count` support). 

cc @TomAugspurger @jreback 
More comments?
Some of the code here looks similar to the min/max methods in DatetimeIndex/TimedeltaIndex (specifically the Indexes, not the EAs) since those use their cached masks.  It'd be nice if some of that could be shared
> Some of the code here looks similar to the min/max methods in DatetimeIndex/TimedeltaIndex (specifically the Indexes, not the EAs) since those use their cached masks. It'd be nice if some of that could be shared

Do you mean this one?

https://github.com/pandas-dev/pandas/blob/37a700699235be283078847447ffdd00a244a295/pandas/core/indexes/datetimelike.py#L239-L271

There are indeed some similar concepts, but I am not sure if it is that easy to share code (if you have specific ideas, feel free to share). Eg the above doesn't need to handle null_count, in my version I can hardcode pd.NA instead of needing to make this a variable. And it would only be the lines 262-269 in the above code that could be replaced with a function call.

Maybe if we have a "templated" version (eg where you pass the numpy function to call as variable), as mentioned in one of the other inline comments, this would become easier. But I would personally only look into it when also implementing min/max in mask_ops (sum/prod are a bit special with the min_count).
@jorisvandenbossche we need to share more code with the very well tested internal functions 

adding more and more exclusively for the extension types just adds more bugs

i would refactor much sooner than later (now)

rather than continuing to add things 
So I know this is adding code, but reasons to do this separately: 

* The current nanops have a lot of complexity that is not needed for the masked arrays:
  * It deals with bottleneck switching, which is not needed here without NaNs
  * It deals with 2D, which is not needed here
* The nanops also take a different approach: it fills the array with a `fill_value` appropriate for the specific op / dtype combo, while here I am applying the mask to filter out missing values
* Trying to integrate the code and concepts I use here into nanops, will make the already complex code in nanops even more complex. So while in this PR I am adding a separated implementation, I think it actually gives a simpler and easier to understand end result than trying to combine both.
* This new implementation here is (IMO, but I also wrote it, so I am biased) much more readable than the existing nanops. 

I could try to do a POC of combining so we have something to compare with. But even if possible with not too much hooks, for the above reasons I prefer to keep the "masked ops" separate.

Personally, I would (short term) rather put effort in sharing the testing code instead of putting effort in sharing the actual implementation: improving testing by ensuring we re-use a lot of more of our extensive, existings tests for the numeric ops (so run those tests also for the nullable dtypes, instead of testing it separately as we do now).

Another way of sharing code could also be the other way around: instead of trying to integrate this into nanops; we could also start looking into gradually using those "masked ops" more for some of the existing dtypes. For example, as @jbrockmendel mentioned above, the Datetime/TimedeltaIndex min/max functions could use a masked op version of min/max. 
> There are indeed some similar concepts, but I am not sure if it is that easy to share code (if you have specific ideas, feel free to share).

The closest thing I had to a specific idea was "huh, this looks kinda similar to that other thing"
@jbrockmendel but so on second though: yes, I think we can probably call the mask_ops version from DatetimeIndex.min/max, but so for that first need to add a min/max function (now it's only sum). 
More comments on this?

I moved to the `min_count` checking to nanops so the code is now shared with the implementation there.

And an additional note: I don't directly plan to do something similar for all statistical functions in nanops (like corr, kurt, skew, median, ..). Those all have quite some convoluted logic in nanops, and I fully agree we cannot just duplicate that / reinvent the wheel for them. If we want to have those work for masked arrays as well, that can only be done by sharing the stat function specific logic. 
But for the `sum` I am doing here, there is not much convoluted logic. It's a very simple implementation, and by doing it custom for masked arrays as done in this PR, we can have a faster and easier to understand implementation (and I think the same will be true for prod, min and max, and maybe mean, but probably not for the others).
Added typing / docstring to `_below_min_count`.

> this entire file is in a really weird place.  ...  
> I think this PR needs to move things in the right place, ... 
> I believe we had this discussion originally when this location was selected (pre 1.0.0)

The ``masked_ops.py`` file was added in https://github.com/pandas-dev/pandas/pull/29842. I looked back there, but didn't see much discussion about the file or its location (it was first added to nanops.py, but both me and Brock preferred to have it in a different file (because that file is already long, and it doesn't fully fit there). And for the Kleene logical ops, the `core/ops/` directory made sense at the time).

Do you have a concrete suggestion for this PR of where you want to see this added instead? 
> Do you have a concrete suggestion for this PR of where you want to see this added instead?

core.ops generally refers to binary operators.  core.nanops is where we keep most of our reductions.

I need to take another look at the code here, but this might be a good fit for something like `array_algos.reductions`
@jreback how does Brock's proposal sound to you?
yep brock proposal sounds good
OK, moved, so this is ready for another look
does this need asv's?
> does this need asv's?

Added the Int64/boolean dtype to the parametrization of some existing benchmarks (Any/All/NanOps and FrameOps).

And added a short whatsnew notice.
lgtm @jbrockmendel @TomAugspurger if any comments
Don't think I'll have any today. Last I looked (a while ago) I was happy
with the overall strategy.

On Thu, Mar 26, 2020 at 8:09 AM Jeff Reback <notifications@github.com>
wrote:

> lgtm @jbrockmendel <https://github.com/jbrockmendel> @TomAugspurger
> <https://github.com/TomAugspurger> if any comments
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/30982#issuecomment-604421400>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAKAOIUVSY2EJRRMQC6TORLRJNHYZANCNFSM4KGISNPA>
> .
>

@jbrockmendel merge if you are good with responses
thanks @jorisvandenbossche 
And thanks for the reviews!