A few validation fixups - #8066
Conversation
jcrist
added
improvement
|
|
||
|
|
||
| @reflect | ||
| def spectral_embedding( |
There was a problem hiding this comment.
Functions like this should be based on the estimator (not the estimator based on them). This eases validation and avoids double processing.
I only moved the definition down after the class for readability (since the function depends on the class, not other way around).
| @pytest.mark.filterwarnings("ignore:The number of bins.*:UserWarning") | ||
| @pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning") | ||
| def test_sklearn_compatible_estimator(estimator, check): | ||
| # Check that all estimators pass the "common estimator" checks |
There was a problem hiding this comment.
With this we no longer have any skipped tests, no more known crashes!
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR centralizes input validation using check_inputs/check_array across SpectralEmbedding, UMAP, and GaussianNB; moves affinity preprocessing into SpectralEmbedding.fit with a thin module-level wrapper; updates pointer extraction and return wrappers; and tightens related tests' error-message assertions. ChangesSpectralEmbedding Refactoring and Function Wrapping
UMAP Input Validation Modernization
GaussianNB Priors Validation Enhancement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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/manifold/umap/umap.pyx (1)
1884-1957:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame memory type concern applies here.
Similar to
fuzzy_simplicial_set, this function callscheck_arraywithoutmem_type(lines 1884, 1926) and then accesses.data.ptr(lines 1955-1956). This will fail if numpy arrays are returned. Apply the same fix as recommended above.🤖 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/manifold/umap/umap.pyx` around lines 1884 - 1957, The check_array calls that produce X and embedding must request device (CuPy) memory so subsequent .data.ptr access is valid; update the two check_array invocations (the one that assigns X, index and the one that validates init/creates embedding) to include mem_type="device" (or the project's equivalent device mem_type constant) so they return cupy arrays, then keep using embedding.data.ptr and X.data.ptr for RaftCOO.from_cupy_coo and downstream code.python/cuml/cuml/manifold/spectral_embedding.pyx (1)
229-237:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
ensure_min_features=2to match scikit-learn validation.scikit-learn's
SpectralEmbedding.fit()calls_validate_data()with onlyensure_min_samples=2and no feature minimum, allowing single-feature inputs (n_features=1). Thenearest_neighborsaffinity usesNearestNeighborswhich supports 1D data, andrbfaffinity works with single features. cuML'sensure_min_features=2rejects valid inputs that sklearn accepts, violating sklearn parity. Change toensure_min_features=1.Suggested change
X = check_inputs( self, X, dtype="float32", order="C", accept_sparse="coo" if self.affinity == "precomputed" else False, ensure_min_samples=2, - ensure_min_features=2, + ensure_min_features=1, reset=True, )🤖 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/manifold/spectral_embedding.pyx` around lines 229 - 237, The SpectralEmbedding.fit input validation currently forces ensure_min_features=2 in the check_inputs call, which rejects valid single-feature inputs; update the check_inputs invocation in SpectralEmbedding.fit (the call named check_inputs) to remove ensure_min_features=2 or change it to ensure_min_features=1 so it matches scikit-learn behavior and allows n_features=1 (keep other args like ensure_min_samples=2 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/manifold/spectral_embedding.pyx`:
- Around line 247-274: In the precomputed affinity branch of
spectral_embedding.pyx (the block handling affinity == "precomputed"), add a
check that the input sparse matrix X is square (X.shape[0] == X.shape[1]) before
extracting COO indices (affinity_rows, affinity_cols, affinity_data) and
computing affinity_nnz; if not square raise a clear ValueError explaining that
precomputed affinity must be n_samples × n_samples so the downstream
transform(...) overload that expects row/col/value vectors and n_samples cannot
receive invalid column indices. Update the control flow so the diagonal-removal
and pointer extraction (affinity_data_ptr, affinity_rows_ptr, affinity_cols_ptr)
happen only after the square check, and add a small regression test that passes
a rectangular precomputed matrix to spectral_embedding to assert it raises the
new ValueError.
---
Outside diff comments:
In `@python/cuml/cuml/manifold/spectral_embedding.pyx`:
- Around line 229-237: The SpectralEmbedding.fit input validation currently
forces ensure_min_features=2 in the check_inputs call, which rejects valid
single-feature inputs; update the check_inputs invocation in
SpectralEmbedding.fit (the call named check_inputs) to remove
ensure_min_features=2 or change it to ensure_min_features=1 so it matches
scikit-learn behavior and allows n_features=1 (keep other args like
ensure_min_samples=2 unchanged).
In `@python/cuml/cuml/manifold/umap/umap.pyx`:
- Around line 1884-1957: The check_array calls that produce X and embedding must
request device (CuPy) memory so subsequent .data.ptr access is valid; update the
two check_array invocations (the one that assigns X, index and the one that
validates init/creates embedding) to include mem_type="device" (or the project's
equivalent device mem_type constant) so they return cupy arrays, then keep using
embedding.data.ptr and X.data.ptr for RaftCOO.from_cupy_coo and downstream code.
🪄 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: 095ceb19-1119-4e25-8c28-dc0d7a18580b
📒 Files selected for processing (6)
python/cuml/cuml/manifold/spectral_embedding.pyxpython/cuml/cuml/manifold/umap/umap.pyxpython/cuml/cuml/naive_bayes/naive_bayes.pypython/cuml/tests/test_sklearn_compatibility.pypython/cuml/tests/test_spectral_embedding.pypython/cuml/tests/test_umap.py
💤 Files with no reviewable changes (1)
- python/cuml/tests/test_sklearn_compatibility.py
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/manifold/spectral_embedding.pyx (1)
229-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChange
ensure_min_features=2toensure_min_features=1to allow single-feature inputs.scikit-learn's
SpectralEmbeddingwithaffinity="nearest_neighbors"accepts input arrays of shape(n_samples, 1). The current validation requirement of 2 features rejects valid inputs and introduces a regression from sklearn-compatible behavior.Suggested fix
X = check_inputs( self, X, dtype="float32", order="C", accept_sparse="coo" if self.affinity == "precomputed" else False, ensure_min_samples=2, - ensure_min_features=2, + ensure_min_features=1, reset=True, )🤖 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/manifold/spectral_embedding.pyx` around lines 229 - 236, The input validation in spectral_embedding uses check_inputs with ensure_min_features=2 which incorrectly rejects single-feature arrays; update the call in spectral_embedding (the check_inputs invocation inside spectral_embedding.pyx) to use ensure_min_features=1 so inputs of shape (n_samples, 1) are accepted, preserving sklearn-compatible behavior for affinity="nearest_neighbors".
🤖 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/cuml/cuml/manifold/spectral_embedding.pyx`:
- Around line 229-236: The input validation in spectral_embedding uses
check_inputs with ensure_min_features=2 which incorrectly rejects single-feature
arrays; update the call in spectral_embedding (the check_inputs invocation
inside spectral_embedding.pyx) to use ensure_min_features=1 so inputs of shape
(n_samples, 1) are accepted, preserving sklearn-compatible behavior for
affinity="nearest_neighbors".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8a770bab-edfb-4295-a29e-61f9e9be055e
📒 Files selected for processing (6)
python/cuml/cuml/manifold/spectral_embedding.pyxpython/cuml/cuml/manifold/umap/umap.pyxpython/cuml/cuml/naive_bayes/naive_bayes.pypython/cuml/tests/test_sklearn_compatibility.pypython/cuml/tests/test_spectral_embedding.pypython/cuml/tests/test_umap.py
💤 Files with no reviewable changes (1)
- python/cuml/tests/test_sklearn_compatibility.py
✅ Files skipped from review due to trivial changes (1)
- python/cuml/cuml/naive_bayes/naive_bayes.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/cuml/manifold/umap/umap.pyx
| if len(X.shape) != 2: | ||
| raise ValueError("Reshape your data: data should be two dimensional") | ||
| # Normalize X as cheaply as possible to minimize copies and work | ||
| X, index = check_inputs( |
There was a problem hiding this comment.
We never want to touch X (if we can avoid it) before it's validated.
The logic here is updated to do a cheap pre-validation pass of X, which should normalize it to a numpy/cupy type with minimal-to-no copies or extra work.
We then determine the required mem_type, and after that do a full validation and coercion pass.
This minimizes work, while keeping the logic readable and relying on standard tools.
| # Determine the required mem_type based on params and X | ||
| if X_is_sparse: | ||
| mem_type = base_mem_type | ||
| mem_type = "device" |
There was a problem hiding this comment.
The old logic didn't do what the comment said (coerce sparse inputs to device). Things were only saved by SparseCumlArray coercing to device memory implicitly later on.
| BernoulliNB(), | ||
| MultinomialNB(), | ||
| UMAP(), | ||
| UMAP(n_neighbors=5), |
There was a problem hiding this comment.
The data sizes here can be small, the default of n_neighbors=15 would work but lead to some warnings in the tests. Lowering it silences the warnings, same as done for *RandomProjection.
|
/merge |
3 participants
Removes some lingering old
input_to_*/CumlArray/SparseCumlArraycalls in some modules that were already addressed. These just slipped through the cracks in review.Fixes #7997.