-
Notifications
You must be signed in to change notification settings - Fork 908
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
Deprecate cudf.isclose #17351
base: branch-25.02
Are you sure you want to change the base?
Deprecate cudf.isclose #17351
Conversation
@@ -5,17 +5,17 @@ | |||
"args": { | |||
"CUDA": "11.8", | |||
"PYTHON_PACKAGE_MANAGER": "conda", | |||
"BASE": "rapidsai/devcontainers:24.12-cpp-cuda11.8-mambaforge-ubuntu22.04" | |||
"BASE": "rapidsai/devcontainers:25.02-cpp-cuda11.8-mambaforge-ubuntu22.04" |
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 PR targets 24.12. Why have all the dependencies been changed to 25.02?
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.
The PR's title, name, description, and actual changes do not make sense to me.
Recommend closing this.
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 appears to be an attempt at #13593, an issue I filed a while back. @the-gates-of-Zion I can help work with you on this, if you are interested. This should target branch-25.02
, so I will retarget the PR.
@@ -5322,6 +5322,29 @@ def isclose(a, b, rtol=1e-05, atol=1e-08, equal_nan=False): | |||
5 False | |||
dtype: bool | |||
""" | |||
warnings.warn( | |||
"`cudf.close` is deprecated and will be removed in a future version of cudf. " |
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.
"`cudf.close` is deprecated and will be removed in a future version of cudf. " | |
"`cudf.isclose` is deprecated and will be removed in a future version of cudf. " |
''' | ||
import cupy as cp | ||
import pandas as pd | ||
from cudf.core.column import ( | ||
as_column, | ||
) | ||
|
||
a = pd.array([1.0, 2.0, None]) | ||
b = pd.array([1.0, 2.1, None]) | ||
|
||
a_col = as_column(a) | ||
a_array = cupy.asarray(a_col.data_array_view(mode="read")) | ||
|
||
b_col = as_column(b) | ||
b_array = cupy.asarray(b_col.data_array_view(mode="read")) | ||
|
||
result = cp.isclose(a, b, equal_nan=True) | ||
print(result) # Output: [ True False True] | ||
''', |
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.
We noted in #13593: "As part of the deprecation, we decided to add a warning message indicating how to support nulls while using cupy.isclose." However, I am not sure if this is the right code snippet.
The code we want to give to the user as a replacement for this deprecated code path is essentially the current implementation in this method. Basically it has two steps:
- Call
cupy.isclose
to see if the values are "close" to each other. This does not account for null values in the input. - Compare the null masks of each input. If
equal_nan
isFalse
, we mark all null values asFalse
("not close") in the result. Ifequal_nan
isTrue
, we set the values where one column's input is null asFalse
and the values where both columns' input are null asTrue
.
I would suggest we add this snippet to the page "Working with missing data": https://docs.rapids.ai/api/cudf/stable/user_guide/missing-data/
Then we can link to this snippet in the docs from the deprecated method's docstring and in the deprecation warning.
Does that make sense? Let me know if you have questions.
Description
add warnings.warn("
cudf.close
is deprecated and will be removed in a future version of cudf. ", ''' ... ''')(from 25.02)
Checklist