Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add groupby scan aggregation to cudf #7759

Merged
merged 72 commits into from
Apr 22, 2021
Merged

Add groupby scan aggregation to cudf #7759

merged 72 commits into from
Apr 22, 2021

Conversation

karthikeyann
Copy link
Contributor

@karthikeyann karthikeyann commented Mar 30, 2021 •
edited
Loading

closes #1296 Groupby cumulative count
closes #1298 Groupby cumulative sum

  • Add cython code for groupby scan (cannot mix reduce aggs and scan aggs)
  • Add python code for groupby scan functions - cumsum, cummin, cummax, cumcount, groupby.agg()
  • unit tests

vyasr and others added 30 commits March 24, 2021 12:29
@vyasr
Don't identify decimals as strings.
26bafd0
@vyasr
Reject all extension types as string types.
babcdfc
@vyasr
Create separate lists for extension type methods.
2b00611
@vyasr
Merge branch 'branch-0.19' into fix/issue7687_part2
76ab556
@vyasr
Enable collect for decimals.
1ebde51
@vyasr
Enable argmin and argmax.
4c5d876
@vyasr
Fix variance key name.
4134e43
@vyasr
Move groupby aggregation list to groupby.py and clean up the assignme…
43cf580 
…nt of aggregators.
@vyasr
Disable aggs that are overrides of actual methods.
474a179
@vyasr
Move more logic out of the GroupBy class.
25e74ef
@vyasr
Simplify getattr usage.
8a44827
@vyasr
Clearly documented unknown failures.
6b5c67f
@vyasr
Match other class groupbys to strings.
8e45ad0
@vyasr
Fix style and remove unsupported operations.
81ffe0a
@vyasr
Apply black reformattings.
6d3fad3
@vyasr
Remove variance from obviously unsupported types.
714742d
@vyasr
Defer getattr to getitem if possible.
ea4ed2e
@vyasr
Make getattr safe for copying.
026bb4e
@vyasr
Remove support for aggregating structs.
1259032
@vyasr
Update documented list of groupby operations.
6c61806
@vyasr
Move function out of loop.
a14d30f
@vyasr
Merge branch 'branch-0.19' into fix/issue7687_part2
12caa06
@vyasr
Remove redundant test, add test of decimal.
5c71bfe
@vyasr
Fix formatting.
25811b0
@vyasr
Merge branch 'branch-0.19' into fix/issue7687_part2
1450f2d
@vyasr
Simplify drop aggs logic.
4a0f27d
@vyasr
Inline drop logic.
596496f
@vyasr
Remove empty aggregations in place rather than beforehand.
1f5dadd
@vyasr
Reuse aggregation object.
f02231f
@karthikeyann
add cumsum, cummin, cummax, cumcount to Aggregation
5835f56
@vyasr vyasr mentioned this pull request Apr 13, 2021
Merged
vyasr
vyasr requested changes Apr 16, 2021
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this, was waiting until the changes from #7818 could be merged in. I think we can simplify this code a little and make it match the C++ internals by mapping cumulative operations to their non-cumulative counterparts earlier, but I've left a couple of comments to ascertain that this won't cause significant problems when we start supporting mixed scan/aggregate operations.

python/cudf/cudf/_lib/groupby.pyx Show resolved Hide resolved
python/cudf/cudf/_lib/groupby.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/aggregation.pyx Show resolved Hide resolved
python/cudf/cudf/_lib/groupby.pyx Show resolved Hide resolved
python/cudf/cudf/tests/test_groupby.py
Comment on lines +1576 to +1578
# pd.groupby.cumcount returns a series.
if isinstance(expect_df, pd.Series):
expect_df = expect_df.to_frame("val")
Copy link
Contributor

Choose a reason for hiding this comment

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

@shwina did you already write up the issue? If so, this conversation can be resolved.

@karthikeyann karthikeyann requested a review from vyasr April 16, 2021 20:24
vyasr
vyasr approved these changes Apr 16, 2021
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

One or two minor comments from me, otherwise looks ready.

python/cudf/cudf/_lib/groupby.pyx Outdated Show resolved Hide resolved
@kkraus14
Copy link
Collaborator

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 8dae31c into rapidsai:branch-0.20 Apr 22, 2021
@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkraus14 kkraus14 kkraus14 approved these changes

@vyasr vyasr vyasr approved these changes

@rgsl888prabhu rgsl888prabhu Awaiting requested review from rgsl888prabhu

@brandon-b-miller brandon-b-miller Awaiting requested review from brandon-b-miller

@shwina shwina Awaiting requested review from shwina

Assignees

@karthikeyann karthikeyann

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Groupby cumulative sum [FEA] Groupby cumulative count
5 participants
@karthikeyann @kkraus14 @vyasr @shwina @galipremsagar