Cleanup Lars, apply new validation - #8024
Merged
Merged
Conversation
jcrist
added
improvement
jcrist
added
the
sklearn-api-compat
📝 WalkthroughWalkthroughReplaces legacy preprocessing in the experimental LARS estimator with centralized input validation ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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/experimental/linear_model/lars.pyx`:
- Around line 309-325: Trim the coef_path array to match the actual number of
active steps before assigning to self.coef_path_: after you slice
active/beta/alphas to n_active, also slice coef_path to the documented shape
(n_alphas, n_alphas + 1) using n_active (i.e., take the first n_active rows and
first n_active+1 columns) and then wrap that into CumlArray when setting
self.coef_path_; keep the existing logic that sets self.coef_path_ to None if
coef_path is None.
- Around line 264-266: The code currently declares eps as a C float which forces
float64 values into float32 precision and uses the wrong machine epsilon for
float32 fits; change the logic to choose a dtype-specific epsilon based on
use_float32: compute default eps via cp.finfo(cp.float32).eps when use_float32
is true and cp.finfo(float).eps otherwise, and when self.eps is provided cast it
to cp.float32 if use_float32 else to double; update the local variable(s) (e.g.,
introduce eps_f: float and eps_d: double or a single appropriately-typed eps
chosen per-branch) and replace references to the old eps so all downstream code
uses the dtype-correct eps derived from use_float32 and self.eps.
🪄 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: 5afc7388-00d9-40a1-b420-673cd6ad7a62
📒 Files selected for processing (1)
python/cuml/cuml/experimental/linear_model/lars.pyx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
python/cuml/cuml/experimental/linear_model/lars.pyx (2)
264-266:⚠️ Potential issue | 🟠 MajorKeep
epsdtype-specific in the solver dispatch.
epsis materialized as adoublefromcp.finfo(float).eps, so the float32 branch gets the float64 default tolerance and then casts it back down at the call site. That changes the stopping criterion across dtypes.Suggested fix
- cdef double eps = cp.finfo(float).eps if self.eps is None else self.eps + cdef float eps32 + cdef double eps64 + if self.eps is None: + eps32 = <float>cp.finfo(cp.float32).eps + eps64 = <double>cp.finfo(cp.float64).eps + else: + eps32 = <float>self.eps + eps64 = <double>self.eps @@ - <float> eps, + eps32, @@ - <double> eps, + eps64,Also applies to: 268-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/experimental/linear_model/lars.pyx` around lines 264 - 266, The eps default is being materialized as a double unconditionally, altering stopping criteria for float32 inputs; change the eps handling in lars.pyx so it is dtype-specific: when use_float32 is true pick cp.finfo(cp.float32).eps (or cast self.eps to float32) and otherwise use cp.finfo(float).eps, and ensure the variable passed into the solver dispatch matches the solver's expected precision (i.e., keep separate float32 and double eps variables or cast appropriately before calling the float32 vs float64 solver paths); adjust the code around the existing use_float32 and eps symbols so the solver receives a matching-precision tolerance.
309-326:⚠️ Potential issue | 🟠 MajorTrim
coef_path_on both axes before storing it.Only slicing the second axis leaves trailing zero rows whenever
n_active < n_features, socoef_path_no longer matches the active-only layout used by the sklearn comparison in the tests.Suggested fix
- if coef_path is not None: - coef_path = coef_path[:, :n_active + 1] + if coef_path is not None: + coef_path = coef_path[:n_active, :n_active + 1]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/experimental/linear_model/lars.pyx` around lines 309 - 326, The coef_path currently only slices columns which leaves zero rows when n_active < n_cols; update the handling of coef_path so you trim both axes to active rows and recorded alphas columns (e.g., replace coef_path = coef_path[:, :n_active + 1] with coef_path = coef_path[:n_active, :n_active + 1] or equivalent) before assigning self.coef_path_ = None if coef_path is None else CumlArray(coef_path), ensuring the stored coef_path_ matches the active-only layout used by the tests and other attributes like active, beta, and alphas.
🤖 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/experimental/linear_model/lars.pyx`:
- Around line 218-225: The solver contract requires centering/scaling X before
building the Gram and fitting; restore the original X preprocessing in fit():
compute X_mean and X_scale (unit-norm columns), transform X -> X_transformed,
compute gram = self._calc_gram(X_transformed) (not raw X), center y and keep
y_mean, run the LARS solver on X_transformed to get coefficients, then rescale
coefficients back to the original feature scale and set intercept_ = y_mean -
(X_mean * coef_rescaled). Ensure predict() continues to accept raw X by applying
the same centering/scaling implicitly (or by using the rescaled coefficients and
intercept_) so predictions match original feature space; apply the same fix in
the other fit/predict code blocks referenced (the other occurrences around the
later blocks).
---
Duplicate comments:
In `@python/cuml/cuml/experimental/linear_model/lars.pyx`:
- Around line 264-266: The eps default is being materialized as a double
unconditionally, altering stopping criteria for float32 inputs; change the eps
handling in lars.pyx so it is dtype-specific: when use_float32 is true pick
cp.finfo(cp.float32).eps (or cast self.eps to float32) and otherwise use
cp.finfo(float).eps, and ensure the variable passed into the solver dispatch
matches the solver's expected precision (i.e., keep separate float32 and double
eps variables or cast appropriately before calling the float32 vs float64 solver
paths); adjust the code around the existing use_float32 and eps symbols so the
solver receives a matching-precision tolerance.
- Around line 309-326: The coef_path currently only slices columns which leaves
zero rows when n_active < n_cols; update the handling of coef_path so you trim
both axes to active rows and recorded alphas columns (e.g., replace coef_path =
coef_path[:, :n_active + 1] with coef_path = coef_path[:n_active, :n_active + 1]
or equivalent) before assigning self.coef_path_ = None if coef_path is None else
CumlArray(coef_path), ensuring the stored coef_path_ matches the active-only
layout used by the tests and other attributes like active, beta, and alphas.
🪄 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: 0f6eb2b1-363f-402f-80f5-77ace0bbb4cb
📒 Files selected for processing (2)
python/cuml/cuml/experimental/linear_model/lars.pyxpython/cuml/tests/test_lars.py
47 tasks
csadorf
approved these changes
Apr 29, 2026
Member
Author
|
/merge |
This was referenced May 1, 2026
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:
cuml.experimental.linear_model.LarsLarsThis is mostly an internals refactor, no major changes in logic or behavior.
Fixes #7992. Part of #7317.