-
Notifications
You must be signed in to change notification settings - Fork 882
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
Refactor ColumnMethods and its subclasses to remove column
argument and require parent
argument
#8306
Refactor ColumnMethods and its subclasses to remove column
argument and require parent
argument
#8306
Conversation
…column-methods-cleanup
Few small questions otherwise lgtm. |
Also might want to target 21.08 |
…column-methods-cleanup
…column-methods-cleanup
Sorry for the delay in updating this PR. I've merged conflicts and addressed your comments @brandon-b-miller |
Codecov Report
@@ Coverage Diff @@
## branch-21.08 #8306 +/- ##
==============================================
Coverage 10.66% 10.66%
==============================================
Files 109 110 +1
Lines 18302 18680 +378
==============================================
+ Hits 1951 1993 +42
- Misses 16351 16687 +336
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. This has some overlapping with my Categorical refactor branch. Should make it a bit easier to progress.
The PR description only captures part of what's going on though - maybe adding two bullet points mentioning the changes to CategoricalAccessor
and StringMethods
?
@@ -2299,7 +2299,62 @@ def full(size: int, fill_value: ScalarLike, dtype: Dtype = None) -> ColumnBase: | |||
return ColumnBase.from_scalar(cudf.Scalar(fill_value, dtype), size) | |||
|
|||
|
|||
def _concat_columns(objs: "MutableSequence[ColumnBase]") -> ColumnBase: | |||
def _copy_type_metadata_from_arrow( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad merge? I thought this was removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, good catch! Fixed, and also changed title
column
argument and require parent
argument
@gpucibot merge |
Part of #8660. Note that the issue is asking for this feature in _dask-cudf_, which this PR does not implement. Depends on: #8306 Authors: - Ashwin Srinath (https://github.com/shwina) Approvers: - https://github.com/brandon-b-miller - Vyas Ramasubramani (https://github.com/vyasr) URL: #8729
Prior to this PR, ColumnMethods takes a
column
argument as well as an optionalparent
argument.When passed only a
column
argument, its methods return Columns. We have made use of this internally to do certain operations on Columns.When passed a
parent
argument, its methods return objects of the type of theparent
. This enables us to use the same class to implement accessor methods for both Series and Index.This PR makes it so that we only accept (and now require) a
parent
argument, simplifying the class and its usage. All instances ofColumnMethods
being used internally have been replaced.Part of the Array refactor.