Fix to_numpy object null handling - #22621
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an internal is_numpy_object_dtype helper and refines Frame._to_array null-filling so converting to NumPy object dtype preserves Python None when no explicit na_value is provided; adds tests validating preservation and na_value replacement for DataFrame and Series. ChangesObject dtype null preservation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cudf/cudf/core/frame.py (1)
645-791:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd a regression benchmark for the new object-dtype null path in
Frame._to_array.This behavior change is in a frequently used conversion path; please add a unit benchmark to guard against regressions for
to_numpy(dtype=object)with null string/object data (with and without explicitna_value).As per coding guidelines, "
**/*.{cpp,cu,py,pyx}: Add unit tests and unit benchmarks for code contributions".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf/cudf/core/frame.py` around lines 645 - 791, Add a regression benchmark and unit test exercising the new object-dtype null path in Frame._to_array (used by to_numpy/to_cupy) for conversions to dtype=object: create tests that call DataFrame/Series.to_numpy(dtype=object) and DataFrame/Series._to_array via to_numpy equivalence with string/object columns containing nulls, covering both with and without an explicit na_value, and assert performance/regression expectations and correctness of null boxing (cudf.NA or provided na_value). Place the benchmark alongside existing Python microbenchmarks (matching the repo pattern **/*.{py}) and add a unit test ensuring behavior (including the branch where is_numpy_object_dtype and the null-handling assignment code paths run), referencing Frame._to_array, to_array, and the to_numpy entrypoint so CI will catch regressions.
🧹 Nitpick comments (1)
python/cudf/cudf/tests/dataframe/methods/test_to_cupy.py (1)
200-218: ⚡ Quick winBroaden edge-case coverage for object-dtype null handling tests.
Please add cases for empty input, all-null input, and single-element input for both
DataFrameandSeriesso the newdtype=objectnull behavior is fully pinned down.As per coding guidelines, "
python/**/test_*.py: Ensure test files provide comprehensive edge case coverage (empty, all-null, single-element, mixed types) and do not depend on external datasets".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cudf/cudf/tests/dataframe/methods/test_to_cupy.py` around lines 200 - 218, Expand the test function test_to_numpy_object_dtype_preserves_none_string_nulls to include edge-case inputs (empty, all-null, and single-element) for both constructors ("DataFrame" and "Series"): create pd_obj variants for each case, convert to cudf_obj via getattr(cudf, constructor)(pd_obj), then run the same assertions (dtype==np.dtype("O"), np.testing.assert_array_equal between cudf_obj.to_numpy(...) and pd_obj.to_numpy(...), and checks for None or provided na_value) for both dtype=object and dtype=object with na_value="missing" so empty arrays, all-null arrays, and single-element arrays are covered for both DataFrame and Series.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@python/cudf/cudf/core/frame.py`:
- Around line 645-791: Add a regression benchmark and unit test exercising the
new object-dtype null path in Frame._to_array (used by to_numpy/to_cupy) for
conversions to dtype=object: create tests that call
DataFrame/Series.to_numpy(dtype=object) and DataFrame/Series._to_array via
to_numpy equivalence with string/object columns containing nulls, covering both
with and without an explicit na_value, and assert performance/regression
expectations and correctness of null boxing (cudf.NA or provided na_value).
Place the benchmark alongside existing Python microbenchmarks (matching the repo
pattern **/*.{py}) and add a unit test ensuring behavior (including the branch
where is_numpy_object_dtype and the null-handling assignment code paths run),
referencing Frame._to_array, to_array, and the to_numpy entrypoint so CI will
catch regressions.
---
Nitpick comments:
In `@python/cudf/cudf/tests/dataframe/methods/test_to_cupy.py`:
- Around line 200-218: Expand the test function
test_to_numpy_object_dtype_preserves_none_string_nulls to include edge-case
inputs (empty, all-null, and single-element) for both constructors ("DataFrame"
and "Series"): create pd_obj variants for each case, convert to cudf_obj via
getattr(cudf, constructor)(pd_obj), then run the same assertions
(dtype==np.dtype("O"), np.testing.assert_array_equal between
cudf_obj.to_numpy(...) and pd_obj.to_numpy(...), and checks for None or provided
na_value) for both dtype=object and dtype=object with na_value="missing" so
empty arrays, all-null arrays, and single-element arrays are covered for both
DataFrame and Series.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a6f2063c-a988-427c-bff3-4227aacd760f
📒 Files selected for processing (2)
python/cudf/cudf/core/frame.pypython/cudf/cudf/tests/dataframe/methods/test_to_cupy.py
430afe4 to
3692539
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cudf/cudf/core/frame.py`:
- Around line 656-660: Add a regression benchmark that measures the hot
conversion path exercised by Frame._to_array / DataFrame.to_numpy(dtype=object)
when nulls are present: create a pytest-benchmark (or project-standard
benchmarking harness) test that constructs frames with nulls and calls
to_numpy(dtype=object) twice—once using the default na_value and once passing an
explicit na_value—to capture performance for both paths; name the test clearly
(e.g., bench_to_numpy_object_with_nulls) and place it with other Python
benchmarks so future changes to is_numpy_object_dtype / Frame._to_array will be
covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8fdd0d42-4716-4c0f-b36d-3edba2c662ca
📒 Files selected for processing (2)
python/cudf/cudf/core/frame.pypython/cudf/cudf/tests/dataframe/methods/test_to_cupy.py
| def is_numpy_object_dtype(dtype: Dtype | None) -> bool: | ||
| try: | ||
| return np.dtype(dtype) == np.dtype("O") | ||
| except TypeError: | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add a regression benchmark for the updated object-dtype null path.
This path changes core conversion behavior in Frame._to_array; please add a unit benchmark that covers to_numpy(dtype=object) with nulls (default na_value and explicit na_value) to guard against performance regressions on this hot conversion route.
As per coding guidelines **/*.{cpp,cu,py,pyx}: Add unit tests and unit benchmarks for code contributions.
Also applies to: 713-716
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cudf/cudf/core/frame.py` around lines 656 - 660, Add a regression
benchmark that measures the hot conversion path exercised by Frame._to_array /
DataFrame.to_numpy(dtype=object) when nulls are present: create a
pytest-benchmark (or project-standard benchmarking harness) test that constructs
frames with nulls and calls to_numpy(dtype=object) twice—once using the default
na_value and once passing an explicit na_value—to capture performance for both
paths; name the test clearly (e.g., bench_to_numpy_object_with_nulls) and place
it with other Python benchmarks so future changes to is_numpy_object_dtype /
Frame._to_array will be covered.
galipremsagar
added
bug
|
pre-commit.ci autofix |
|
/okay to test 78c7264 |
|
/okay to test 1d9e482 |
|
/ok to test cabc766 |
|
/okay to test 1e53275 |
|
/merge |
4 participants
Description
Closes #22419.
This PR fixes
to_numpy(dtype=object)for string/object data containing nulls.Previously, when
dtype=objectwas explicitly requested, cuDF could returnpandas.NA/cudf.NAfor null string values, while pandas returnsNone. The conversion path already boxes values correctly for object dtype, but then overwrote string nulls withcudf.NA.This change skips that null overwrite for explicit NumPy object dtype so nulls remain
None, matching pandas behavior. Explicitna_value=is still honored.A regression test was added for both
DataFrame.to_numpy(dtype=object)andSeries.to_numpy(dtype=object), including the default null behavior and explicitna_value.Checklist
Validation
Local checks run:
Could not run targeted pytest locally; this requires RAPIDS CI or a RAPIDS devcontainer.