-
Notifications
You must be signed in to change notification settings - Fork 915
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
[REVIEW] Fix repr and concat of StructColumn
#10042
Conversation
StructColumn
StructColumn
cudf.core.column.Decimal64Column, | ||
cudf.core.column.StructColumn, |
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.
Why are these two types handled the same way? What do they have in common?
Are there other types that might need to be handled here -- and how would developers know to put them here? This may not be something that can be addressed in a code comment, but it seems important to consider. The StructColumn
type was seemingly left out from a place it should have been included, until an issue was raised, cause identified, and bug fixed.
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.
This is a common pattern:
cudf/python/cudf/cudf/core/frame.py
Lines 2154 to 2157 in 813ac97
# TODO: _copy_type_metadata is a common pattern to apply after the | |
# roundtrip from libcudf. We should build this into a factory function | |
# to increase reusability. | |
result._copy_type_metadata(self) |
To answer why only these two types are being handled separately is because _reassign_categories
predates _with_type_metadata
and will likely need a cleanup with some careful benchmarking to eliminate _reassign_categories
which are out of the scope of this PR.
Codecov Report
@@ Coverage Diff @@
## branch-22.02 #10042 +/- ##
================================================
- Coverage 10.49% 10.41% -0.08%
================================================
Files 119 119
Lines 20305 20538 +233
================================================
+ Hits 2130 2139 +9
- Misses 18175 18399 +224
Continue to review full report at Codecov.
|
rerun tests |
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.
I copied the comment from DataFrame._concat
to Series._concat
and committed that suggestion. LGTM!
@gpucibot merge |
@gpucibot merge |
Fixes: #8963
This PR fixes a trivial issue in
concat
where the assumption was that_with_type_metadata
is an in-place operation, but it isn't.