Add LSMR solver to LinearRegression - #7927
Merged
rapids-bot[bot] merged 8 commits intoApr 1, 2026
Merged
Conversation
jcrist
added
feature request
|
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:
📝 WalkthroughWalkthroughRemoved CPU-fallback for sparse X in docs; added a shared GPU least-squares implementation ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 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/linear_model/base.py`:
- Around line 205-210: The kernel _ridge_transform_zero_alpha currently returns
x when abs(y) < 1e-10, which preserves components corresponding to near-zero
singular values; change its behavior to zero those components instead (set z = 0
when abs(y) < 1e-10) so it matches the pseudo-inverse semantics and is
consistent with the behavior in _ridge_transform; update the ElementwiseKernel
body in _ridge_transform_zero_alpha to assign z = 0 for the near-zero branch and
keep z = x / y otherwise, ensuring the same numeric threshold and symbol names
(_ridge_transform_zero_alpha, _ridge_transform) are used for clarity.
In `@python/cuml/cuml/linear_model/linear_regression.pyx`:
- Around line 366-368: The fallback message is incorrect: when checking
X_m.shape[0] == 1 (single row/sample) the code sets fallback_reason =
"single-column X"; change the string to accurately reflect a single row/sample
(e.g., "single-row X" or "single-sample X") so that the variables solver and
fallback_reason in linear_regression.pyx (around the X_m.shape[0] == 1 branch)
correctly describe the condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc082eb9-328e-4968-a380-3dca6814772c
📒 Files selected for processing (8)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/linear_model/base.pypython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml/linear_model/ridge.pyxpython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_exceptions.pypython/cuml/tests/test_linear_model.pypython/cuml/tests/test_sklearn_compatibility.py
💤 Files with no reviewable changes (3)
- docs/source/cuml-accel/limitations.rst
- python/cuml/tests/test_sklearn_compatibility.py
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
jcrist
force-pushed
the
linear-regression-improvements
branch
2 times, most recently
from
March 24, 2026 21:02
59b1fda to
f5d112f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuml/cuml/linear_model/linear_regression.pyx (1)
313-316: Consider validating sparse input dtype for consistency.The sparse input path only checks
dtype.kind != "f"to decide on conversion, which would allow float16 through unchanged. The dense path explicitly validatescheck_dtype=[np.float32, np.float64]. For consistency and to avoid potential downstream issues with unsupported dtypes, consider validating the sparse dtype similarly.♻️ Suggested change
if X_is_sparse := is_sparse(X): + if X.dtype not in (np.float32, np.float64): + convert_dtype = np.float32 + else: + convert_dtype = None X_m = SparseCumlArray( - X, convert_to_dtype=np.float32 if X.dtype.kind != "f" else None + X, convert_to_dtype=convert_dtype )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/linear_model/linear_regression.pyx` around lines 313 - 316, The sparse-input branch currently only checks dtype.kind != "f" and thus lets float16 through; update the handling around is_sparse and SparseCumlArray creation to validate sparse X dtype like the dense path (allowed check_dtype = [np.float32, np.float64]) and convert any unsupported sparse dtypes (e.g., float16 or non-floats) to np.float32 before constructing X_m; ensure the same validation logic/utility used for dense paths (or replicate the check) is applied so X_m is always float32/float64 consistent with the rest of the pipeline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/cuml/cuml/linear_model/linear_regression.pyx`:
- Around line 313-316: The sparse-input branch currently only checks dtype.kind
!= "f" and thus lets float16 through; update the handling around is_sparse and
SparseCumlArray creation to validate sparse X dtype like the dense path (allowed
check_dtype = [np.float32, np.float64]) and convert any unsupported sparse
dtypes (e.g., float16 or non-floats) to np.float32 before constructing X_m;
ensure the same validation logic/utility used for dense paths (or replicate the
check) is applied so X_m is always float32/float64 consistent with the rest of
the pipeline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1eb69be7-0de0-46bb-a4da-c4e55535f4cc
📒 Files selected for processing (6)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_exceptions.pypython/cuml/tests/test_linear_model.pypython/cuml/tests/test_sklearn_compatibility.py
💤 Files with no reviewable changes (3)
- docs/source/cuml-accel/limitations.rst
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
- python/cuml/tests/test_sklearn_compatibility.py
✅ Files skipped from review due to trivial changes (1)
- python/cuml/tests/test_exceptions.py
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/base.py`:
- Around line 273-324: Normalize and validate alpha before the solver branches:
convert any array-like alpha to a cupy array of shape (y.shape[1],) with dtype
X.dtype (use cp.asarray and reshape/broadcast as needed), reject negative values
(raise ValueError) and ensure scalar inputs are expanded to length n_targets;
then use this validated alpha for the SVD branch checks ((alpha == 0).all()) and
for computing damp = cp.sqrt(alpha) in the "lsmr" branch so sqrt gets only
non-negative cupy arrays. Reference symbols: alpha, X, y, solver,
_ridge_transform/_ridge_transform_zero_alpha, damp, and the "lsmr" branch loop.
In `@python/cuml/cuml/linear_model/linear_regression.pyx`:
- Around line 313-316: The sparse branch currently forces non-floating sparse
inputs to float32 and lets float16 bypass dense checks; update the X_is_sparse
handling so it respects the convert_dtype flag and uses the same dense dtype
contract as the dense path: when convert_dtype is False preserve X.dtype (i.e.,
pass convert_to_dtype=None into SparseCumlArray), and when convert_dtype is True
choose the promoted floating dtype using the same logic used for dense inputs
(promote to float32/float64 according to dtype.kind and the dense-path rules),
also ensure float16 is handled consistently with dense conversion; locate
X_is_sparse, SparseCumlArray and mirror the dense conversion decision (or call
the common conversion helper used for dense arrays) rather than unconditionally
passing np.float32.
- Around line 359-384: Before selecting a solver in the solver-determination
block, validate self.algorithm against the allowed values (e.g. "auto",
"libcuml", "lsmr", "svd") and raise a ValueError for any other string so typos
like algorithm="eg" error out early; add this check at the top of the method
containing the shown logic (the same scope that references self.algorithm and
_fit_libcuml()) so fallback paths (sparse X, single-column X, multi-column y) no
longer silently accept invalid algorithm values and preserve existing behavior
for valid values.
- Around line 329-333: The code currently raises for n_rows == 1 (checking
X_m.shape[0] < 2) which breaks compatibility with scikit-learn; instead remove
the hard raise and, when X_m.shape[0] == 1 or when the libcuml path cannot
handle a single sample, route execution to the file's existing SVD fallback path
used for unstable libcuml cases (i.e., replace the raise with a conditional that
invokes the SVD fallback routine for the single-row case or when libcuml signals
inability). Ensure you use the same inputs (X_m, n_rows, etc.) when calling the
fallback so behavior matches scikit-learn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aca308e7-8fbb-4e78-bd0a-9d5cfcf97803
📒 Files selected for processing (7)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/linear_model/base.pypython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_exceptions.pypython/cuml/tests/test_linear_model.pypython/cuml/tests/test_sklearn_compatibility.py
💤 Files with no reviewable changes (3)
- docs/source/cuml-accel/limitations.rst
- python/cuml/tests/test_sklearn_compatibility.py
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
✅ Files skipped from review due to trivial changes (1)
- python/cuml/tests/test_exceptions.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/tests/test_linear_model.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
python/cuml/cuml/linear_model/linear_regression.pyx (1)
313-323:⚠️ Potential issue | 🟠 MajorSparse
Xstill ignoresconvert_dtype=False.Unlike the dense path in Lines 319-324, Line 315 always casts non-floating sparse inputs to
float32, andfloat16also slips past the dense branch’sfloat32/float64gate. That makes sparse fits silently diverge from the dense contract and can feed unsupported dtypes intofit_least_squares.🔧 Suggested fix
if X_is_sparse := is_sparse(X): + allowed_dtypes = (np.dtype(np.float32), np.dtype(np.float64)) + x_dtype = np.dtype(X.dtype) X_m = SparseCumlArray( - X, convert_to_dtype=np.float32 if X.dtype.kind != "f" else None + X, + convert_to_dtype=( + np.float32 + if convert_dtype and x_dtype not in allowed_dtypes + else None + ), ) + if np.dtype(X_m.dtype) not in allowed_dtypes: + raise TypeError( + "X must have dtype float32 or float64 when convert_dtype=False" + ) X_is_copy = FalseA small regression test for sparse
int32/float16with bothconvert_dtypesettings would help lock this down. As per coding guidelines, "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/linear_regression.pyx` around lines 313 - 323, The sparse branch currently always casts non-floating inputs to float32 via SparseCumlArray(X, convert_to_dtype=np.float32 if X.dtype.kind != "f" else None), which ignores the convert_dtype flag and allows float16 through; change the SparseCumlArray invocation to mirror the dense path by using convert_to_dtype=(np.float32 if convert_dtype else None) and enforce check_dtype=[np.float32, np.float64] (matching input_to_cuml_array) so float16 is converted or rejected consistently, and add regression tests for sparse int32/float16 with both convert_dtype True/False; update references: X_is_sparse, SparseCumlArray, convert_dtype, input_to_cuml_array, and fit_least_squares.python/cuml/cuml/linear_model/base.py (1)
273-275:⚠️ Potential issue | 🟠 MajorNormalize non-scalar
alphabefore entering the solver branches.Only scalar
alphavalues are normalized today. A list/tuple still makes Line 280 evaluate to a plain bool, so.all()fails, and mismatched or negative arrays still flow through to Line 310 assqrt(alpha). Please coercealphato a 1D CuPy array of lengthy.shape[1]and reject negatives up front.🔧 Suggested fix
- # Normalize alpha to a cupy array of shape (n_targets,) - if cp.isscalar(alpha): - alpha = cp.full(y.shape[1], alpha, dtype=X.dtype) + # Normalize alpha to a cupy array of shape (n_targets,) + alpha = cp.asarray(alpha, dtype=X.dtype) + if alpha.ndim == 0: + alpha = cp.full(y.shape[1], alpha.item(), dtype=X.dtype) + else: + alpha = alpha.ravel() + if alpha.size != y.shape[1]: + raise ValueError( + f"alpha must have size {y.shape[1]} for {y.shape[1]} target(s), " + f"got {alpha.size}" + ) + if (alpha < 0).any(): + raise ValueError("alpha must be non-negative")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".
Also applies to: 280-285, 309-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/linear_model/base.py` around lines 273 - 275, Normalize and validate non-scalar alpha before entering solver branches: coerce alpha (if not a scalar) into a 1-D CuPy array of length y.shape[1] with dtype matching X.dtype (use cp.asarray(..., dtype=X.dtype) and ravel if needed), check that its shape matches y.shape[1], and raise ValueError for negative entries or length mismatch; then proceed to use cp.sqrt(alpha) in the solver. Apply these checks at the same place where scalar alpha is handled (around the current alpha normalization block) so downstream code (e.g., where cp.sqrt(alpha) is used) always receives a validated 1-D CuPy array.
🤖 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/base.py`:
- Around line 273-275: Normalize and validate non-scalar alpha before entering
solver branches: coerce alpha (if not a scalar) into a 1-D CuPy array of length
y.shape[1] with dtype matching X.dtype (use cp.asarray(..., dtype=X.dtype) and
ravel if needed), check that its shape matches y.shape[1], and raise ValueError
for negative entries or length mismatch; then proceed to use cp.sqrt(alpha) in
the solver. Apply these checks at the same place where scalar alpha is handled
(around the current alpha normalization block) so downstream code (e.g., where
cp.sqrt(alpha) is used) always receives a validated 1-D CuPy array.
In `@python/cuml/cuml/linear_model/linear_regression.pyx`:
- Around line 313-323: The sparse branch currently always casts non-floating
inputs to float32 via SparseCumlArray(X, convert_to_dtype=np.float32 if
X.dtype.kind != "f" else None), which ignores the convert_dtype flag and allows
float16 through; change the SparseCumlArray invocation to mirror the dense path
by using convert_to_dtype=(np.float32 if convert_dtype else None) and enforce
check_dtype=[np.float32, np.float64] (matching input_to_cuml_array) so float16
is converted or rejected consistently, and add regression tests for sparse
int32/float16 with both convert_dtype True/False; update references:
X_is_sparse, SparseCumlArray, convert_dtype, input_to_cuml_array, and
fit_least_squares.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ebeadf8-639f-4256-b8e7-ad939231e224
📒 Files selected for processing (2)
python/cuml/cuml/linear_model/base.pypython/cuml/cuml/linear_model/linear_regression.pyx
viclafargue
approved these changes
Mar 25, 2026
viclafargue
left a comment
viclafargue
left a comment
Contributor
There was a problem hiding this comment.
Thanks! The refactor seems correct to me, but additional reviews would be welcome.
jcrist
force-pushed
the
linear-regression-improvements
branch
from
March 31, 2026 19:45
ec5ebc0 to
3da0db7
Compare
jcrist
force-pushed
the
linear-regression-improvements
branch
from
March 31, 2026 21:10
3da0db7 to
cbfbda1
Compare
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 is a followup to #7922. It adds an LSMR solver to
LinearRegression. The primary motivation here is adding sparse input support, but the LSMR solver is also much faster than our existing solvers for most inputs. For now I kept the default behavior the same (except we use LSMR for sparse inputs) - in the future we might consider preferring LSMR over SVD.To accomplish this, I split the cupy-based solvers out of
cuml/linear_models/ridge.pyxintocuml/linear_models/base.pyas a standalonefit_least_squaresfunction. This feels a bit weird to have it there, but it's nice to have pure-python functions in a.pyfile since we get much better linting/formatting there than we do in cython files. I'm happy with this location for now. SinceLinearRegressionis effectively a special-case ofRidgewithalpha=0.0, sharing this functionality across the models makes sense.Fixes #3105.