Add sparse input support to ElasticNet/Lasso - #7943
Merged
Merged
Conversation
jcrist
added
feature request
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughElasticNet and Lasso were extended to handle sparse inputs: added sparse-input tagging and a reflected Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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
One test was duplicative and was just deleted. The other was moved unchanged.
jcrist
force-pushed
the
sparse-elastic-net
branch
from
April 6, 2026 16:35
84b564d to
21497bd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
python/cuml/cuml/linear_model/elastic_net.py (1)
249-275:⚠️ Potential issue | 🔴 CriticalFail fast on sparse
sample_weightuntil the QN path is correct.This branch now accepts
fit(X_sparse, y, sample_weight=...), but the same PR has to xfailcheck_sample_weight_equivalence_on_sparse_datafor bothElasticNetandLasso. That turns a previously unsupported combination into a silently wrong one. Please reject sparse weighted fits here (or route them through a verified CPU fallback) instead of sending them tofit_qn().🛡️ Proposed guard
- solver = self.solver - if solver == "auto": - solver = "qn" if is_sparse(X) else "cd" + is_sparse_x = is_sparse(X) + solver = self.solver + if solver == "auto": + solver = "qn" if is_sparse_x else "cd" + + if is_sparse_x and sample_weight is not None: + raise ValueError( + "sample_weight with sparse inputs is not supported yet; " + "use dense inputs or omit sample_weight" + ) if solver == "qn": coef, intercept, n_iter, _ = fit_qn( X, y, @@ - elif solver == "cd": - if is_sparse(X): + elif solver == "cd": + if is_sparse_x: raise ValueError(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" and "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 the current code and only fix it if needed. In `@python/cuml/cuml/linear_model/elastic_net.py` around lines 249 - 275, The code currently allows sparse sample_weight to be passed into the QN path (fit_qn) which can yield silently incorrect results; add a guard that detects a sparse sample_weight (using the same is_sparse check) before calling fit_qn (and when solver=="auto" that resolves to "qn") and raise a ValueError instructing the user that sparse sample_weight is unsupported for ElasticNet (or suggest using solver='cd' or a dense sample_weight/CPU fallback). Specifically, update the branch that sets solver (and the subsequent if solver == "qn" block) to check if sample_weight is not None and is_sparse(sample_weight) and then raise an explanatory ValueError prior to invoking fit_qn; reference the symbols solver, is_sparse, sample_weight, and fit_qn so the change is easy to locate.
🧹 Nitpick comments (1)
python/cuml/tests/test_elastic_net.py (1)
393-400: CoverLassoin the solver-error regression too.The new solver contract is public on both estimators, but this only exercises
ElasticNet. Parameterizing the test overcuml.Lassoas well will keepLasso.__init__/param forwarding from drifting away from the inheritedElasticNet.fit()checks.♻️ Proposed change
-def test_solver_errors(): +@pytest.mark.parametrize("cls", [cuml.ElasticNet, cuml.Lasso]) +def test_solver_errors(cls): X, y = make_sparse_regression() with pytest.raises(ValueError, match="solver='bad' is not supported"): - cuml.ElasticNet(solver="bad").fit(X, y) + cls(solver="bad").fit(X, y) with pytest.raises(ValueError, match="solver='cd' doesn't support sparse"): - cuml.ElasticNet(solver="cd").fit(X, y) + cls(solver="cd").fit(X, y)As per coding guidelines, "Update unit tests when making code changes".
🤖 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 393 - 400, The test only checks solver-related errors for cuml.ElasticNet but should also cover cuml.Lasso to ensure Lasso.__init__/parameter forwarding enforces the same solver contract; update test_solver_errors to parameterize the test over the estimator class (e.g., using a loop or pytest.mark.parametrize) and run the same two assertions (bad solver and cd-with-sparse) against both cuml.ElasticNet and cuml.Lasso so both estimators raise the same ValueErrors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@python/cuml/cuml/linear_model/elastic_net.py`:
- Around line 249-275: The code currently allows sparse sample_weight to be
passed into the QN path (fit_qn) which can yield silently incorrect results; add
a guard that detects a sparse sample_weight (using the same is_sparse check)
before calling fit_qn (and when solver=="auto" that resolves to "qn") and raise
a ValueError instructing the user that sparse sample_weight is unsupported for
ElasticNet (or suggest using solver='cd' or a dense sample_weight/CPU fallback).
Specifically, update the branch that sets solver (and the subsequent if solver
== "qn" block) to check if sample_weight is not None and
is_sparse(sample_weight) and then raise an explanatory ValueError prior to
invoking fit_qn; reference the symbols solver, is_sparse, sample_weight, and
fit_qn so the change is easy to locate.
---
Nitpick comments:
In `@python/cuml/tests/test_elastic_net.py`:
- Around line 393-400: The test only checks solver-related errors for
cuml.ElasticNet but should also cover cuml.Lasso to ensure
Lasso.__init__/parameter forwarding enforces the same solver contract; update
test_solver_errors to parameterize the test over the estimator class (e.g.,
using a loop or pytest.mark.parametrize) and run the same two assertions (bad
solver and cd-with-sparse) against both cuml.ElasticNet and cuml.Lasso so both
estimators raise the same ValueErrors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 404e88fb-b090-4374-b5dc-45440b3d45e9
📒 Files selected for processing (9)
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.pypython/cuml/tests/test_sklearn_compatibility.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
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cuml/cuml/accel/_overrides/sklearn/linear_model.py
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
Member
Author
|
/merge |
rapids-bot Bot
pushed a commit
that referenced
this pull request
Apr 8, 2026
- Splits out new test files for `LinearRegression` and `LogisticRegression`. All other linear models now have their own test files (from the old huge `test_linear_models.py`, these were the last two that still weren't split out. - Plumbs through missing `solver` arg in `test_max_iter_n_iter`, as noticed in #7943 (comment) Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Dante Gama Dessavre (https://github.com/dantegd) URL: #7957
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:
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.