Remove unneeded methods in Column - #14730
Merged
rapids-bot[bot] merged 5 commits intoJan 10, 2024
Merged
Conversation
mroeschke
added
Python
vyasr
approved these changes
Jan 10, 2024
vyasr
left a comment
vyasr
left a comment
Contributor
There was a problem hiding this comment.
Thanks! These kinds of simplifications to the API will definitely help ease the pylibcudf transition!
Contributor
Author
|
/merge |
wence-
reviewed
Jan 11, 2024
wence-
left a comment
wence-
left a comment
Contributor
There was a problem hiding this comment.
Sorry, I failed to flush this before merge. Most of the comments are minor but there was one logic mistake in moving from valid_count to null_count, will fix.
| self, | ||
| label, | ||
| side: Literal["left", "right"], | ||
| kind: Literal["ix", "loc", "getitem", None] = None, |
Contributor
There was a problem hiding this comment.
nit Optional[Literal["ix", "loc", "getitem"]] ?
| # Find the first non-null column: | ||
| head = next((obj for obj in objs if obj.valid_count), objs[0]) | ||
| head = next( | ||
| (obj for obj in objs if not obj.null_count != len(obj)), objs[0] |
Contributor
There was a problem hiding this comment.
issue I think this logic is wrong. The old code was equivalent to:
(obj for obj in objs if (len(obj) - obj.null_count != 0))
Rearranging the condition:
len(obj) != obj.null_count
So we've gained an extra negation.
Suggested change
| (obj for obj in objs if not obj.null_count != len(obj)), objs[0] | |
| (obj for obj in objs if obj.null_count != len(obj)), objs[0] |
Comment on lines
+143
to
146
| def has_nulls(self, include_nan: bool = False) -> bool: | ||
| return bool(self.null_count != 0) or ( | ||
| include_nan and bool(self.nan_count != 0) | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggested change
| def has_nulls(self, include_nan: bool = False) -> bool: | |
| return bool(self.null_count != 0) or ( | |
| include_nan and bool(self.nan_count != 0) | |
| ) | |
| def has_nulls(self, include_nan: bool = False) -> bool: | |
| return self.null_count != 0 or (include_nan and self.nan_count != 0) |
Comment on lines
1710
to
+1713
| @_cudf_nvtx_annotate | ||
| def valid_count(self): | ||
| """Number of non-null values""" | ||
| return self._column.valid_count | ||
| return len(self) - self._column.null_count |
Contributor
There was a problem hiding this comment.
This is not here for API compat (pandas series do not have a valid_count method), could we deprecate it at the series level too?
wence-
added a commit
to wence-/cudf
that referenced
this pull request
Jan 11, 2024
3 tasks
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jan 12, 2024
The removal of `valid_count` on columns in #14730 had one logic bug, fixed here. Authors: - Lawrence Mitchell (https://github.com/wence-) Approvers: - Matthew Roeschke (https://github.com/mroeschke) - Vyas Ramasubramani (https://github.com/vyasr) URL: #14742
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
valid_countcan be composed ofnull_countor where checkedhas_nullscontains_na_entriesis redundant withhas_nullssearchsortedChecklist