Consolidate sparse utilities - #8288
Merged
Merged
Conversation
📝 WalkthroughWalkthroughSparse helpers are consolidated into ChangesSparse utility consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🤖 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/common/sparse.py`:
- Around line 44-57: Preserve the CSR index width throughout the CUDA sparse
kernels: the row-offset and column-index parameters in the normalizer and
Gram/Covariance paths are currently truncated to int, which breaks int64 CSR
inputs and large nnz offsets. Update the kernels behind the normalizers,
compute_gram, and the L2 normalization path to use the same dtype as
X.indptr/X.indices for indptr and indices, and make the loop counters/offset
arithmetic match that wider type instead of hard-coded int. Keep the fix
consistent across the affected sparse kernel helpers so row boundaries and
column access remain correct for large inputs.
In `@python/cuml/cuml/decomposition/pca.pyx`:
- Line 431: The sparse PCA path in `PCA._fit_sparse` is using
`sparse_cov_and_mean(X)`, which returns population covariance and breaks the
variance/singular value scaling expected by `explained_variance_`,
`singular_values_`, and `noise_variance_`. Update the sparse covariance
computation or its caller so `_fit_sparse` receives sample covariance normalized
by `n_samples - 1`, matching the dense PCA/sklearn behavior; keep the fix
localized around `sparse_cov_and_mean` and the `_fit_sparse` assignment.
In `@python/cuml/tests/test_sparse_utils.py`:
- Around line 49-61: The test_sparse_cov_and_mean case only checks the
covariance result from sparse_cov_and_mean, leaving the mean output unvalidated.
Update this test to also compute the expected mean from X.todense() (matching
the existing ddof/rowvar settings as applicable) and assert the returned mean
matches it with an appropriate tolerance, using the sparse_cov_and_mean symbol
and the existing res, _ unpacking.
🪄 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: 7c967ebc-cb7a-40ee-8ddf-53d5e0ba30b9
📒 Files selected for processing (29)
python/cuml/cuml/_thirdparty/sklearn/preprocessing/_data.pypython/cuml/cuml/accel/_overrides/sklearn/linear_model.pypython/cuml/cuml/accel/_overrides/sklearn/manifold.pypython/cuml/cuml/accel/_overrides/sklearn/neighbors.pypython/cuml/cuml/accel/estimator_proxy.pypython/cuml/cuml/common/sparse.pypython/cuml/cuml/common/sparse_utils.pypython/cuml/cuml/common/sparsefuncs.pypython/cuml/cuml/dask/linear_model/logistic_regression.pypython/cuml/cuml/decomposition/incremental_pca.pypython/cuml/cuml/decomposition/pca.pyxpython/cuml/cuml/feature_extraction/_tfidf.pypython/cuml/cuml/feature_extraction/_vectorizers.pypython/cuml/cuml/internals/array.pypython/cuml/cuml/linear_model/elastic_net.pypython/cuml/cuml/manifold/t_sne.pyxpython/cuml/cuml/manifold/umap/umap.pyxpython/cuml/cuml/manifold/utils.pypython/cuml/cuml/neighbors/nearest_neighbors.pyxpython/cuml/cuml/prims/__init__.pypython/cuml/cuml/prims/stats/__init__.pypython/cuml/cuml/prims/stats/covariance.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml/svm/svm_base.pyxpython/cuml/cuml/svm/svr.pypython/cuml/cuml/thirdparty_adapters/sparsefuncs_fast.pypython/cuml/tests/test_metrics.pypython/cuml/tests/test_sparse_utils.pypython/cuml/tests/test_stats.py
💤 Files with no reviewable changes (6)
- python/cuml/cuml/prims/init.py
- python/cuml/tests/test_stats.py
- python/cuml/cuml/prims/stats/init.py
- python/cuml/cuml/prims/stats/covariance.py
- python/cuml/cuml/common/sparsefuncs.py
- python/cuml/cuml/common/sparse_utils.py
dantegd
requested changes
Jun 24, 2026
dantegd
left a comment
dantegd
left a comment
Contributor
There was a problem hiding this comment.
Two small questions, otherwise the changes look good to me
Moves this function into the only module it's actually used in. This function was too specific to `CountVectorizer` to make sense outside of this module.
This module contained only one tiny function.
Also revert workaround for cupyx.scipy.sparse matrix multiplication on cupy >= 14.
There's still a bug here.
Fixes a longstanding bug in our sparse PCA implementation, where the denominator was `n_samples` instead of `n_samples - 1`. I've updated the tests to catch this issue, and fixed the bug. Also fixed a few other asks in `cuml.common.sparse` from review.
jcrist
force-pushed
the
consolidate-sparse
branch
from
June 24, 2026 18:30
f062d05 to
915d24e
Compare
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/tests/test_pca.py`:
- Line 51: The PCA test is comparing components_ directly, which fails when the
valid solution differs only by sign. Update test_pca.py around the PCA
assertions to make the components_ check sign-invariant again, using the
existing PCA result objects (res and sol) so either matching sign is accepted,
while leaving the direct comparisons for sign-stable attributes 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: 9799f74c-1894-47cb-b794-b8454962f236
📒 Files selected for processing (33)
python/cuml/cuml/_thirdparty/sklearn/preprocessing/_data.pypython/cuml/cuml/accel/_overrides/sklearn/linear_model.pypython/cuml/cuml/accel/_overrides/sklearn/manifold.pypython/cuml/cuml/accel/_overrides/sklearn/neighbors.pypython/cuml/cuml/accel/estimator_proxy.pypython/cuml/cuml/common/sparse.pypython/cuml/cuml/common/sparse_utils.pypython/cuml/cuml/common/sparsefuncs.pypython/cuml/cuml/dask/linear_model/logistic_regression.pypython/cuml/cuml/decomposition/incremental_pca.pypython/cuml/cuml/decomposition/pca.pyxpython/cuml/cuml/feature_extraction/_tfidf.pypython/cuml/cuml/feature_extraction/_vectorizers.pypython/cuml/cuml/internals/array.pypython/cuml/cuml/linear_model/elastic_net.pypython/cuml/cuml/manifold/t_sne.pyxpython/cuml/cuml/manifold/umap/umap.pyxpython/cuml/cuml/manifold/utils.pypython/cuml/cuml/neighbors/nearest_neighbors.pyxpython/cuml/cuml/prims/__init__.pypython/cuml/cuml/prims/stats/__init__.pypython/cuml/cuml/prims/stats/covariance.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml/svm/svm_base.pyxpython/cuml/cuml/svm/svr.pypython/cuml/cuml/thirdparty_adapters/sparsefuncs_fast.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_adapters.pypython/cuml/tests/test_metrics.pypython/cuml/tests/test_pca.pypython/cuml/tests/test_sparse_utils.pypython/cuml/tests/test_sparsefuncs.pypython/cuml/tests/test_stats.py
💤 Files with no reviewable changes (9)
- python/cuml/cuml/prims/stats/init.py
- python/cuml/cuml/prims/init.py
- python/cuml/tests/test_sparsefuncs.py
- python/cuml/tests/test_stats.py
- python/cuml/tests/test_adapters.py
- python/cuml/cuml/common/sparsefuncs.py
- python/cuml/cuml/common/sparse_utils.py
- python/cuml/cuml/prims/stats/covariance.py
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
✅ Files skipped from review due to trivial changes (6)
- python/cuml/cuml/svm/svr.py
- python/cuml/cuml/decomposition/incremental_pca.py
- python/cuml/cuml/linear_model/elastic_net.py
- python/cuml/cuml/svm/svc.py
- python/cuml/cuml/manifold/utils.py
- python/cuml/cuml/manifold/umap/umap.pyx
🚧 Files skipped from review as they are similar to previous changes (15)
- python/cuml/cuml/svm/svm_base.pyx
- python/cuml/cuml/feature_extraction/_tfidf.py
- python/cuml/cuml/accel/_overrides/sklearn/linear_model.py
- python/cuml/cuml/internals/array.py
- python/cuml/cuml/accel/estimator_proxy.py
- python/cuml/cuml/manifold/t_sne.pyx
- python/cuml/cuml/_thirdparty/sklearn/preprocessing/_data.py
- python/cuml/tests/test_metrics.py
- python/cuml/cuml/accel/_overrides/sklearn/neighbors.py
- python/cuml/cuml/accel/_overrides/sklearn/manifold.py
- python/cuml/cuml/neighbors/nearest_neighbors.pyx
- python/cuml/cuml/decomposition/pca.pyx
- python/cuml/cuml/dask/linear_model/logistic_regression.py
- python/cuml/cuml/thirdparty_adapters/sparsefuncs_fast.py
- python/cuml/cuml/feature_extraction/_vectorizers.py
Member
Author
|
The bot actually pointed out a long standing bug, which I've now fixed. I've updated the top-level comment accordingly with the new changes. I believe this should be good-to-go. |
dantegd
approved these changes
Jun 24, 2026
Member
Author
|
/merge |
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 was motivated as a followup to some recent cleanup work (#8272 and #8269), aiming to consolidate some modules and delete some dead code, but in the process fixed a few bugs and added one feature.
Highlights a user would care about
PCAon sparse inputs submitted in COO format.PCA. Fixes Support large sparse matrices inPCA#8159.PCAwhere the incorrectddofwas used when computing the covariance.Details a maintainer would care about
cuml.common.sparse_utilsandcuml.common.sparsefuncsintocuml.common.sparsecreate_csr_matrix_from_count_dftocuml.feature_extraction._vectorizers, the only module it's used in.cuml.prims.stats.covtocuml.common.sparse(keeping only the sparse version, as that's the only version we use), and removes the remaining emptycuml.primsmodule.cuml.common.statscsr_row_normalize_l1andcsr_row_normalize_l2fromcuml.thirdparty_adapters. I ran some tests and benchmarks just to be sure - the versions incuml.common.sparseusingcupyare consistently faster (~15%) and yield equivalent results. Less code to manage, and lessnumba.cudausage. We're almost to a point where this won't be a required dep either.covfor COO matrices. This wasn't caught with the previous test, but was caught with a more thorough one on denser data. I spent some time trying to fix the kernel, but decided that relying on the single CSR kernel implementation was simpler, more correct, and as performant.covto rely onX.T.dot(X)instead incupy >= 14. Removing the fallback in that case resulted in a nice speedup on small inputs, but when tested on larger data I ran into issues - unfortunately there still appears to be a bug there. I opened Incorrect results for sparse gemm cupy/cupy#10033, and updated the inline comment to note the new issue.ddofwas used when computingcov. I've improved the test suite to catch the issue going forward. This also fixed 300+ xfails in the sklearn compat test suite.PCA. This was relatively straightforward to do. Like other cases, this is hard to test in CI (cupy rightfully normalizes indices as int32 when possible), but I can confirm things work properly from local testing.There are still some dispersed sparse utilities in
cuml.thirdparty_adapters/cuml._thirdparty.sklearn.utils.sparsefuncs. In the interest of time I opted to skip these for now (sans relying on any implementations incuml.common.sparseinstead, as noted above). Further migration work may happen in followup(s).