ENH: enable skipna on groupby reduction ops #43671

geoffrey-eisenbarth · 2021-09-20T18:41:27Z

closes ENH: enable skipna on groupby reduction ops #15675
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

@mzeitlin11 Any help you can provide on what tests I should add, or where to start digging for similar GroupBy tests would be appreciated. As stated in PR #41399, I'm not super familiar with Cython, so hoping that the previous contributors (stale) PR is close to what's needed (I suppose the tests will tell us).

Thanks!

mzeitlin11 · 2021-09-21T02:19:00Z

Thanks for reheating this @geoffrey-eisenbarth! Might not have much time this week, but will look deeper at the cython and testing when I can. For now, I'd recommend fixing up the easier issues like precommit first.

With respect to testing, I'd take a look at the tests we have for groupby ops which do accept skipna (like any/all, the cumulative methods) and try to ensure similar coverage (as in make sure the new possibility skipna=False is covered for different types). Can look at tests in pandas/tests/groupby for inspiration. Some testing is nicely organized, some testing is stuffed in places like pandas/tests/groupby/test_functions.

Also, until we add tests, probably best to put the pr in draft state so others know it's not fully ready for review

mzeitlin11

Just some initial syntax comments, will take a more thorough look at logic when I can

mzeitlin11 · 2021-09-21T02:20:09Z

pandas/_libs/groupby.pyx

@@ -709,6 +723,11 @@ def group_mean(floating[:, ::1] out,
                    t = sumx[lab, j] + y
                    compensation[lab, j] = t - sumx[lab, j] - y
                    sumx[lab, j] = t
+                # don't skip nan
+                elif skipna == False:


This skipna is missing in the function arguments, causing a build failure

Also should be elif not skipna

mzeitlin11 · 2021-09-21T02:20:46Z

pandas/_libs/groupby.pyx

@@ -603,6 +612,10 @@ def group_prod(floating[:, ::1] out,
                if val == val:
                    nobs[lab, j] += 1
                    prodx[lab, j] *= val
+                # don't skip nan
+                elif skipna == False:


Suggested change

elif skipna == False:

elif not skipna:

mzeitlin11 · 2021-09-21T02:20:58Z

pandas/_libs/groupby.pyx

@@ -555,6 +559,10 @@ def group_add(add_t[:, ::1] out,
                        t = sumx[lab, j] + y
                        compensation[lab, j] = t - sumx[lab, j] - y
                        sumx[lab, j] = t
+                    # don't skip nan
+                    elif skipna == False:


Suggested change

elif skipna == False:

elif not skipna:

jbrockmendel · 2021-09-21T22:32:04Z

pandas/_libs/groupby.pyx

@@ -530,6 +531,9 @@ def group_add(add_t[:, ::1] out,
                    else:
                        t = sumx[lab, j] + val
                    sumx[lab, j] = t
+                elif skipna == False:
+                    # NOTE: Does this case need to be considered?


yes. if skipna is False and not checknull(val) (L524 above) then we sumx[lab, j] needs to be incremented by val (so will either become NaN or raise)

geoffrey-eisenbarth · 2021-09-27T19:38:21Z

Is CircleCI a new addition to the pandas contribution flow? I don't recall running into this when I worked on PR 41321.

When I click on the details, it says I need a configuration specified for this project. Is that something I need to do on my own?

mzeitlin11 · 2021-09-28T02:58:49Z

Is CircleCI a new addition to the pandas contribution flow? I don't recall running into this when I worked on PR 41321.

When I click on the details, it says I need a configuration specified for this project. Is that something I need to do on my own?

If you merge master it should pick up the config and fix this failure

github-actions · 2021-10-29T00:03:08Z

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

jbrockmendel · 2021-11-01T18:01:26Z

@geoffrey-eisenbarth planning to pick this back up? it looked promising

geoffrey-eisenbarth · 2021-11-01T18:05:07Z

@jbrockmendel I was hoping to, since I rely on the old sum(level=level, skipna=False) syntax quite a bit. I know I let it fall behind and now two days ago was the birth my first child, so now it feels like I have zero time, but I will try to merge with master today to fix the CircleCI issues that came up (I'm not sure if CircleCI is new, but my previous contribution didn't seem to involve it) and see where that takes us.

jbrockmendel · 2021-11-01T22:40:11Z

No sweat, totally reasonable to have higher priorities under the circumstances.

Not sure about the circleci bit; often times I re-push and hope for CI issues to resolve themselves.

pep8speaks · 2021-11-02T16:09:58Z

Hello @geoffrey-eisenbarth! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file pandas/core/groupby/generic.py:

Line 340:1: E305 expected 2 blank lines after class or function definition, found 1
Line 340:3: E227 missing whitespace around bitwise or shift operator
Line 340:7: E225 missing whitespace around operator
Line 341:5: E113 unexpected indentation
Line 388:3: E225 missing whitespace around operator
Line 388:5: E225 missing whitespace around operator
Line 388:7: E225 missing whitespace around operator
Line 389:7: E225 missing whitespace around operator

jreback · 2022-01-16T18:02:27Z

would likely take this, but needs to be passing an fully code-reviewed. closing as stale.

geoffrey-eisenbarth added 3 commits September 15, 2021 20:24

ENH: Initial commit to add skipna parameter to groupby.

ee4134e

ENH: Second commit to add skipna parameter to groupby.

e319655

Add skipna reference to whatsnew.

23c84b4

mzeitlin11 changed the title ~~Skipna~~ ENH: enable skipna on groupby reduction ops Sep 21, 2021

mzeitlin11 reviewed Sep 21, 2021

View reviewed changes

mzeitlin11 added Enhancement Groupby Reduction Operations sum, mean, min, max, etc. labels Sep 21, 2021

jbrockmendel reviewed Sep 21, 2021

View reviewed changes

geoffrey-eisenbarth marked this pull request as draft September 21, 2021 23:54

Fix function signature syntax error and simplify elif statements.

440ad04

github-actions bot added the Stale label Oct 29, 2021

Add skipna progress before merging in upstream master.

de8b9fe

jreback closed this Jan 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: enable skipna on groupby reduction ops #43671

ENH: enable skipna on groupby reduction ops #43671

geoffrey-eisenbarth commented Sep 20, 2021

mzeitlin11 commented Sep 21, 2021

mzeitlin11 left a comment

mzeitlin11 Sep 21, 2021

mzeitlin11 Sep 21, 2021

mzeitlin11 Sep 21, 2021

mzeitlin11 Sep 21, 2021

jbrockmendel Sep 21, 2021

geoffrey-eisenbarth commented Sep 27, 2021

mzeitlin11 commented Sep 28, 2021

github-actions bot commented Oct 29, 2021

jbrockmendel commented Nov 1, 2021

geoffrey-eisenbarth commented Nov 1, 2021

jbrockmendel commented Nov 1, 2021

pep8speaks commented Nov 2, 2021

jreback commented Jan 16, 2022

ENH: enable skipna on groupby reduction ops #43671

ENH: enable skipna on groupby reduction ops #43671

Conversation

geoffrey-eisenbarth commented Sep 20, 2021

mzeitlin11 commented Sep 21, 2021

mzeitlin11 left a comment

Choose a reason for hiding this comment

mzeitlin11 Sep 21, 2021

Choose a reason for hiding this comment

mzeitlin11 Sep 21, 2021

Choose a reason for hiding this comment

mzeitlin11 Sep 21, 2021

Choose a reason for hiding this comment

mzeitlin11 Sep 21, 2021

Choose a reason for hiding this comment

jbrockmendel Sep 21, 2021

Choose a reason for hiding this comment

geoffrey-eisenbarth commented Sep 27, 2021

mzeitlin11 commented Sep 28, 2021

github-actions bot commented Oct 29, 2021

jbrockmendel commented Nov 1, 2021

geoffrey-eisenbarth commented Nov 1, 2021

jbrockmendel commented Nov 1, 2021

pep8speaks commented Nov 2, 2021

jreback commented Jan 16, 2022