Use new input validation infrastructure for cuml.decomposition. - #8006
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:
📝 WalkthroughWalkthroughReplace bespoke array conversion and sparse helpers in IncrementalPCA, PCA, and TruncatedSVD with unified validation ( Changes
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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/decomposition/incremental_pca.py`:
- Around line 277-283: The code resets input metadata unconditionally using
n_samples_seen_, _set_output_type and check_features even when check_input is
False; wrap the initial metadata setup so it only runs if check_input is True:
compute first_call and call self._set_output_type(X) and check_features(self, X,
reset=first_call) inside an if check_input: block (or ensure check_features is
invoked with reset=False when check_input is False). Update the partial_fit/fit
flow to preserve the original reflected input type and feature_names_in_ by
avoiding these resets for internal CuPy batches.
- Around line 222-228: In fit() the call to check_array strips pandas/cuDF
feature names before check_features(..., reset=True) runs, so feature_names_in_
and n_features_in_ never get set; fix by capturing feature names and resetting
feature metadata before coercion—either call check_features(self, X, reset=True)
before check_array or switch to check_inputs(X, accept_sparse=..., dtype=...,
convert_dtype=..., reset=True) so names are preserved, and ensure fit() assigns
n_features_in_ and feature_names_in_ (the learned attributes) after validation;
update references to check_array, check_features, check_inputs,
feature_names_in_, and n_features_in_ accordingly.
🪄 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: Pro Plus
Run ID: f36de441-99a0-4085-8c87-f74bfad41870
📒 Files selected for processing (6)
python/cuml/cuml/decomposition/incremental_pca.pypython/cuml/cuml/decomposition/pca.pyxpython/cuml/cuml/decomposition/tsvd.pyxpython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_incremental_pca.pypython/cuml/tests/test_sklearn_compatibility.py
💤 Files with no reviewable changes (3)
- python/cuml/tests/test_incremental_pca.py
- python/cuml/tests/test_sklearn_compatibility.py
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
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/cuml/cuml/decomposition/incremental_pca.py (1)
431-439:⚠️ Potential issue | 🟡 MinorPotential
AttributeErrorwhentransformis called on sparse data afterpartial_fit-only usage.
batch_size_is set only infit()(line 236-238), not inpartial_fit(). If a user trains exclusively viapartial_fit()calls and then callstransform()on sparse input, line 435 will raiseAttributeError: 'IncrementalPCA' object has no attribute 'batch_size_'.Consider falling back to a default batch size when
batch_size_is not set:Suggested fix
+ batch_size = getattr(self, "batch_size_", None) + if batch_size is None: + batch_size = 5 * X.shape[1] for batch in _gen_batches( n_samples, - self.batch_size_, + batch_size, min_batch_size=self.n_components or 0, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/decomposition/incremental_pca.py` around lines 431 - 439, transform()'s sparse-path uses self.batch_size_ which is only set in fit(), causing AttributeError when the user trained only via partial_fit(); update transform (sparse branch) to fall back to a default batch size when batch_size_ is missing (e.g., use getattr(self, "batch_size_", some_default) or compute a safe default from n_samples/n_components) before calling _gen_batches so _transform_sparse and _gen_batches receive a valid batch range; touch the transform method and references to batch_size_ (and possibly __init__ or partial_fit) to ensure consistency with partial_fit-only workflows and preserve min_batch_size=self.n_components or 0 behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@python/cuml/cuml/decomposition/incremental_pca.py`:
- Around line 431-439: transform()'s sparse-path uses self.batch_size_ which is
only set in fit(), causing AttributeError when the user trained only via
partial_fit(); update transform (sparse branch) to fall back to a default batch
size when batch_size_ is missing (e.g., use getattr(self, "batch_size_",
some_default) or compute a safe default from n_samples/n_components) before
calling _gen_batches so _transform_sparse and _gen_batches receive a valid batch
range; touch the transform method and references to batch_size_ (and possibly
__init__ or partial_fit) to ensure consistency with partial_fit-only workflows
and preserve min_batch_size=self.n_components or 0 behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fccc59e-4423-4549-aaec-8d4fd2e32323
📒 Files selected for processing (2)
python/cuml/cuml/decomposition/incremental_pca.pypython/cuml/tests/test_exceptions.py
jcrist
left a comment
There was a problem hiding this comment.
Looks good, thanks! Looks like a small merge conflict, but otherwise
!
| check_features(self, X, reset=first_call) | ||
|
|
||
| if check_input: | ||
| X = check_array(X, dtype=("float32", "float64")) |
There was a problem hiding this comment.
This check_input kwarg is odd, but it's what sklearn does too so 🤷. I do think there's a way to make this work with check_inputs instead of check_features + check_array, but what you have here is fine too.
|
/merge |
…ter partial_fit (#8010) Previously `transform` on sparse inputs accessed `self.batch_size_` directly, which is only set by `fit`, not `partial_fit`. Calling `transform` after `partial_fit` raised an `AttributeError`. Fall back to `self.batch_size` (or `5 * n_features` if unset) when `batch_size_` is not present, matching the behavior used in `fit`. Follow-up to #8006 Authors: - Simon Adorf (https://github.com/csadorf) Approvers: - Jim Crist-Harif (https://github.com/jcrist) URL: #8010
This applies the new input validation utilities added in #7973 to
cuml.decomposition.Doing this fixed ~9 failing sklearn compatibility tests for cuml proper, and at least 36 upstream tests for
cuml.accel.Fixes #7990
Part of #7428