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.

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

Column refactoring 2 #8130

Merged

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Apr 30, 2021

Follow up to #8081. This PR 1) inlines the binary op functions in different columns (which I presume were mainly created for profiling with nvtx), 2) moves all categorical column concatenation logic into the CategoricalColumn class, which allows the deletion of certain unimplemented functions in ColumnBase that only existed to satisfy mypy, and 3) performs some cleanup in the form of removing unused properties and using the cached_property decorator where appropriate.

@vyasr vyasr requested a review from a team as a code owner April 30, 2021 22:01
@github-actions github-actions bot added the Python Affects Python cuDF API. label Apr 30, 2021
@vyasr vyasr self-assigned this Apr 30, 2021
@vyasr vyasr added 3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function tech debt non-breaking Non-breaking change labels Apr 30, 2021
@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.06    #8130   +/-   ##
===============================================
  Coverage                ?   82.85%           
===============================================
  Files                   ?      105           
  Lines                   ?    17891           
  Branches                ?        0           
===============================================
  Hits                    ?    14823           
  Misses                  ?     3068           
  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 92a432d...49ca36a. Read the comment docs.

Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Minor comments, these extra helpers for binop bugged me for a while, nice work getting rid of them.

python/cudf/cudf/core/column/categorical.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/column/categorical.py Outdated Show resolved Hide resolved
python/cudf/cudf/core/column/column.py Show resolved Hide resolved
python/cudf/cudf/tests/test_string.py Show resolved Hide resolved
@vyasr vyasr requested review from shwina, kkraus14 and isVoid May 14, 2021 21:32
@kkraus14
Copy link
Collaborator

Will let @isVoid and @shwina take another pass at reviewing before we merge.

@vyasr vyasr changed the title Column refactoring 2 Column refac toring 2 May 17, 2021
@vyasr vyasr changed the title Column refac toring 2 Column refactoring 2 May 17, 2021
@kkraus14
Copy link
Collaborator

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 65e8372 into rapidsai:branch-21.06 May 17, 2021
@vyasr vyasr added this to the cuDF Python Refactoring milestone Jul 22, 2021
@vyasr vyasr deleted the refactor/reduce_column_duplication2 branch January 14, 2022 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants