BUG: Remove null values before sorting during groupby nunique calculation
- [x] Closes #27904
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Does this completely close #27904? Or should it remain open for the case of `dropna=False`? In the case of `dropna=False`, what should `nunique` return? As in, do the `NaN`s all count as distinct values? I wouldn't expect them to, although as `np.nan == np.nan` returns `False`, so I'm not sure Just one NA value I think. And to be clear, we can leave handling `dropna=False` to a followup PR. Just want to make sure that we have an open issue for it. Actually... I'm going to push this off the 0.25.1 milestone (sorry @MarcoGorelli)

Can you check a few things:

1. How's the performance of this? I assume `np.lexsort` has to do some kinds of NA masking and filtering internally. Are we much slower than that?
2. It looks like there's a now unnecessary `if dropna` starting around line 1171 now. IIUC, `val` should now have all the na values removed, so masking again shouldn't be necessary. Sorry for the delay - yes, I agree with your decision, Tom :) I'll pick this up again next week Hello @MarcoGorelli! 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-06 15:50:47 UTC As noted as a comment in #27904, this works fine with numpy's own `nat` value...is it considered cheating just replace `pandas.NaT` with `np.datetime64("NaT")`? I tried that, and both the existing and the new tests (which include `dropna=False`) pass. Haven't looked deeply yet but does this also impact Series and DataFrame methods? The location of the change would imply so hence my reason for asking Thanks for your review!

Sure, whatsnew entry added.

If I've understood your question, then the `pandas.DataFrame.nunique` and `pandas.Series.nunique` methods didn't have this issue to begin with, and are unaffected by this change. Ah misread the file location but makes sense so thanks for confirming. Should be able to review more deeply over the next few days Thanks @MarcoGorelli!

Can you confirm that this fixed all of #27904, or are there outstanding tasks? That's right - thanks for having taken the time to review my work!