Skip to content
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.

Sign up for GitHub

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

Fix nan_as_null not being respected when passing cudf object #14687

Closed
wants to merge 13 commits into from
Closed

Fix nan_as_null not being respected when passing cudf object #14687

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
46a61c3
Fix nan_as_null not being respected when passing cudf object
mroeschke Dec 29, 2023
324376e
Merge remote-tracking branch 'upstream/branch-24.02' into bug/constr/…
mroeschke Jan 9, 2024
45bb9e3
add year
mroeschke Jan 9, 2024
65cbaa1
Merge remote-tracking branch 'upstream/branch-24.02' into bug/constr/…
mroeschke Jan 9, 2024
64b5a32
Merge remote-tracking branch 'upstream/branch-24.02' into bug/constr/…
mroeschke Jan 10, 2024
b569b62
test concat with nan_as_null=False
mroeschke Jan 10, 2024
f3cf614
Merge remote-tracking branch 'upstream/branch-24.02' into bug/constr/…
mroeschke Jan 11, 2024
5289adf
Merge remote-tracking branch 'upstream/branch-24.02' into bug/constr/…
mroeschke Jan 12, 2024
8855f40
Adjust some tests
mroeschke Jan 12, 2024
79c318f
Make as_column nan_as_null=False by default
mroeschke Jan 12, 2024
4c7e3a5
Use _from_data
mroeschke Jan 12, 2024
9456db9
Merge remote-tracking branch 'upstream/branch-24.02' into bug/constr/…
mroeschke Jan 12, 2024
302df52
Revert test_dlpack
mroeschke Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions python/cudf/cudf/core/column/column.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright (c) 2018-2023, NVIDIA CORPORATION.
# Copyright (c) 2018-2024, NVIDIA CORPORATION.

from __future__ import annotations

Expand Down Expand Up @@ -1939,20 +1939,18 @@ def as_column(
* pyarrow array
* pandas.Categorical objects
"""
if isinstance(arbitrary, ColumnBase):
if dtype is not None:
return arbitrary.astype(dtype)
if isinstance(arbitrary, (ColumnBase, cudf.Series, cudf.BaseIndex)):
if isinstance(arbitrary, cudf.Series):
column = arbitrary._column
elif isinstance(arbitrary, cudf.BaseIndex):
column = arbitrary._values
else:
return arbitrary

elif isinstance(arbitrary, cudf.Series):
data = arbitrary._column
column = arbitrary
if column.dtype.kind == "f" and (nan_as_null is None or nan_as_null):
column = column.nans_to_nulls()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually I tend to prefer some parameter handling at the start of the function, such as:

if nan_as_null is None:
    nan_as_null = True

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mroeschke - The CI failure is a result of us hitting this check when we call compute() in the dask-cudf test.

When a dask collection is "computed", the partitions need to be concatenated together. It seems that calling cudf.concat uses the cudf.Series._concat, which in turn calls this as_column function with a default nan_as_null=True setting. This means that dask-cudf correctly uses the user-provided nan_as_null=False option when each partition is converted from pandas to cudf, but then the nans are lost when the partitions are concatenated together in compute.

One possible fix may be to modify this line to something like:

 return cls(data=col, index=index, name=name, nan_as_null=False)

However, I'm not immediately sure if there are other corner cases that this doesn't catch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I suppose the best way to test this outside of dask is to add a cudf.concat test that checks that existing nan values are preserved. In the case that there are both nan and null values, then we would probably need to raise an error or "promote" everything to null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the insight! The Series._concat piece is what I was missing. I am going to see what breaks if nan_as_null is set to False. In theory, I would imagine nans and nulls should not be mangled together when merged together

if dtype is not None:
data = data.astype(dtype)
elif isinstance(arbitrary, cudf.BaseIndex):
data = arbitrary._values
if dtype is not None:
data = data.astype(dtype)
column = column.astype(dtype)
return column

elif hasattr(arbitrary, "__cuda_array_interface__"):
desc = arbitrary.__cuda_array_interface__
Expand Down
8 changes: 8 additions & 0 deletions python/cudf/cudf/tests/test_series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2606,6 +2606,14 @@ def test_series_error_nan_non_float_dtypes():
s[0] = np.nan


@pytest.mark.parametrize("klass", [cudf.Index, cudf.Series])
def test_nan_as_null_from_cudf_objects(klass):
data = klass(pa.array([float("nan")]))
result = klass(data, nan_as_null=True)
expected = klass(pa.array([None], type=pa.float64()))
assert_eq(result, expected)


@pytest.mark.parametrize(
"dtype",
[
Expand Down
Loading