Add Dataframe and Index nunique #10077

martinfalisse · 2022-01-19T19:09:55Z

Add Dataframe and Index nunique. Resolves #9611

GPUtester · 2022-01-19T19:09:57Z

Can one of the admins verify this patch?

vyasr · 2022-01-19T23:33:41Z

Hi @martinfalisse thanks for the contribution! Could you retarget this PR to 22.04? We are no longer accepting new changes on 22.02 since we're in the process of finalizing a release.

martinfalisse · 2022-01-20T09:58:14Z

Hi @martinfalisse thanks for the contribution! Could you retarget this PR to 22.04? We are no longer accepting new changes on 22.02 since we're in the process of finalizing a release.

Sure, sorry about that, should be fixed now.

python/cudf/cudf/core/dataframe.py

python/cudf/cudf/core/single_column_frame.py

Co-authored-by: Ashwin Srinath <3190405+shwina@users.noreply.github.com>

shwina

This looks great! Thank you @martinfalisse

beckernick · 2022-01-31T14:39:53Z

Thanks for the contribution @martinfalisse !

May I ask how you decided to work on #9611 vs another issue?

martinfalisse · 2022-01-31T16:00:39Z

Thanks for the contribution @martinfalisse !

May I ask how you decided to work on #9611 vs another issue?

Sure! I searched for the issues under the category 'good first issue', and then picked one that I felt I could implement. This specific issue seemed to only require putting some things together on the python side which is why I chose it.

Hope to contribute more in the future!

vyasr

This is almost there! Thanks for the work, the changes are looking good.

python/cudf/cudf/core/series.py

python/cudf/cudf/core/single_column_frame.py

python/cudf/cudf/tests/test_dataframe.py

ajschmidt8 · 2022-02-01T03:59:33Z

ok to test

vyasr · 2022-02-02T20:06:42Z

@martinfalisse you're almost there! The code looks great now, but the style checks are failing. The easiest way for you to fix this is to use pre-commit. If you haven't used it before, you can pip install pre-commit, then run pre-commit run --all-files inside the cudf repo. It should automatically fix as many issues as it can, and you can read the other errors to see what's failing. From a quick look it does appear that fixing the outstanding issues will require some manual intervention, so feel free to ping back here if you have trouble addressing them!

As an aside, you can run pre-commit install inside the repo once you have pre-commit on your machine and it will install git hooks that will run our style checks every time you try to commit. That will ensure that every commit you make will pass our tests.

vyasr

LGTM! Thanks for the great work.

For future reference, please don't force push changes once reviews have started. It can erase enough history to make comments vanish and cause other problems. It's easier to just add commits, and we squash all commits when a PR is merged anyway so you don't need to worry about keeping the history clean.

codecov · 2022-02-02T23:33:25Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.04@b217d7e). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.04   #10077   +/-   ##
===============================================
  Coverage                ?   10.47%           
===============================================
  Files                   ?      122           
  Lines                   ?    20506           
  Branches                ?        0           
===============================================
  Hits                    ?     2148           
  Misses                  ?    18358           
  Partials                ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b217d7e...bcd84ad. Read the comment docs.

vyasr · 2022-02-02T23:50:19Z

@gpucibot merge

@vyasr

Removes unnecessary nunique function in Series, as indicated by @vyasr here #10077 (comment). Seem to have made a mistake when correcting the style of my last commit and not have kept the original changes of that commit. Authors: - https://github.com/martinfalisse Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: #10205

…0411) The APIs `DataFrame.nunique`, `Series.nunique`, and `ColumnBase.distinct_count` have a `method` argument. The pandas API for `nunique` [does not have a `method` argument](https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.nunique.html). This `method` argument was meant to distinguish distinct counts computed by sort/hash approaches, but there are two issues: only `"sort"` is supported as an input value, and the algorithm in libcudf actually uses a _hash-based approach_. To resolve this inconsistency and align with the pandas API, I have removed the `method` parameter from `DataFrame.nunique`, `Series.nunique`, and `ColumnBase.distinct_count`. This is a breaking change but I don't think a deprecation is needed. The `nunique` feature was added in 22.04 via #10077, so it has not yet been released. The internal changes to `distinct_count` can be made without a deprecation. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) URL: #10411

martinfalisse requested a review from a team as a code owner January 19, 2022 19:09

martinfalisse requested review from marlenezw and skirui-source January 19, 2022 19:09

github-actions bot added the Python Affects Python cuDF API. label Jan 19, 2022

martinfalisse changed the title ~~Add Dataframe and Index nunique~~ Add Dataframe and Index nunique :improvement :non-breaking Jan 19, 2022

martinfalisse changed the title ~~Add Dataframe and Index nunique :improvement :non-breaking~~ Add Dataframe and Index nunique Jan 19, 2022

vyasr assigned martinfalisse Jan 19, 2022

vyasr added 3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change labels Jan 19, 2022

martinfalisse changed the base branch from branch-22.02 to branch-22.04 January 20, 2022 07:26

vyasr requested changes Jan 26, 2022

View reviewed changes

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved

Add Dataframe and Index nunique

7291d3a

martinfalisse force-pushed the nunique branch from 232c112 to 7291d3a Compare January 27, 2022 11:29

shwina reviewed Jan 27, 2022

View reviewed changes

python/cudf/cudf/core/single_column_frame.py Outdated Show resolved Hide resolved

Use distinct_count instead of super().nunique in single_column_frame

90c2c0e

Co-authored-by: Ashwin Srinath <3190405+shwina@users.noreply.github.com>

martinfalisse requested a review from vyasr January 31, 2022 11:59

shwina approved these changes Jan 31, 2022

View reviewed changes

Merge branch 'branch-22.04' into nunique

a4f1268

vyasr requested changes Jan 31, 2022

View reviewed changes

python/cudf/cudf/core/series.py Show resolved Hide resolved

python/cudf/cudf/core/single_column_frame.py Show resolved Hide resolved

python/cudf/cudf/tests/test_dataframe.py Outdated Show resolved Hide resolved

Remove unnecessary nunique function in series.

bcd84ad

martinfalisse force-pushed the nunique branch from fcb37ba to bcd84ad Compare February 2, 2022 21:44

vyasr approved these changes Feb 2, 2022

View reviewed changes

rapids-bot bot merged commit b7257a3 into rapidsai:branch-22.04 Feb 2, 2022

martinfalisse mentioned this pull request Feb 3, 2022

Remove unnecessary nunique function in Series. #10205

Merged

bdice mentioned this pull request Mar 11, 2022

Drop unsupported method argument from nunique and distinct_count. #10411

Merged

shwina mentioned this pull request Mar 23, 2022

[DOC] RAPIDS 22.04 Release Blog Outline #10383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Dataframe and Index nunique #10077

Add Dataframe and Index nunique #10077

martinfalisse commented Jan 19, 2022 •

edited by vyasr

Loading

GPUtester commented Jan 19, 2022

vyasr commented Jan 19, 2022

martinfalisse commented Jan 20, 2022 •

edited

Loading

shwina left a comment

beckernick commented Jan 31, 2022

martinfalisse commented Jan 31, 2022

vyasr left a comment

ajschmidt8 commented Feb 1, 2022

vyasr commented Feb 2, 2022

vyasr left a comment

codecov bot commented Feb 2, 2022 •

edited

Loading

vyasr commented Feb 2, 2022

Add Dataframe and Index nunique #10077

Add Dataframe and Index nunique #10077

Conversation

martinfalisse commented Jan 19, 2022 • edited by vyasr Loading

GPUtester commented Jan 19, 2022

vyasr commented Jan 19, 2022

martinfalisse commented Jan 20, 2022 • edited Loading

shwina left a comment

Choose a reason for hiding this comment

beckernick commented Jan 31, 2022

martinfalisse commented Jan 31, 2022

vyasr left a comment

Choose a reason for hiding this comment

ajschmidt8 commented Feb 1, 2022

vyasr commented Feb 2, 2022

vyasr left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 2, 2022 • edited Loading

Codecov Report

vyasr commented Feb 2, 2022

martinfalisse commented Jan 19, 2022 •

edited by vyasr

Loading

martinfalisse commented Jan 20, 2022 •

edited

Loading

codecov bot commented Feb 2, 2022 •

edited

Loading