Cleanup TargetEncoder - #8075
Conversation
jcrist
added
improvement
|
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:
📝 WalkthroughWalkthroughRefactors cuML TargetEncoder: narrows GPU fallback to target-type-only checks, standardizes internal column naming and input coercion, reorganizes fit/transform and per-feature encoding computation, tightens sklearn interop, and updates docs, xfails, and tests to match normalized outputs. ChangesTargetEncoder Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🤖 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/accel/_overrides/sklearn/preprocessing.py`:
- Around line 56-68: In _check_targetencoder_y, the UnsupportedOnGPU error
message uses "{target_type!r}" literally; change the string to an f-string so
target_type is interpolated (e.g., prefix the message with f) when raising
UnsupportedOnGPU, keeping the same message text and referencing target_type, the
_check_targetencoder_y function and the UnsupportedOnGPU exception.
🪄 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: 1d8496eb-7b81-4ae0-b124-1799e437141c
📒 Files selected for processing (5)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/preprocessing.pypython/cuml/cuml/preprocessing/_target_encoder.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_target_encoder.py
| marker: cuml_accel_bugs | ||
| tests: | ||
| - "sklearn.preprocessing.tests.test_target_encoder::test_encoding[42-binary-5.0-auto-3]" | ||
| - "sklearn.preprocessing.tests.test_target_encoder::test_encoding[42-binary-auto-auto-3]" |
There was a problem hiding this comment.
These previously would fallback due to object dtype for y. We now don't fallback, but give slightly different results than sklearn. The failure here is due to the same reason the continuous case was already in the xfail list.
|
|
||
| test_encoded = encoder.transform(test) | ||
| answer = np.array([1.5, 1.5, 1.5, 1.5])[:, None] | ||
| answer = np.array([0.5, 0.5, 0.5, 0.5])[:, None] |
There was a problem hiding this comment.
The values here changed due to a previous bug in the implementation. The code intended to treat binary integral values as binary inputs, but accidentally would fail for non-numpy inputs (like the cudf.Series used here), falling back to continuous. I fixed the bug, and had to update the values accordingly.
| {"category": ["a", "a", "a", "a", "b", "b", "b", "b"]} | ||
| ) | ||
| label = cudf.Series([1, 22, 15, 17, 70, 9, 99, 56]) | ||
| label = cudf.Series([1, 22, 15, 17, 70, 9, 99, 56], dtype="float32") |
There was a problem hiding this comment.
This is a continuous target, and needs to be float32 to be treated accordingly.
There was a problem hiding this comment.
This might be a good comment to add in the test itself for future reference
| self.out_col = "__TARGET_ENCODE__" | ||
| self.out_col2 = "__TARGET_ENCODE__SQUARE__" | ||
| self.fold_col = "__FOLD__" | ||
| self.id_col = "__INDEX__" |
There was a problem hiding this comment.
These were all private implementation details, and aren't needed for the cleaned up implementation.
1dca6b7 to
eb05daa
Compare
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/preprocessing/_target_encoder.py (2)
319-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset mode-specific fitted state before branching into the new fit.
_fit_transformnever clears_independent_mode_fitted,_encode_all_per_feature, or_encodings_per_feature. After one multi-feature independent fit, a later refit in single-feature or combination mode can still report the old output shape and sendtransformdown the stale independent-path with previous encodings.Suggested reset at the start of `_fit_transform`
def _fit_transform(self, X, y, fold_ids): + self._independent_mode_fitted = False + self.encode_all = None + if hasattr(self, "_encode_all_per_feature"): + del self._encode_all_per_feature + if hasattr(self, "_encodings_per_feature"): + del self._encodings_per_feature + if self.smooth < 0: raise ValueError(f"smooth {self.smooth} is not zero or positive")As per coding guidelines, "Model state management must ensure fit/predict/transform maintain consistent state, fit() resets all learned attributes, and state from previous fit() calls does not affect new training."
🤖 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/preprocessing/_target_encoder.py` around lines 319 - 380, The _fit_transform method doesn't clear multi-feature-specific state, so previous fits can leak into later fits; at the start of _fit_transform reset the mode-specific attributes _independent_mode_fitted (set to False), _encode_all_per_feature (set to False or appropriate default), and _encodings_per_feature (set to {} or empty list as used elsewhere) before any validation or branching, so every call to _fit_transform starts from a clean fitted state and subsequent branching to _fit_transform_independent/_fit_transform_combination uses only newly computed encodings.
445-499:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftIndependent mode is no longer cross-fitted, and
stat='var'returns the wrong statistic.This method never uses
fold, sofit_transform/transformon training data merge full-data encodings back into each row instead of leave-fold-out values. Also, the non-median branch calls_compute_single_feature_encodingfor both"mean"and"var", but that helper only computes a smoothed mean. Multi-feature independent mode is therefore leaking targets for all stats and is outright incorrect for variance.As per coding guidelines, "Logic errors in ML algorithm implementations, incorrect distance metrics, kernels, or loss function implementations, numerical instability causing wrong results, and incorrect model parameter initialization must be corrected."
🤖 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/preprocessing/_target_encoder.py` around lines 445 - 499, _fit_transform_independent is leaking target information because it builds full-data encodings (encode_all_i) and merges them back into every row instead of computing leave-fold-out encodings using fold_ids, and it calls _compute_single_feature_encoding for stat=="var" even though that helper computes only a smoothed mean. Fix by computing encodings per fold: iterate fold_ids for each feature and call the appropriate helper that accepts a fold mask (or implement a new _compute_single_feature_encoding_var) to produce fold-specific encode_all_i, then merge the encoding for each row using its fold id (not the full-data table) so transform() gets true out-of-fold values; also add a proper branch for stat=="var" that computes variance (or calls _compute_single_feature_encoding_median-style var helper) and ensure you populate _encode_all_per_feature and self.encode_all with per-fold encodings (or a structure keyed by fold) and keep y_stat_val/fillna behavior unchanged.
🤖 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/_target_encoder.py`:
- Around line 548-551: The "continuous" branch is producing fractional fold ids
because it uses true division; change it to perform integer block assignment by
scaling the sample indices by n_folds and integer-dividing by n_samples (using
cp.arange, n_samples and n_folds) and ensure the result is an integer dtype so
you get contiguous fold labels for self.split_method == "continuous".
---
Outside diff comments:
In `@python/cuml/cuml/preprocessing/_target_encoder.py`:
- Around line 319-380: The _fit_transform method doesn't clear
multi-feature-specific state, so previous fits can leak into later fits; at the
start of _fit_transform reset the mode-specific attributes
_independent_mode_fitted (set to False), _encode_all_per_feature (set to False
or appropriate default), and _encodings_per_feature (set to {} or empty list as
used elsewhere) before any validation or branching, so every call to
_fit_transform starts from a clean fitted state and subsequent branching to
_fit_transform_independent/_fit_transform_combination uses only newly computed
encodings.
- Around line 445-499: _fit_transform_independent is leaking target information
because it builds full-data encodings (encode_all_i) and merges them back into
every row instead of computing leave-fold-out encodings using fold_ids, and it
calls _compute_single_feature_encoding for stat=="var" even though that helper
computes only a smoothed mean. Fix by computing encodings per fold: iterate
fold_ids for each feature and call the appropriate helper that accepts a fold
mask (or implement a new _compute_single_feature_encoding_var) to produce
fold-specific encode_all_i, then merge the encoding for each row using its fold
id (not the full-data table) so transform() gets true out-of-fold values; also
add a proper branch for stat=="var" that computes variance (or calls
_compute_single_feature_encoding_median-style var helper) and ensure you
populate _encode_all_per_feature and self.encode_all with per-fold encodings (or
a structure keyed by fold) and keep y_stat_val/fillna behavior unchanged.
🪄 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: ae2b72f0-625a-4988-b53f-80b9b1418291
📒 Files selected for processing (5)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/preprocessing.pypython/cuml/cuml/preprocessing/_target_encoder.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_target_encoder.py
✅ Files skipped from review due to trivial changes (2)
- docs/source/cuml-accel/limitations.rst
- python/cuml/tests/test_target_encoder.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cuml/cuml/accel/_overrides/sklearn/preprocessing.py
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
| elif self.split_method == "continuous": | ||
| return ( | ||
| cp.arange(len_train) / (len_train / self.n_folds) | ||
| ) % self.n_folds | ||
| return (cp.arange(n_samples) / (n_samples / n_folds)) % n_folds | ||
| elif self.split_method == "interleaved": | ||
| return cp.arange(len_train) % self.n_folds | ||
| elif self.split_method == "customize": | ||
| if fold_ids is None: | ||
| raise ValueError( | ||
| "fold_ids can't be None" | ||
| "since split_method is set to" | ||
| "'customize'." | ||
| ) | ||
| return fold_ids | ||
| return cp.arange(n_samples) % n_folds |
There was a problem hiding this comment.
continuous splitting is generating fractional fold ids instead of contiguous folds.
Line 549 uses true division, so the result is values like 0.0, 0.4, 0.8, 1.2, ... rather than integer fold labels. That creates many more effective groups than n_folds and changes the cross-fitting behavior.
Use integer block assignment here
elif self.split_method == "continuous":
- return (cp.arange(n_samples) / (n_samples / n_folds)) % n_folds
+ return (
+ cp.arange(n_samples, dtype=cp.int32) * n_folds // n_samples
+ )As per coding guidelines, "Logic errors in ML algorithm implementations, incorrect distance metrics, kernels, or loss function implementations, numerical instability causing wrong results, and incorrect model parameter initialization must be corrected."
🤖 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/preprocessing/_target_encoder.py` around lines 548 - 551,
The "continuous" branch is producing fractional fold ids because it uses true
division; change it to perform integer block assignment by scaling the sample
indices by n_folds and integer-dividing by n_samples (using cp.arange, n_samples
and n_folds) and ensure the result is an integer dtype so you get contiguous
fold labels for self.split_method == "continuous".
dantegd
left a comment
dantegd
left a comment
There was a problem hiding this comment.
Cleanup is a big improvement, love it and just had a few questions
| {"category": ["a", "a", "a", "a", "b", "b", "b", "b"]} | ||
| ) | ||
| label = cudf.Series([1, 22, 15, 17, 70, 9, 99, 56]) | ||
| label = cudf.Series([1, 22, 15, 17, 70, 9, 99, 56], dtype="float32") |
There was a problem hiding this comment.
This might be a good comment to add in the test itself for future reference
eb05daa to
69cfc10
Compare
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 (1)
python/cuml/cuml/preprocessing/_target_encoder.py (1)
427-481:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftIndependent-mode
fit_transformis no longer cross-fitting.This branch never uses
df.foldorfold_ids; it builds eachencode_all_ifrom the full training frame and merges it straight back into the same rows. That makesfit_transform(..., multi_feature_mode="independent")target-leaky, andn_folds/split_method/fold_idsbecome no-ops in this mode.As per coding guidelines, "Logic errors in ML algorithm implementations, incorrect distance metrics, kernels, or loss function implementations, numerical instability causing wrong results, and incorrect model parameter initialization must be corrected."
🤖 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/preprocessing/_target_encoder.py` around lines 427 - 481, _fit_transform_independent currently computes each feature's encoding from the full df (via _compute_single_feature_encoding/_compute_single_feature_encoding_median) and merges it back into the same rows, causing target leakage and ignoring fold_ids; change it to perform out-of-fold (cross-fit) encoding: for each feature col and for each fold id in fold_ids, compute encode_all_i using only rows NOT in that fold (i.e., exclude df[df.fold == fold] or use fold_ids mask) and then merge the per-fold encodings into the rows of that fold (so each row gets encoding computed without its own target), falling back to global OOF mean for unseen categories; update calls to _compute_single_feature_encoding/_median (or add a parameter) so they accept an exclusion mask or fold id, ensure df merging uses fold-aware mapping rather than a full-frame merge, and keep storing per-feature encodings in _encode_all_per_feature and _encodings_per_feature, then set _independent_mode_fitted true as before.
♻️ Duplicate comments (2)
python/cuml/cuml/preprocessing/_target_encoder.py (2)
510-535:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse integer block assignment for the
continuoussplit.Line 534 still uses true division, so the fold labels are fractional (
0.0, 0.4, 0.8, ...) instead of contiguous fold ids. That changes the effective grouping and breaks the intended cross-fitting behavior forsplit_method="continuous".Suggested fix
elif self.split_method == "continuous": - return (cp.arange(n_samples) / (n_samples / n_folds)) % n_folds + return ( + cp.arange(n_samples, dtype=cp.int32) * n_folds // n_samples + )As per coding guidelines, "Logic errors in ML algorithm implementations, incorrect distance metrics, kernels, or loss function implementations, numerical instability causing wrong results, and incorrect model parameter initialization must be corrected."
🤖 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/preprocessing/_target_encoder.py` around lines 510 - 535, The continuous split currently computes fractional fold labels using true division in the method that generates fold IDs (the continuous branch in _target_encoder.py), producing values like 0.0, 0.4, etc.; replace that expression with integer block assignment so labels are contiguous integers 0..n_folds-1 (e.g. use (cp.arange(n_samples) * n_folds) // n_samples and take modulo n_folds if needed), and return as an integer array (cast to a suitable integer dtype) so cross-fitting grouping behaves correctly.
510-513:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
n_foldsis integral before comparing it.
self.n_folds < 1still throws a rawTypeErrorfor inputs like"4", and non-integral floats can slip deeper into fold generation. Please reject non-integer values explicitly here so invalid inputs fail with a clearValueError.As per coding guidelines, "Silent data corruption from type coercion, incorrect handling of cuDF vs pandas vs NumPy inputs, or missing validation causing crashes on invalid input must be addressed."
🤖 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/preprocessing/_target_encoder.py` around lines 510 - 513, Validate that self.n_folds is an integer before comparing or using it: add an explicit type check (e.g., using isinstance(self.n_folds, numbers.Integral) or similar) and raise a ValueError like "n_folds must be an integer >= 1" if it is not integral; then keep the existing range check (if self.n_folds < 1) and the subsequent assignment n_folds = min(self.n_folds, n_samples), casting to int if necessary to avoid float propagation into fold generation.
🧹 Nitpick comments (1)
python/cuml/tests/test_target_encoder.py (1)
296-324: ⚡ Quick winAdd a non-NumPy case to this
target_type_regression test.The refactor here is mostly in
_check_X_y/check_cudf, but this new coverage only exercises NumPy inputs. Adding at least onecudf.Seriesorpandas.Seriestarget case would better protect the behavior that changed.As per coding guidelines, "Test files must validate numerical correctness by comparing with scikit-learn, include edge case coverage (empty datasets, single sample, high-dimensional data), test fit/predict/transform consistency, and 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_target_encoder.py` around lines 296 - 324, Add a non-NumPy target case to test_target_encoder_target_type_and_classes to cover the refactor in _check_X_y / check_cudf: after the existing NumPy cases, create at least one pandas.Series or cudf.Series y (e.g., string or numeric labels) and call TargetEncoder().fit(X, y) and assert the same expected target_type_ and classes_ as the equivalent NumPy case; ensure you import pandas or cudf in the test and mirror one of the existing assertions (e.g., string labels -> target_type_ == "binary" and classes_ equals ["a","b"]) so the test verifies non-NumPy input handling.
🤖 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/_target_encoder.py`:
- Around line 325-362: The _fit_transform method must clear any leftover
independent-mode state before fitting: at the start of _fit_transform (in
function _fit_transform) reset/initialize _independent_mode_fitted = False,
_encode_all_per_feature = None (or appropriate default), and
_encodings_per_feature = None (or empty structure), and ensure _n_features_out
is recomputed later from the current fit (not left from prior runs) so
transform() won't incorrectly route to _transform_independent; update those
attributes before computing categories_ and before branching to
_fit_transform_independent/_fit_transform_combination to guarantee a fresh state
for each fit.
---
Outside diff comments:
In `@python/cuml/cuml/preprocessing/_target_encoder.py`:
- Around line 427-481: _fit_transform_independent currently computes each
feature's encoding from the full df (via
_compute_single_feature_encoding/_compute_single_feature_encoding_median) and
merges it back into the same rows, causing target leakage and ignoring fold_ids;
change it to perform out-of-fold (cross-fit) encoding: for each feature col and
for each fold id in fold_ids, compute encode_all_i using only rows NOT in that
fold (i.e., exclude df[df.fold == fold] or use fold_ids mask) and then merge the
per-fold encodings into the rows of that fold (so each row gets encoding
computed without its own target), falling back to global OOF mean for unseen
categories; update calls to _compute_single_feature_encoding/_median (or add a
parameter) so they accept an exclusion mask or fold id, ensure df merging uses
fold-aware mapping rather than a full-frame merge, and keep storing per-feature
encodings in _encode_all_per_feature and _encodings_per_feature, then set
_independent_mode_fitted true as before.
---
Duplicate comments:
In `@python/cuml/cuml/preprocessing/_target_encoder.py`:
- Around line 510-535: The continuous split currently computes fractional fold
labels using true division in the method that generates fold IDs (the continuous
branch in _target_encoder.py), producing values like 0.0, 0.4, etc.; replace
that expression with integer block assignment so labels are contiguous integers
0..n_folds-1 (e.g. use (cp.arange(n_samples) * n_folds) // n_samples and take
modulo n_folds if needed), and return as an integer array (cast to a suitable
integer dtype) so cross-fitting grouping behaves correctly.
- Around line 510-513: Validate that self.n_folds is an integer before comparing
or using it: add an explicit type check (e.g., using isinstance(self.n_folds,
numbers.Integral) or similar) and raise a ValueError like "n_folds must be an
integer >= 1" if it is not integral; then keep the existing range check (if
self.n_folds < 1) and the subsequent assignment n_folds = min(self.n_folds,
n_samples), casting to int if necessary to avoid float propagation into fold
generation.
---
Nitpick comments:
In `@python/cuml/tests/test_target_encoder.py`:
- Around line 296-324: Add a non-NumPy target case to
test_target_encoder_target_type_and_classes to cover the refactor in _check_X_y
/ check_cudf: after the existing NumPy cases, create at least one pandas.Series
or cudf.Series y (e.g., string or numeric labels) and call
TargetEncoder().fit(X, y) and assert the same expected target_type_ and classes_
as the equivalent NumPy case; ensure you import pandas or cudf in the test and
mirror one of the existing assertions (e.g., string labels -> target_type_ ==
"binary" and classes_ equals ["a","b"]) so the test verifies non-NumPy input
handling.
🪄 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: 48ed5f13-b5ce-4b90-8af3-e92224371c58
📒 Files selected for processing (5)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/preprocessing.pypython/cuml/cuml/preprocessing/_target_encoder.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_target_encoder.py
✅ Files skipped from review due to trivial changes (2)
- docs/source/cuml-accel/limitations.rst
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/cuml/accel/_overrides/sklearn/preprocessing.py
csadorf
left a comment
csadorf
left a comment
There was a problem hiding this comment.
No further comments. LGTM!
- Updates TargetEncoder to use the new validation utilities - Updates TargetEncoder to follow cuml conventions (simple __init__, etc...). I left some documented fitted attributes that don't follow our conventions (no trailing `_`) for now. I consider deprecating/changing those to be outside the scope of this PR. - Greatly simplifies the implementation, mostly removing duplicate lines. - Cleans up the cuml.accel wrapper to remove unnecessary fallback cases, and put the fallback checks in the proper places. - Updates xfails appropriately.
69cfc10 to
4d45c5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
python/cuml/cuml/preprocessing/_target_encoder.py (3)
510-513:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
n_foldsas a positive integer.
self.n_folds < 1still accepts values like2.5, which then produce float fold ids in the deterministic branches and confusing errors in the random branch. Reject non-integral values beforemin(...).Suggested guard
- if self.n_folds < 1: - raise ValueError("n_folds >= 1 is required") + if not isinstance(self.n_folds, (int, np.integer)) or self.n_folds < 1: + raise ValueError("n_folds must be a positive integer")🤖 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/preprocessing/_target_encoder.py` around lines 510 - 513, The current guard only checks self.n_folds < 1 which allows non-integer values like 2.5; update the validation (in the TargetEncoder method where self.n_folds is used) to first verify self.n_folds is a positive integer (e.g., isinstance(self.n_folds, numbers.Integral) or int(self.n_folds) == self.n_folds and self.n_folds >= 1) and raise a TypeError or ValueError if not, before computing n_folds = min(self.n_folds, n_samples), so n_folds remains an integer and downstream fold-id logic using self.n_folds and n_folds won’t produce floats.
325-362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset independent-mode state before dispatching to the new fit path.
After one
multi_feature_mode="independent"fit, a later combination/median fit leaves_independent_mode_fitted,_encode_all_per_feature, and_encodings_per_featurealive.fit()then derives_n_features_outfrom the stale flag,transform()can route through_transform_independent(), and_attrs_to_cpu()can export outdated per-feature encodings. Clear that state at the top of_fit_transform().Minimal reset at the start of `_fit_transform`
def _fit_transform(self, X, y, fold_ids): + self._independent_mode_fitted = False + self._encode_all_per_feature = [] + self._encodings_per_feature = [] + if self.smooth < 0: raise ValueError(f"smooth {self.smooth} is not zero or positive")As per coding guidelines, "Model state management must ensure fit/predict/transform maintain consistent state, fit() resets all learned attributes, and state from previous fit() calls does not affect new training."
🤖 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/preprocessing/_target_encoder.py` around lines 325 - 362, The independent-mode learned state must be cleared at the start of _fit_transform to avoid stale per-feature data affecting later combination/median fits: at the top of _fit_transform (before validations and df creation) reset _independent_mode_fitted = False, _encode_all_per_feature = False (or None if that’s the canonical unset), and _encodings_per_feature = None or an empty dict/list whatever the rest of the class expects; ensure these same attribute names (_independent_mode_fitted, _encode_all_per_feature, _encodings_per_feature) are referenced so subsequent logic (including _n_features_out calculation, transform(), and _attrs_to_cpu()) uses the fresh state.
533-535:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse integer block assignment for
continuousfolds.This branch still returns fractional ids (
0.0,0.4,0.8, …) instead of contiguous fold labels, so cross-fitting groups on many more values thann_folds. The fold ids need to stay integral here.Integer fold assignment
- elif self.split_method == "continuous": - return (cp.arange(n_samples) / (n_samples / n_folds)) % n_folds + elif self.split_method == "continuous": + return cp.arange(n_samples, dtype=cp.int32) * n_folds // n_samplesAs per coding guidelines, "Logic errors in ML algorithm implementations, incorrect distance metrics, kernels, or loss function implementations, numerical instability causing wrong results, and incorrect model parameter initialization must be corrected."
🤖 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/preprocessing/_target_encoder.py` around lines 533 - 535, The continuous split branch returns fractional fold ids; change it to produce integer fold labels by mapping sample indices to folds using integer math instead of floating division. In the branch where self.split_method == "continuous", replace the current expression that divides cp.arange(n_samples) by (n_samples / n_folds) with an integer assignment such as scaling indices by n_folds and using integer division or floor (e.g., (cp.arange(n_samples) * n_folds) // n_samples or cp.floor(cp.arange(n_samples) * n_folds / n_samples)) and then apply % n_folds so the resulting fold ids are integral; ensure you update the code in the same function containing self.split_method, n_samples, and n_folds.
🤖 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/internals/validation.py`:
- Around line 857-866: The code currently forces any non-string object array to
float64 in the block inside check_cudf()/TargetEncoder._check_X, which can
silently change integer/bool/category keys; instead, attempt to infer a concrete
numeric dtype first and only coerce when inference succeeds: use an explicit
numeric inference (e.g., pandas.api.types.infer_dtype or pandas.to_numeric(...,
errors='raise')) on array.flat[0] or the whole array to confirm all entries are
numeric, determine the appropriate numeric dtype (int, bool, float) and only
astype to that inferred dtype (not always float64); if inference fails, leave
the array as object and fall back to the non-numeric path. Ensure this logic is
applied in the same branch where array.dtype == "object" and array.size to avoid
unconditional float casting.
In `@python/cuml/cuml/preprocessing/_target_encoder.py`:
- Around line 637-643: The current except ValueError block in the float-branch
of _target_encoder.py sets continuous=True for any check_classification_targets
failure, which incorrectly treats NaN/inf as continuous; change the except to
re-raise the original ValueError when the failure is due to non-finite targets
and only set continuous=True when the failure is not caused by NaN/inf.
Concretely, in the try/except around check_classification_targets(y) catch
ValueError as e, then if y contains NaN or non-finite values (use
cudf.isna(y).any() or cudf.isfinite(y).all()) re-raise e, otherwise set
continuous = True; reference the existing check_classification_targets call and
the continuous variable so you modify that specific branch.
---
Duplicate comments:
In `@python/cuml/cuml/preprocessing/_target_encoder.py`:
- Around line 510-513: The current guard only checks self.n_folds < 1 which
allows non-integer values like 2.5; update the validation (in the TargetEncoder
method where self.n_folds is used) to first verify self.n_folds is a positive
integer (e.g., isinstance(self.n_folds, numbers.Integral) or int(self.n_folds)
== self.n_folds and self.n_folds >= 1) and raise a TypeError or ValueError if
not, before computing n_folds = min(self.n_folds, n_samples), so n_folds remains
an integer and downstream fold-id logic using self.n_folds and n_folds won’t
produce floats.
- Around line 325-362: The independent-mode learned state must be cleared at the
start of _fit_transform to avoid stale per-feature data affecting later
combination/median fits: at the top of _fit_transform (before validations and df
creation) reset _independent_mode_fitted = False, _encode_all_per_feature =
False (or None if that’s the canonical unset), and _encodings_per_feature = None
or an empty dict/list whatever the rest of the class expects; ensure these same
attribute names (_independent_mode_fitted, _encode_all_per_feature,
_encodings_per_feature) are referenced so subsequent logic (including
_n_features_out calculation, transform(), and _attrs_to_cpu()) uses the fresh
state.
- Around line 533-535: The continuous split branch returns fractional fold ids;
change it to produce integer fold labels by mapping sample indices to folds
using integer math instead of floating division. In the branch where
self.split_method == "continuous", replace the current expression that divides
cp.arange(n_samples) by (n_samples / n_folds) with an integer assignment such as
scaling indices by n_folds and using integer division or floor (e.g.,
(cp.arange(n_samples) * n_folds) // n_samples or cp.floor(cp.arange(n_samples) *
n_folds / n_samples)) and then apply % n_folds so the resulting fold ids are
integral; ensure you update the code in the same function containing
self.split_method, n_samples, and n_folds.
🪄 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: 8fdaaf17-eb0f-40cc-8f86-0f47325489a3
📒 Files selected for processing (7)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/preprocessing.pypython/cuml/cuml/internals/validation.pypython/cuml/cuml/preprocessing/_target_encoder.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_target_encoder.pypython/cuml/tests/test_validation.py
✅ Files skipped from review due to trivial changes (1)
- docs/source/cuml-accel/limitations.rst
🚧 Files skipped from review as they are similar to previous changes (3)
- python/cuml/tests/test_target_encoder.py
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
- python/cuml/cuml/accel/_overrides/sklearn/preprocessing.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
python/cuml/cuml/preprocessing/_target_encoder.py (2)
630-649:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatching all
ValueErrormay incorrectly treat invalid targets as continuous.The broad
except ValueErrorcatches any failure fromcheck_classification_targets, not just the "continuous" label detection. If the function raises for other reasons (e.g., multi-output labels, or certain malformed inputs), this would incorrectly setcontinuous = Trueand bypass the multiclass check.Proposed fix: check for the specific continuous label error
if cudf.api.types.is_float_dtype(y): # Floating input. Check if it's a valid classification target. try: check_classification_targets(y) - except ValueError: - continuous = True + except ValueError as exc: + if "Unknown label type" in str(exc) and "continuous" in str(exc): + continuous = True + else: + raise#!/bin/bash # Check what error messages check_classification_targets can raise ast-grep --pattern 'def check_classification_targets($$$) { $$$ }' rg -n "check_classification_targets" --type py -A 10 -B 2 | head -100🤖 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/preprocessing/_target_encoder.py` around lines 630 - 649, The code currently catches any ValueError from check_classification_targets and treats the target as continuous; change the broad except to capture the exception (except ValueError as e) and only set continuous = True when the exception message matches the specific continuous-target signal (e.g., contains "Continuous" or the exact message emitted by check_classification_targets for continuous labels); otherwise re-raise the exception so malformed/multi-output errors are not misclassified. Locate this logic around check_classification_targets, the continuous variable, and the subsequent setting of self.target_type_ / self.classes_ to apply the conditional re-raise behavior.
526-527:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
continuoussplit produces fractional fold IDs instead of integer folds.The true division produces values like
[0.0, 0.4, 0.8, 1.2, ...]instead of integer fold labels. When used in groupby operations, this creates many more effective groups thann_folds, breaking the cross-validation logic.Proposed fix: use integer block assignment
elif self.split_method == "continuous": - return (cp.arange(n_samples) / (n_samples / n_folds)) % n_folds + return cp.arange(n_samples, dtype=cp.int32) * n_folds // n_samples🤖 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/preprocessing/_target_encoder.py` around lines 526 - 527, The continuous split currently uses true division producing fractional fold labels; change the calculation to produce integer fold IDs in range [0, n_folds-1] (e.g. using integer block assignment like floor or integer division) so groupby sees exactly n_folds groups. Update the branch for self.split_method == "continuous" (the return that uses cp.arange(n_samples) / (n_samples / n_folds)) to compute integer fold indices (e.g. use cp.floor(...).astype(int) or (cp.arange(n_samples) * n_folds) // n_samples) so the returned array contains integer fold IDs.
🤖 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/_target_encoder.py`:
- Around line 514-516: The warnings.warn call inside the TargetEncoder logic (in
_target_encoder.py where the message "Using `split_method='customize'` since
`fold_ids` are provided`" is emitted) should include stacklevel=2 so the warning
points at the caller rather than this internal helper; update that warnings.warn
invocation to pass stacklevel=2 (e.g., warnings.warn(<message>, stacklevel=2)).
---
Duplicate comments:
In `@python/cuml/cuml/preprocessing/_target_encoder.py`:
- Around line 630-649: The code currently catches any ValueError from
check_classification_targets and treats the target as continuous; change the
broad except to capture the exception (except ValueError as e) and only set
continuous = True when the exception message matches the specific
continuous-target signal (e.g., contains "Continuous" or the exact message
emitted by check_classification_targets for continuous labels); otherwise
re-raise the exception so malformed/multi-output errors are not misclassified.
Locate this logic around check_classification_targets, the continuous variable,
and the subsequent setting of self.target_type_ / self.classes_ to apply the
conditional re-raise behavior.
- Around line 526-527: The continuous split currently uses true division
producing fractional fold labels; change the calculation to produce integer fold
IDs in range [0, n_folds-1] (e.g. using integer block assignment like floor or
integer division) so groupby sees exactly n_folds groups. Update the branch for
self.split_method == "continuous" (the return that uses cp.arange(n_samples) /
(n_samples / n_folds)) to compute integer fold indices (e.g. use
cp.floor(...).astype(int) or (cp.arange(n_samples) * n_folds) // n_samples) so
the returned array contains integer fold IDs.
🪄 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: c215c338-fea6-4988-a25d-d70e3a5874d5
📒 Files selected for processing (1)
python/cuml/cuml/preprocessing/_target_encoder.py
|
/merge |
5 participants
_) for now. I consider deprecating/changing those to be outside the scope of this PR.Part of #7317. Part of #8002.