CI Fix testing references to take into account string dtype - #7830
Merged
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
Author
|
/ok to test a1bbe70 |
betatim
added
improvement
betatim
commented
Feb 24, 2026
| assert_frame_equal( | ||
| inv.compute().to_pandas().reset_index(drop=True), df.to_pandas() | ||
| inv.compute().to_pandas().reset_index(drop=True), | ||
| X.compute().to_pandas().reset_index(drop=True), |
Contributor
Author
There was a problem hiding this comment.
X is what is passed to fit_transform so it seems reasonable to use it as reference for the result of inverse_transform. And round tripping it through dask-cudf means we should get what ever behaviour it has (across versions)?!
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds dtype-normalization before membership checks in OneHotEncoder/encoder logic and adjusts Dask OneHotEncoder tests by removing xfail markers and updating expected comparisons. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/cuml/tests/dask/test_dask_one_hot_encoder.py (1)
109-112: Considercheck_dtype=Falseinstead of the dask_cudf round-trip for the reference.Routing the static
reffixture throughdask_cudf.from_cudf(...).compute()just to align dtypes is fragile: if dask_cudf's dtype inference diverges from the encoder's internal conversion path, this test can become a false positive without any visible signal. Usingcheck_dtype=Falseinassert_frame_equaldirectly expresses the intent (values match, dtypes may differ) and is immune to that coupling.♻️ Proposed alternative
- ref = dask_cudf.from_cudf(ref, npartitions=1).compute().to_pandas() - assert_frame_equal(df.compute().to_pandas(), ref) + assert_frame_equal(df.compute().to_pandas(), ref.to_pandas(), check_dtype=False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/tests/dask/test_dask_one_hot_encoder.py` around lines 109 - 112, The test currently forces ref through dask_cudf.from_cudf(...).compute() to match dtypes before comparing to df from enc.inverse_transform(Y_ohe); instead, remove the dask_cudf round-trip and call assert_frame_equal(df.compute().to_pandas(), ref, check_dtype=False) so the comparison checks values only while allowing dtype differences; update the assertion that references assert_frame_equal, df (result of enc.inverse_transform), ref, enc, and Y_ohe accordingly.python/cuml/cuml/preprocessing/onehotencoder_mg.py (1)
47-50: Dtype-normalization logic is duplicated fromOneHotEncoder._has_unknown; extract a shared helper.Lines 48–49 are verbatim copies of
encoders.pylines 323–324. Since the only structural difference between the two overrides is the trailing.compute(), a small protected helper (e.g.,_normalize_encoder_cat_dtype) on the base class would eliminate the duplication and keep the two implementations in sync automatically.♻️ Suggested refactor
In
encoders.pyOneHotEncoder:+ `@staticmethod` + def _normalize_encoder_cat_dtype(X_cat, encoder_cat): + if hasattr(encoder_cat, "dtype") and X_cat.dtype != encoder_cat.dtype: + encoder_cat = encoder_cat.astype(X_cat.dtype) + return encoder_cat + def _has_unknown(self, X_cat, encoder_cat): """Check if X_cat has categories that are not present in encoder_cat.""" - if hasattr(encoder_cat, "dtype") and X_cat.dtype != encoder_cat.dtype: - encoder_cat = encoder_cat.astype(X_cat.dtype) + encoder_cat = self._normalize_encoder_cat_dtype(X_cat, encoder_cat) return not X_cat.isin(encoder_cat).all()In
onehotencoder_mg.pyOneHotEncoderMG:def _has_unknown(self, X_cat, encoder_cat): - if hasattr(encoder_cat, "dtype") and X_cat.dtype != encoder_cat.dtype: - encoder_cat = encoder_cat.astype(X_cat.dtype) + encoder_cat = self._normalize_encoder_cat_dtype(X_cat, encoder_cat) return not X_cat.isin(encoder_cat).all().compute()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/preprocessing/onehotencoder_mg.py` around lines 47 - 50, Extract the duplicated dtype-normalization into a protected helper on the base OneHotEncoder (e.g., add a method _normalize_encoder_cat_dtype(self, X_cat, encoder_cat) in OneHotEncoder that checks hasattr(encoder_cat, "dtype") and casts encoder_cat to X_cat.dtype when needed), then replace the duplicated lines in OneHotEncoder._has_unknown and OneHotEncoderMG._has_unknown by calling this new helper before performing the remaining logic (keep OneHotEncoderMG's trailing .compute() behavior). Ensure method names are exactly _normalize_encoder_cat_dtype, OneHotEncoder._has_unknown, and OneHotEncoderMG._has_unknown so both classes call the shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/preprocessing/encoders.py`:
- Around line 307-308: The bug: drop_idx[feature] is set to
cp.asarray(cats[idx].index) which yields a 1D cupy array instead of a Python int
and later breaks get_feature_names() when names.pop(self.drop_idx_[i]) expects
an int. Fix by extracting a plain Python int scalar from the matching index
(e.g., use cats[idx].index[0] or int(cp.asarray(cats[idx].index)[0])) when
assigning drop_idx[feature] so drop_idx values match the scalar type used in the
"first" branch; update the assignment in the block that computes drop_idx
(references: cats, drop_vals, idx, drop_idx, get_feature_names).
---
Nitpick comments:
In `@python/cuml/cuml/preprocessing/onehotencoder_mg.py`:
- Around line 47-50: Extract the duplicated dtype-normalization into a protected
helper on the base OneHotEncoder (e.g., add a method
_normalize_encoder_cat_dtype(self, X_cat, encoder_cat) in OneHotEncoder that
checks hasattr(encoder_cat, "dtype") and casts encoder_cat to X_cat.dtype when
needed), then replace the duplicated lines in OneHotEncoder._has_unknown and
OneHotEncoderMG._has_unknown by calling this new helper before performing the
remaining logic (keep OneHotEncoderMG's trailing .compute() behavior). Ensure
method names are exactly _normalize_encoder_cat_dtype,
OneHotEncoder._has_unknown, and OneHotEncoderMG._has_unknown so both classes
call the shared helper.
In `@python/cuml/tests/dask/test_dask_one_hot_encoder.py`:
- Around line 109-112: The test currently forces ref through
dask_cudf.from_cudf(...).compute() to match dtypes before comparing to df from
enc.inverse_transform(Y_ohe); instead, remove the dask_cudf round-trip and call
assert_frame_equal(df.compute().to_pandas(), ref, check_dtype=False) so the
comparison checks values only while allowing dtype differences; update the
assertion that references assert_frame_equal, df (result of
enc.inverse_transform), ref, enc, and Y_ohe accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
python/cuml/cuml/preprocessing/encoders.pypython/cuml/cuml/preprocessing/onehotencoder_mg.pypython/cuml/tests/dask/test_dask_one_hot_encoder.py
jcrist
approved these changes
Feb 24, 2026
jcrist
left a comment
jcrist
left a comment
Member
There was a problem hiding this comment.
Overall seems fine to me, just one quick question.
Contributor
Author
|
/merge |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
python/cuml/cuml/preprocessing/encoders.py (1)
310-311:⚠️ Potential issue | 🟠 MajorStore
drop_idx_as a scalarint, not an array.Line 311 still stores
cp.asarray(cats[idx].index), which is array-shaped; later (Line 550)names.pop(...)expects an integer index and can fail for dict/listdroppaths.Proposed change
idx = cats.isin(drop_vals) - drop_idx[feature] = cp.asarray(cats[idx].index) + drop_idx[feature] = int(cats[idx].index[0])#!/bin/bash python - <<'PY' import numpy as np names = ["a", "b", "c"] idx = np.asarray([1]) # analogous shape issue try: names.pop(idx) except Exception as e: print(type(e).__name__, str(e)) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/preprocessing/encoders.py` around lines 310 - 311, The code currently assigns drop_idx[feature] = cp.asarray(cats[idx].index) which produces an array; change this to store a scalar Python int so later names.pop(...) works. Replace the assignment in the block that computes drop_idx (referencing drop_idx, cats, and feature) with a scalar conversion such as using .item() or indexing plus int(): e.g. extract the single index value then assign int(value) (or value.item()) to drop_idx[feature] so drop_idx_ becomes an integer rather than an array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/preprocessing/encoders.py`:
- Around line 300-301: The current unconditional dtype coercion of drop_vals to
cats (variables drop_vals and cats) should be restricted so you only coerce when
one side is a string-like/object dtype (e.g., pandas object or pyarrow string)
to handle benign string-representation differences; if the dtypes differ and
neither is string/object, or the mismatch is numeric vs string, raise a
TypeError (or ValueError) instead of astype-ing. Update the logic around the
astype calls used before the isin validation (the blocks that currently do
drop_vals = drop_vals.astype(cats.dtype)) to: (1) detect string-like vs object
using pandas/pyarrow checks, (2) perform astype only for those
string/object-compatible cases, and (3) otherwise abort with a clear error
referencing the offending feature/categories so the subsequent isin(cats).all()
cannot produce false positives.
---
Duplicate comments:
In `@python/cuml/cuml/preprocessing/encoders.py`:
- Around line 310-311: The code currently assigns drop_idx[feature] =
cp.asarray(cats[idx].index) which produces an array; change this to store a
scalar Python int so later names.pop(...) works. Replace the assignment in the
block that computes drop_idx (referencing drop_idx, cats, and feature) with a
scalar conversion such as using .item() or indexing plus int(): e.g. extract the
single index value then assign int(value) (or value.item()) to drop_idx[feature]
so drop_idx_ becomes an integer rather than an array.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
python/cuml/cuml/preprocessing/encoders.pypython/cuml/cuml/preprocessing/onehotencoder_mg.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/cuml/preprocessing/onehotencoder_mg.py
Merged
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Newer versions of cudf are working towards supporting the arrow based string type that is coming to pandas land.
The reference dataframes we use for comparison still use a
objectdtype, this makes the tests fail. This PR attempts to make things more robust.I've not looked at why dask-cudf does this, but I guess it has to do with the migration of pandas to using arrow string dtypes. On the face of it, it seems odd that directly constructing a series produces a
objectdtype and that feeding it through dask-cudf changes it:My curiosity is satisfied with having this fix. I expect a bit of upheaval in this transition for pandas.
fixes #7826