Cleanup LabelEncoder - #8039
Merged
Merged
Conversation
jcrist
added
improvement
|
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. |
Member
Author
|
/ok to test d27e04a |
44 tasks
Member
Author
|
/ok to test 9afcae6 |
Member
Author
|
/ok to test 7aa2c69 |
jcrist
force-pushed
the
cleanup-label-encoder
branch
from
May 1, 2026 19:25
7aa2c69 to
a3b6a0d
Compare
Member
Author
|
/ok to test a3b6a0d |
jcrist
force-pushed
the
cleanup-label-encoder
branch
from
May 4, 2026 21:46
a3b6a0d to
d7e6490
Compare
jcrist
marked this pull request as ready for review
May 4, 2026 21:47
|
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:
📝 WalkthroughWalkthroughLabel-encoding and encoder internals refactored to validate inputs, preserve and propagate index, force CUDF internal execution for per-feature encoders, and update unseen-label handling to raise ValueError; decode_labels signature extended and NumPy object coercion adjusted. Tests and dask wiring updated accordingly. ChangesLabel Encoding Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cuml/cuml/dask/preprocessing/_label.py (1)
166-170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring states
KeyErrorbut implementation now raisesValueError.The docstring mentions raising
KeyErrorfor unseen categories, but the underlyingLabelEncoder.transformnow raisesValueError. Update the docstring to match the new behavior.📝 Proposed fix
Raises ------ - KeyError + ValueError if a category appears that was not seen in `fit`🤖 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/cuml/cuml/dask/preprocessing/_label.py` around lines 166 - 170, Update the docstring for the method/class that documents raising on unseen categories to reflect the actual exception thrown: replace the "KeyError" mention with "ValueError" (or state that LabelEncoder.transform raises ValueError for unseen categories) so the docstring matches the implementation (see LabelEncoder.transform in _label.py).python/cuml/tests/test_ordinal_encoder.py (1)
119-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a real assertion for the array unknown-path check.
Line 120 computes
np.isnan(encoded[0, 0])but does not assert it, so theas_array=Truebranch can pass even when behavior is wrong.Proposed fix
encoded = enc.transform(Y) if as_array: - np.isnan(encoded[0, 0]) + assert np.isnan(encoded[0, 0]) else: assert pd.isna(encoded.iloc[0, 0])As per coding guidelines:
python/**/test_*.py: “Test files must ... test different input types (cuDF, pandas, NumPy)”.🤖 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/cuml/tests/test_ordinal_encoder.py` around lines 119 - 121, The test's unknown-value path for NumPy arrays currently computes np.isnan(encoded[0, 0]) but doesn't assert it; update the branch guarded by as_array to assert that np.isnan(encoded[0, 0]) is True (or use np.testing.assert_true/np.assert to explicitly check NaN) so the as_array=True path fails when encoding is incorrect; locate the as_array conditional and the variable encoded and replace the bare expression with an explicit assertion using np.isnan(encoded[0, 0]).
🤖 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/cuml/cuml/preprocessing/_label.py`:
- Line 208: Replace the unsupported np.concat call with np.concatenate to
maintain compatibility with numpy>=1.23; specifically, update the expression
using the classes variable (currently written as np.concat([classes, [None]]))
to np.concatenate([classes, [None]]) in the preprocessing label code (look for
the classes variable assignment in _label.py) so the code works across NumPy
1.23–2.x.
---
Outside diff comments:
In `@python/cuml/cuml/dask/preprocessing/_label.py`:
- Around line 166-170: Update the docstring for the method/class that documents
raising on unseen categories to reflect the actual exception thrown: replace the
"KeyError" mention with "ValueError" (or state that LabelEncoder.transform
raises ValueError for unseen categories) so the docstring matches the
implementation (see LabelEncoder.transform in _label.py).
In `@python/cuml/tests/test_ordinal_encoder.py`:
- Around line 119-121: The test's unknown-value path for NumPy arrays currently
computes np.isnan(encoded[0, 0]) but doesn't assert it; update the branch
guarded by as_array to assert that np.isnan(encoded[0, 0]) is True (or use
np.testing.assert_true/np.assert to explicitly check NaN) so the as_array=True
path fails when encoding is incorrect; locate the as_array conditional and the
variable encoded and replace the bare expression with an explicit assertion
using np.isnan(encoded[0, 0]).
🪄 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: 74302a2f-f268-4f90-b700-eaf0a07dda4e
📒 Files selected for processing (11)
python/cuml/cuml/common/classification.pypython/cuml/cuml/dask/preprocessing/_label.pypython/cuml/cuml/internals/validation.pypython/cuml/cuml/preprocessing/_label.pypython/cuml/cuml/preprocessing/encoders.pypython/cuml/tests/dask/test_dask_label_encoder.pypython/cuml/tests/dask/test_dask_one_hot_encoder.pypython/cuml/tests/dask/test_dask_ordinal_encoder.pypython/cuml/tests/test_label_encoder.pypython/cuml/tests/test_one_hot_encoder.pypython/cuml/tests/test_ordinal_encoder.py
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/cuml/cuml/preprocessing/_label.py`:
- Around line 188-213: The inverse transform unpacks index from check_y but
doesn't pass it to decode_labels, causing loss of the input index on
inverse_transform; update the call site (the decode_labels invocation in the
method containing check_y and codes handling) to pass the unpacked index (i.e.,
call decode_labels with the index=index argument along with codes, classes, and
output_type) so inverse_transform preserves the original index like
fit_transform and transform do.
🪄 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: 7defa29c-539e-4873-8cf0-be57af5f7607
📒 Files selected for processing (1)
python/cuml/cuml/preprocessing/_label.py
jcrist
force-pushed
the
cleanup-label-encoder
branch
from
May 4, 2026 22:16
f39444c to
1fdca94
Compare
This was referenced May 6, 2026
rapids-bot Bot
pushed a commit
that referenced
this pull request
May 6, 2026
When returning pandas/cudf outputs, we want the index of the output to be aligned with the index of the input. This PR plumbs through `index` in `decode_labels`, adds a test for all classifiers, then applies the necessary fixes and plumbing so the output has an aligned index. In the long run I hope to move index alignment handling to the `reflect` decorator so method code doesn't need to worry about this, but for now this is the most idiomatic and cleanest solution. Split out and expanded from #8039. Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Simon Adorf (https://github.com/csadorf) URL: #8056
jcrist
force-pushed
the
cleanup-label-encoder
branch
from
May 6, 2026 19:10
1fdca94 to
e573a0b
Compare
csadorf
requested changes
May 7, 2026
- Simplifies internals - Applies standard type reflection - Moves `classes_` to be a numpy array instead of a cudf object - Cleans up docstrings - Improves reliability across input and output types
Other encoders and tests needed some updating after the fallout of the `LabelEncoder` changes. This is mostly cosmetic and not user-facing.
`OneHotEncoder` and `OrdinalEncoder` don't use our standard reflection machinery, which is annoying. We have to use the context manager in all method-local call sites to ensure things properly work and aren't overridden - if these classes used `reflect` like a good citizen then we wouldn't have this problem.
jcrist
force-pushed
the
cleanup-label-encoder
branch
from
May 7, 2026 23:17
e573a0b to
170805f
Compare
csadorf
approved these changes
May 8, 2026
csadorf
left a comment
csadorf
left a comment
Contributor
There was a problem hiding this comment.
Not sure about the failures, but my concerns have been addressed.
Member
Author
|
The failures are on |
Contributor
|
Created #8068 as an independent fix. |
Contributor
|
/merge |
This was referenced May 11, 2026
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.
This cleans up our
LabelEncoderimplementation, preparing for exposing it throughcuml.accel. This one needed a lot of work, and there are several breaking changes.LabelEncoder.classes_is now anumpy.ndarrayinstead of acudf.Series. Note thatclasses_was undocumented. This mirrors theclasses_attribute in all our other classifiers (all of which also made this as a breaking change many releases ago). I view this in the same vein.OneHotEncoderandOrdinalEncodermake use ofLabelEncoderinternally, the type ofcategories_has changed there fromlist[cudf.Series]tolist[numpy.ndarray]. This now better matches with sklearn, and makes sense IMO in the same way that changingclasses_above did.LabelEncodernow follows our type-reflection standards, previously it always returnedcudfobjects (even though the docstrings implied type-reflection was followed). I view this as a bugfix.LabelEncoder.inverse_transformnow handleshandle_unknown="ignore"as documented. Previously this would return incorrect values in the presence of missing values. Note thathandle_unknownis not an argument tosklearn.preprocessing.LabelEncoder. I view this as a bugfix.This required munging in our tests and the implementations of
OrdinalEncoder/OneHotEncoder(and their dask counterparts), but beyond the changes listed above should have no other user-facing ramifications.Stacked on top of #8038.
Part of #7317.
Precursor for #8015.