Make group_mean compatible with NaT
State: closed

Make group_mean compatible with NaT

## Background
NaT is the datetime equivalent of NaN and is set to be the lowest possible 64-bit integer -(2**63). Previously, we could not support this value in any groupby.mean() calculations which lead to #43132.

## Implementation
On a high level, we slightly modify the `group_mean` to not count NaT values. To do so, we introduce the `is_datetimelike` parameter to the function call (already present in other functions, e.g., `group_cumsum`).

## Tests
This PR adds integration and unit tests for the new functionality. In contrast to other tests in classes, I've tried to keep an individual test's scope as small as possible.

## Notes
Additionally, I've taken the liberty to:
 * Add a docstring for the group_mean algorithm.
 
- [x] closes #43132
- [x] tests added / passed
- [x] Ensure all linting tests pass, see [here](https://pandas.pydata.org/pandas-docs/dev/development/contributing.html#code-standards) for how to run them
- [x] whatsnew entry

Labels:
name: Groupby
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate

Comments:
Looks like there is a conflict with https://github.com/pandas-dev/pandas/commit/6f4c382e17e14751005aefd6aeb44e97e5e5a4a6, will add `mask` and `result_mask` as parameters. @jbrockmendel .
> Looks like there is a conflict with

you'll need to merge master and resolve the conflict
@jbrockmendel There not a merge conflict but a semantic conflict: `mask` and `result_mask` were added to the aggregate invocation that this commit modifies in [ops.py](https://github.com/pandas-dev/pandas/pull/43467/commits/f023f4bb0ecde5e3163627d1e243b111ab452c81#diff-17420aa54e4b74826eaaeedc6ef6b320c16691aa74c09420e6098854284b4874) but that shouldn’t be too hard to fix (however, a bit more difficult than just a merge conflict). Just need some more time.
I have a couple of questions for the review: 
 * I am not sure if I filled out `whatsnew` correctly.
 * Should I have put the new integration test under `pandas/tests/groupby/transform/test_transform.py` or under `pandas/tests/groupby/test_groupby.py`. I've opted for just one integration test, should I create more? If yes, any tests that you would like to see?
Master is currently broken, CI cannot pass:
```
pytest pandas/tests/groupby/test_quantile.py -x
```
results in
```
>   ndarray[int64_t] counts, non_na_counts, sort_arr
E   TypeError: group_quantile() takes exactly 6 positional arguments (4 given)
```

Waiting on green master before submitting the PR for review.
@AlexeyGy Thanks for the PR.

The PR in the current state gives the expected output for #43132 but does not fix #42659 which looks like a very similar issue.

The fact that both these regressions were the result of the same commit, I had hoped that a single patch would resolve both issues.

Also because this is resolving a regression we should target 1.3.x. We don't have a milestone for 1.3.x and 1.3.3 is scheduled for tomorrow. I'll milestone 1.3.4 for now.
> Master is currently broken, CI cannot pass:

not looked in detail but #43512 just ran through ci fine.
Thanks @AlexeyGy for the updates. No need to add the 1.3.4 whatsnew (fails anyway until the 1.3.3 tag is available) it will be done just after the 1.3.3 release.

best bet is to leave the release note in 1.3.3 for now, get ci passing and mark the PR as ready for review.
@simonjayhawkins thanks for getting back. Agreed, as suggested, will adapt to 1.3.3 and try to get CI to pass.
Hello @AlexeyGy! 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 2021-09-15 19:43:05 UTC
Looks like CI is failing after 60 mins due to timeout or am I missing something? Not sure what to do now.
> Looks like CI is failing after 60 mins due to timeout or am I missing something? Not sure what to do now.

its fine for now. these should now pass in the time limit, but are still failing occasionaly, each time you push be sure to merge master (if you are not already).
As a sidenote, does anyone know why `group_mean` doesn't use `group_add`? Seems like the right long-term solution would be to allow `group_mean` to take the fused type `add_t`, then just call `group_add` to do all the additions it does anyway. This would fix the issues described above since datetimelike ints wouldn't need to be cast to float (although it appears `group_add` doesn't use the `datetimelike` arg, so that might need to be fixed first). 
> As a sidenote, does anyone know why `group_mean` doesn't use `group_add`? Seems like the right long-term solution would be to allow `group_mean` to take the fused type `add_t`, then just call `group_add` to do all the additions it does anyway. This would fix the issues described above since datetimelike ints wouldn't need to be cast to float (although it appears `group_add` doesn't use the `datetimelike` arg, so that might need to be fixed first).

historical :-> we should try to unify things.
> Left some concerns about implicit casts and edge cases with data near NPY_NAT.

Yah don't worry about that for now.  This particular problem pop is hidden in a bunch of places and we don't have a great solution for it.  I mentioned it because in that one particular line it happened to be more obvious than it is in other places.

> As a sidenote, does anyone know why group_mean doesn't use group_add?

Best guess is it wasn't an active decision, just turned out that way.  Many people working on focused pieces of the code over many years, refactoring opportunities pile up.
On the discussion about `group_add` and `group_mean`, just a quick question: which type does the pandas team prefer? 

 A. `add_t` like [group_add](https://github.com/pandas-dev/pandas/blob/177f1abcf5cb50885484290bcfa8f236afc8767a/pandas/_libs/groupby.pyx#L486) 
B. or the Cython-native `floating` [group_mean`](https://github.com/pandas-dev/pandas/blob/177f1abcf5cb50885484290bcfa8f236afc8767a/pandas/_libs/groupby.pyx#L672) 
as an argument for functions? 

From what I see both are union types but I find it very hard to argue about Cython's floating. When I look into [Shadow.py](https://github.com/cython/cython/blob/master/Cython/Shadow.py): `integral = floating = numeric = _FusedType()` it seems very non-transparent what it can or cannot be.
> On the discussion about `group_add` and `group_mean`, just a quick question: which type does the pandas team prefer?
> 
AFAIU `floating` is just another fused type (https://cython.readthedocs.io/en/latest/src/userguide/fusedtypes.html#built-in-fused-types). So not really a difference except that `floating` is defined by `cython`. But I think it's equivalent to just defining our own fused type with `float32` and `float64`
i am ok to merge this as its patching a regression, cc @mzeitlin11 @jbrockmendel certainly this area could use some refactoring (e.g. should we have all function shave the common signature as min,max,mean so we can call them generically) or similar things.
> i am ok to merge this as its patching a regression

Fine merging this too, just want to make sure not causing a _new_ regression (https://github.com/pandas-dev/pandas/pull/43467#pullrequestreview-753196593) (or that we accept this regression as more of an edge case than the other)


@AlexeyGy can you add a test for this

> I don't think the concerns are easy to address, though, since we coerce data to float in group_mean. Since this is fixing a regression, I'd be fine merging as is, though would feel better if testing was added to make sure dates that become pd.NaT + 1, pd.NaT + 2 don't now become treated as missing.


Added a test and realized that we actually [HAVE to stop converting to float as per this comment thread](https://github.com/pandas-dev/pandas/pull/43467#discussion_r709588997) since the float64 precision is way too low to even add 1 to NaT in the input data:

```python
import numpy as np
np.array(np.iinfo(np.int64).min).astype('float64') ==  ( np.array(np.iinfo(np.int64).min).astype('float64') + 1)
```
Returns `True`.

Actually, even adding 100 would have no effect, I've noticed that numbers <~1000 (didn't check the exact number) get lost due to fp-arithmetic.

> I've noticed that numbers <~1000 (didn't check the exact number) 

I think its around 2^54 where the conversion gets lossy
ok i think this is fine to merge. we have this NaN/NaT comparison issue currently everyone so no reason to hold this up.
thanks @AlexeyGy very nice!
@meeseeksdev backport 1.3.x
Something went wrong ... Please have  a look at my logs.