Define Column.nan_as_null to return self #15923
Conversation
mroeschke
added
Python
| @@ -1247,6 +1249,10 @@ def unary_operator(self, unaryop: str): | |||
| f"Operation {unaryop} not supported for dtype {self.dtype}." | |||
| ) | |||
|
|
|||
| def nans_to_nulls(self: Self) -> Self: | |||
| """Convert NaN to NA.""" | |||
| return self | |||
There was a problem hiding this comment.
question: If the column is a floating point one, nans_to_nulls might (always?) produce a copy. Here, we would share data. Is that problematic for downstream consumers who might implicitly assume that nans_to_nulls copies?
I see you handle this explicitly in the one case it is necessary below.
There was a problem hiding this comment.
Yeah I was wrestling back and forth between whether this should copy or not.
One one hand, as you mentioned, there was only 1 API where we needed to guarantee the result didn't share data.
For most other cases, we wanted this to no-op in the cases where no nan conversions would happen.
So I see it as avoid unnecessary copies by default vs the caller guards against calling nan_as_null if there are no nans to avoid unnecessary copies. WDYT?
There was a problem hiding this comment.
For most other cases, we wanted this to no-op in the cases where no nan conversions would happen.
For this reason, I feel it's okay not to create copies and generate more memory pressure for this API.
| @@ -702,7 +702,9 @@ def fillna( | |||
| Returns a copy with null filled. | |||
| """ | |||
| return libcudf.replace.replace_nulls( | |||
| input_col=self, replacement=fill_value, method=method | |||
| input_col=self.nans_to_nulls(), | |||
There was a problem hiding this comment.
I think you fixed a bug that is resulting in an increase in test pass rate:
Would you be able to add a test having a mix of nan's & NA's performing a to_arrow ? Just wanting to make sure we have it captured in pytest so that we know if it breaks/someone changes this code unintentionally.
There was a problem hiding this comment.
Would you be able to add a test having a mix of nan's & NA's performing a to_arrow ?
I assume you meant fillna instead of to_arrow? If so, I added that test in 0d74275
|
/merge |
Description
While trying to clean all the
fillnalogic, I needed to have aColumn.nan_as_nulldefined to make thefillnalogic more re-useable.This allows other
nan_as_nullusages in cudf to avoiding checking whether it's defined on the column or not.Checklist