Add sparse input support to ElasticNet/Lasso#7943
Open
jcrist wants to merge 4 commits intorapidsai:mainfrom
Open
Add sparse input support to ElasticNet/Lasso#7943jcrist wants to merge 4 commits intorapidsai:mainfrom
ElasticNet/Lasso#7943jcrist wants to merge 4 commits intorapidsai:mainfrom
Conversation
One test was duplicative and was just deleted. The other was moved unchanged.
jcrist
added
feature request
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds sparse matrix support to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 (1)
python/cuml/cuml/linear_model/lasso.py (1)
33-46:⚠️ Potential issue | 🟡 MinorClarify that
selectiononly affects coordinate descent.
solver='auto'now resolves toqnfor sparse inputs, but theselectionparagraph still reads as if it always changes fitting behavior. Please mirrorElasticNethere and state thatselectionis only used when the resolved solver is'cd'.As per coding guidelines, "Missing docstrings for public methods, undocumented hyperparameters, or missing scikit-learn compatibility notes in documentation must be addressed."
Also applies to: 125-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/linear_model/lasso.py` around lines 33 - 46, Update the Lasso docstring to clarify that the selection parameter only applies when the coordinate descent solver is used: explicitly state that when solver resolves to 'cd' (including when solver='auto' resolves to 'cd' for dense inputs) the selection option ('cyclic' or 'random') affects coefficient updates, and that selection is ignored when the resolved solver is 'qn' (including when solver='auto' resolves to 'qn' for sparse inputs); mirror the wording used in ElasticNet's docstring so the behavior is consistent and also update any other Lasso method docstring mentioning selection to the same wording.
🧹 Nitpick comments (1)
python/cuml/tests/test_elastic_net.py (1)
369-390: Please exercise at least one sparse-array input here.This only covers
scipy.sparse.csr_matrix, but the new code path is keyed off generic sparse detection and the same PR adds separate sparse-array / other sparse-format xfails upstream. Adding acsr_arraycase here would give us a local guardrail for the sparse-array path too.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 the current code and only fix it if needed. In `@python/cuml/tests/test_elastic_net.py` around lines 369 - 390, The test_sparse function currently only exercises scipy.sparse.csr_matrix; add a sparse-array case to exercise the new sparse-array code path by parameterizing or branching on an input_type and passing a scipy.sparse.csr_array into the training/prediction code. Concretely, update the test_sparse parametrize to include an input_type (e.g., ["csr_matrix", "csr_array"]) or add a small loop inside test_sparse that converts X to scipy.sparse.csr_array when the csr_array case is selected, then proceed to construct cu_model/sk_model, fit, and compare coef_/intercept_/score as before (leave test names and assertions unchanged) so the same numerical comparisons cover both csr_matrix and csr_array inputs.
🤖 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/linear_model/elastic_net.py`:
- Around line 30-35: The current SparseInputTagMixin together with is_sparse(X)
routes all scipy/cupyx sparse inputs into the QN/GPU path even though we still
mark some sparse cases as xfail (e.g., sample_weight on sparse inputs and
specific csc_*/lil_* cases in upstream/scikit-learn/xfail-list.yaml); update
ElasticNet's sparse handling to either (a) narrow the sparse contract by
changing the gating logic in SparseInputTagMixin/is_sparse(X) so only supported
sparse formats (e.g., CSR/CSC without sample_weight) are routed to the QN GPU
path, or (b) add explicit guards in ElasticNet (or the QN entrypoint) that
detect the unsupported combinations (sparse sample_weight, csc/lil formats) and
fall back to the CPU/scikit-learn code path; reference and modify the
SparseInputTagMixin, is_sparse(X) checks and the ElasticNet QN dispatch to
ensure parity with scikit-learn edge-case behavior and avoid sending xfailed
cases to the GPU path.
---
Outside diff comments:
In `@python/cuml/cuml/linear_model/lasso.py`:
- Around line 33-46: Update the Lasso docstring to clarify that the selection
parameter only applies when the coordinate descent solver is used: explicitly
state that when solver resolves to 'cd' (including when solver='auto' resolves
to 'cd' for dense inputs) the selection option ('cyclic' or 'random') affects
coefficient updates, and that selection is ignored when the resolved solver is
'qn' (including when solver='auto' resolves to 'qn' for sparse inputs); mirror
the wording used in ElasticNet's docstring so the behavior is consistent and
also update any other Lasso method docstring mentioning selection to the same
wording.
---
Nitpick comments:
In `@python/cuml/tests/test_elastic_net.py`:
- Around line 369-390: The test_sparse function currently only exercises
scipy.sparse.csr_matrix; add a sparse-array case to exercise the new
sparse-array code path by parameterizing or branching on an input_type and
passing a scipy.sparse.csr_array into the training/prediction code. Concretely,
update the test_sparse parametrize to include an input_type (e.g.,
["csr_matrix", "csr_array"]) or add a small loop inside test_sparse that
converts X to scipy.sparse.csr_array when the csr_array case is selected, then
proceed to construct cu_model/sk_model, fit, and compare coef_/intercept_/score
as before (leave test names and assertions unchanged) so the same numerical
comparisons cover both csr_matrix and csr_array inputs.
🪄 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
Run ID: a72e33dd-6a9a-49eb-81be-ea08f41b8910
📒 Files selected for processing (8)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/linear_model.pypython/cuml/cuml/linear_model/elastic_net.pypython/cuml/cuml/linear_model/lasso.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_elastic_net.pypython/cuml/tests/test_exceptions.pypython/cuml/tests/test_linear_model.py
💤 Files with no reviewable changes (3)
- python/cuml/tests/test_exceptions.py
- docs/source/cuml-accel/limitations.rst
- python/cuml/tests/test_linear_model.py
Comment on lines
+30
to
+35
| Base, | ||
| InteropMixin, | ||
| LinearPredictMixin, | ||
| RegressorMixin, | ||
| SparseInputTagMixin, | ||
| FMajorInputTagMixin, |
There was a problem hiding this comment.
The new sparse contract is broader than the behavior we still xfail.
SparseInputTagMixin plus is_sparse(X) now routes any scipy/cupyx sparse input through QN, but this same PR still adds xfails for sparse sample_weight equivalence and several csc_* / lil_* cases in python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml. Please canonicalize or gate the unsupported sparse formats/combinations here, or narrow the advertised sparse contract, before defaulting all sparse inputs into the GPU path.
As per coding guidelines, "Function and parameter names or defaults must match scikit-learn without justification; behavior for edge cases (empty arrays, single sample) must match scikit-learn."
Also applies to: 249-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuml/cuml/linear_model/elastic_net.py` around lines 30 - 35, The
current SparseInputTagMixin together with is_sparse(X) routes all scipy/cupyx
sparse inputs into the QN/GPU path even though we still mark some sparse cases
as xfail (e.g., sample_weight on sparse inputs and specific csc_*/lil_* cases in
upstream/scikit-learn/xfail-list.yaml); update ElasticNet's sparse handling to
either (a) narrow the sparse contract by changing the gating logic in
SparseInputTagMixin/is_sparse(X) so only supported sparse formats (e.g., CSR/CSC
without sample_weight) are routed to the QN GPU path, or (b) add explicit guards
in ElasticNet (or the QN entrypoint) that detect the unsupported combinations
(sparse sample_weight, csc/lil formats) and fall back to the CPU/scikit-learn
code path; reference and modify the SparseInputTagMixin, is_sparse(X) checks and
the ElasticNet QN dispatch to ensure parity with scikit-learn edge-case behavior
and avoid sending xfailed cases to the GPU path.
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
2 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:
ElasticNetandLasso, based on the existingQNsolver. To accomplish this, we change the default ofsolverto'auto', which will use'cd'when dense and'qn'when sparse.ElasticNetandLassointest_elastic_net.py. Previously these were duplicated and spread among a few files.cuml.accelintegration and docs accordingly. Most new xfails are due to numerical equivalences or lack of support fordual_gap_.Fixes #7912.