Skip to content

Conversation

@smithto1
Copy link
Contributor

@smithto1 smithto1 commented Oct 1, 2020

See #34725 for a good description of the problem.

This fixes the issue in the tidiest way I could find.

One test is added that covers all relevant cases I could think of. The comments in the test can be removed, they are just there to make it easier for the reviewer to follow as the test is a bit complicated.

This also fixes the copy-pastable example from the #34725.

@rhshadrach rhshadrach added Bug Groupby Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Oct 2, 2020
@rhshadrach
Copy link
Member

Is there a way to fix group_fillna_indexer rather than correct the results with post-processing? Or is there a reason not to do this?

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with @rhshadrach comment here - this should be fixed directly in the underlying function

@smithto1
Copy link
Contributor Author

smithto1 commented Oct 6, 2020

Is there a way to fix group_fillna_indexer rather than correct the results with post-processing? Or is there a reason not to do this?

@rhshadrach This is fixed in group_fillna_indexer now. It is much cleaner and faster.

@smithto1 smithto1 requested a review from WillAyd October 6, 2020 07:12
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good, two minor requests.

@smithto1
Copy link
Contributor Author

smithto1 commented Oct 7, 2020

@rhshadrach @WillAyd Comments have been addressed. Can you please have another look?

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Can you move tests to tests.groupby.test_missing - ref #36326

@jreback jreback added this to the 1.2 milestone Oct 8, 2020
@smithto1
Copy link
Contributor Author

smithto1 commented Oct 8, 2020

Looks good. Can you move tests to tests.groupby.test_missing - ref #36326

@rhshadrach Done.

@rhshadrach
Copy link
Member

@smithto1 CI failure due to import order in unrelated files; I think merging master will fix.

@smithto1
Copy link
Contributor Author

smithto1 commented Oct 9, 2020

@smithto1 CI failure due to import order in unrelated files; I think merging master will fix.

@rhshadrach I think that is all fixed now.

@rhshadrach
Copy link
Member

@smithto1 Got a new conflict that needs resolved.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm can merge on green.

@jreback jreback merged commit 389be17 into pandas-dev:master Oct 10, 2020
@jreback
Copy link
Contributor

jreback commented Oct 10, 2020

thanks @smithto1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Groupby Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: fillna(method="ffill") and ffill() on DataFrameGroupBy gives different results

4 participants