[REVIEW] Remove **kwargs from string/categorical methods #6750

shwina · 2020-11-12T18:06:37Z

This PR removes **kwargs from the string/categorical accessors where unnecessary, and exposes keyword arguments like inplace to the user directly.

If we want to maintain parity with Pandas APIs for Dask/others using cuDF internally, we can consider using the approach described in #6135, which will automatically raise NotimplementedError when unsupported kwargs are passed.

…m-str-kwargs

GPUtester · 2020-11-12T18:07:13Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

codecov · 2020-11-12T21:50:33Z

Codecov Report

Merging #6750 (4e08bea) into branch-0.18 (f6b16ab) will increase coverage by 0.44%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           branch-0.18    #6750      +/-   ##
===============================================
+ Coverage        81.59%   82.03%   +0.44%     
===============================================
  Files               96       96              
  Lines            15927    16262     +335     
===============================================
+ Hits             12996    13341     +345     
+ Misses            2931     2921      -10

Impacted Files	Coverage Δ
python/cudf/cudf/core/dataframe.py	`91.08% <ø> (+0.12%)`	⬆️
python/cudf/cudf/core/column/categorical.py	`93.33% <100.00%> (+0.28%)`	⬆️
python/cudf/cudf/core/column/column.py	`87.92% <100.00%> (+0.10%)`	⬆️
python/cudf/cudf/core/column/datetime.py	`88.88% <100.00%> (+0.59%)`	⬆️
python/cudf/cudf/core/column/methods.py	`96.00% <100.00%> (-0.43%)`	⬇️
python/cudf/cudf/core/column/numerical.py	`94.53% <100.00%> (ø)`
python/cudf/cudf/core/column/string.py	`86.58% <100.00%> (+0.48%)`	⬆️
python/cudf/cudf/core/column/timedelta.py	`89.53% <100.00%> (+0.31%)`	⬆️
python/cudf/cudf/io/feather.py	`100.00% <0.00%> (ø)`
python/cudf/cudf/comm/serialize.py	`0.00% <0.00%> (ø)`
... and 43 more

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 f6b16ab...4e08bea. Read the comment docs.

…m-str-kwargs

python/cudf/cudf/_lib/string_casting.pyx

python/cudf/cudf/tests/test_dataframe.py

galipremsagar

The changes LGTM ! 🎉 🎉

isVoid

Some general comments and doubts. Otherwise lgtm!

isVoid · 2020-11-18T18:43:36Z

python/cudf/cudf/core/column/categorical.py

-        new_categories = new_categories.astype(common_dtype, copy=False)
-        old_categories = old_categories.astype(common_dtype, copy=False)
+        new_categories = new_categories.astype(common_dtype)
+        old_categories = old_categories.astype(common_dtype)


Why remove the explicit copy parameter here?

Because that parameter ends up never getting used. These lines call ColumnBase.astype(), which in turn calls as_categorical_column. I didn't find the use of copy in either of those

Just a note: Series.astype() will continue to need the copy parameter: https://pandas.pydata.org/docs/reference/api/pandas.Series.astype.html

python/cudf/cudf/core/column/string.py

…m-str-kwargs

shwina · 2020-12-04T17:51:47Z

@kkraus14 Pushed to 0.18 as this is a QOL thing and more important for typing.

python/cudf/cudf/core/column/string.py

kkraus14 · 2020-12-04T18:19:44Z

Looks like there's a few test failures: https://gpuci.gpuopenanalytics.com/blue/organizations/jenkins/rapidsai%2Fgpuci-a100%2Fcudf%2Fprb%2Fcudf-gpu-build/detail/cudf-gpu-build/2213/tests

CHANGELOG.md

…m-str-kwargs

Remove keyword "stops" from call to cudf.core.column.string.slice, which no longer accepts arbitrary keywords. cuDF change introduced in rapidsai/cudf#6750. Authors: - William Hicks <[email protected]> Approvers: - John Zedlewski - Micka URL: #3289

shwina added 4 commits November 9, 2020 16:38

Add DecimalDtype.itemsize

48a94d9

Add DecimalColumn

3264366

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into r…

9a14676

…m-str-kwargs

Remove string/categorical accessor kwargs

7038138

shwina requested a review from a team as a code owner November 12, 2020 18:06

shwina requested review from galipremsagar and isVoid November 12, 2020 18:06

shwina added 2 commits November 12, 2020 13:08

Changelog

b76f88a

Remove itemsize

2f90afa

shwina marked this pull request as draft November 12, 2020 18:10

shwina added 7 commits November 13, 2020 16:59

Removing some **kwargs from CategoricalColumn

662762a

tmp commit

ff23a79

More kwargs related fixes

d6144a9

Fix CI failures

079e69d

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into r…

41f5794

…m-str-kwargs

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into r…

af96a99

…m-str-kwargs

as_numerical_column() still needs **kwargs

b2f2ed9

shwina changed the title ~~[WIP] Remove **kwargs from string/categorical methods~~ [REVIEW] Remove **kwargs from string/categorical methods Nov 17, 2020

shwina marked this pull request as ready for review November 17, 2020 04:16

Remove DecimalColumn from this branch

b713315

galipremsagar reviewed Nov 18, 2020

View reviewed changes

python/cudf/cudf/_lib/string_casting.pyx Show resolved Hide resolved

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

galipremsagar approved these changes Nov 18, 2020

View reviewed changes

isVoid reviewed Nov 18, 2020

View reviewed changes

shwina added 3 commits November 20, 2020 13:44

Remove **kwargs from as_numerical_column()

df3c3ac

Don't pass **kwargs to as_numerical_column

d06b247

Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into r…

2334680

…m-str-kwargs

shwina changed the base branch from branch-0.17 to branch-0.18 December 4, 2020 17:51

kkraus14 added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. labels Dec 4, 2020

kkraus14 approved these changes Dec 4, 2020

View reviewed changes

python/cudf/cudf/core/column/string.py Outdated Show resolved Hide resolved

shwina added 2 commits December 4, 2020 13:26

Pass expand through to return_or_inplace

32bf4a2

Trailing comma

b71b203

kkraus14 added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change 5 - Ready to Merge Testing and reviews complete, ready to merge 6 - Okay to Auto-Merge and removed 3 - Ready for Review Ready for review by team labels Dec 4, 2020

kkraus14 requested changes Dec 4, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

shwina added 2 commits December 8, 2020 12:16

Merge branch 'branch-0.18' of https://github.com/rapidsai/cudf into r…

37c8227

…m-str-kwargs

Changelog

4e08bea

kkraus14 approved these changes Dec 8, 2020

View reviewed changes

rapids-bot bot merged commit 8a1a6d7 into rapidsai:branch-0.18 Dec 8, 2020

This was referenced Dec 9, 2020

Remove unused keyword in PorterStemmer code rapidsai/cuml#3288

Closed

[REVIEW] Remove unused keyword in PorterStemmer code rapidsai/cuml#3289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Remove **kwargs from string/categorical methods #6750

[REVIEW] Remove **kwargs from string/categorical methods #6750

shwina commented Nov 12, 2020 •

edited

Loading

GPUtester commented Nov 12, 2020

codecov bot commented Nov 12, 2020 •

edited

Loading

galipremsagar left a comment

isVoid left a comment

isVoid Nov 18, 2020

shwina Nov 20, 2020

kkraus14 Dec 4, 2020

shwina commented Dec 4, 2020

kkraus14 commented Dec 4, 2020

[REVIEW] Remove **kwargs from string/categorical methods #6750

[REVIEW] Remove **kwargs from string/categorical methods #6750

Conversation

shwina commented Nov 12, 2020 • edited Loading

GPUtester commented Nov 12, 2020

codecov bot commented Nov 12, 2020 • edited Loading

Codecov Report

galipremsagar left a comment

Choose a reason for hiding this comment

isVoid left a comment

Choose a reason for hiding this comment

isVoid Nov 18, 2020

Choose a reason for hiding this comment

shwina Nov 20, 2020

Choose a reason for hiding this comment

kkraus14 Dec 4, 2020

Choose a reason for hiding this comment

shwina commented Dec 4, 2020

kkraus14 commented Dec 4, 2020

shwina commented Nov 12, 2020 •

edited

Loading

codecov bot commented Nov 12, 2020 •

edited

Loading