Remove unneeded methods in Column #14730
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
mroeschke
added
Python
vyasr
approved these changes
Jan 10, 2024
|
/merge |
wence-
reviewed
Jan 11, 2024
| self, | ||
| label, | ||
| side: Literal["left", "right"], | ||
| kind: Literal["ix", "loc", "getitem", None] = None, |
There was a problem hiding this comment.
nit Optional[Literal["ix", "loc", "getitem"]] ?
| @@ -1381,7 +1381,9 @@ def _concat( | |||
| # improved as the concatenation API is solidified. | |||
|
|
|||
| # 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] | |||
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) | ||
| ) |
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 |
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
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.
Description
valid_countcan be composed ofnull_countor where checkedhas_nullscontains_na_entriesis redundant withhas_nullssearchsortedChecklist