Refactoring column logic Part 1 #8081
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…hould be generic into ColumnBase.
vyasr
added
3 - Ready for Review
shwina
reviewed
Apr 27, 2021
kkraus14
requested changes
Apr 27, 2021
Codecov Report
@@ Coverage Diff @@
## branch-0.20 #8081 +/- ##
===============================================
- Coverage 82.88% 82.87% -0.02%
===============================================
Files 103 103
Lines 17668 17835 +167
===============================================
+ Hits 14645 14781 +136
- Misses 3023 3054 +31
Continue to review full report at Codecov.
|
kkraus14
reviewed
Apr 28, 2021
kkraus14
approved these changes
Apr 28, 2021
|
@gpucibot merge |
Merged
rapids-bot bot
pushed a commit
that referenced
this pull request
May 17, 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. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Keith Kraus (https://github.com/kkraus14) - Ashwin Srinath (https://github.com/shwina) - Michael Wang (https://github.com/isVoid) URL: #8130
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a first pass at refactoring
ColumnBaseand its subclasses to reduce redundancy and improve performance by avoiding runtime type checking. Many functions are implemented in the top-level class but dispatch on dtype, which can instead be accomplished via ducktyping. Additionally, other parts ofcudfrequire various methods to be implemented by a column, butColumnBasedoes not currently clearly delineate an interface, making it difficult to know what to rely on in classes likeFrameand pushing dynamic type dispatch upstream in the call stack where it is even less efficient and causes substantial code duplication. This PR moves specialized implementations of certain methods into the appropriate subclasses ofColumnBaseand establishes a base API in the parent class where appropriate. Since this change will be large, I plan to split it into a few different PRs. This PR primarily modifiesto_pandas,to_arrow, and__cuda_array_interface__, along with a few other minor improvements.