Skip to content

Conversation

@georeth
Copy link
Contributor

@georeth georeth commented Nov 28, 2025

Fixes #342

Checklist

  • My branch is up-to-date with development branch.
  • Everything works and tested on latest stable Rust.
  • Coverage and Linting have been applied

Current behaviour

LASSO always fits intercept

New expected behaviour

add fit_intercept option

Change logs

@georeth georeth requested a review from Mec-iS as a code owner November 28, 2025 10:30
@codecov
Copy link

codecov bot commented Nov 28, 2025

Codecov Report

❌ Patch coverage is 73.91304% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.87%. Comparing base (70d8a0f) to head (c8787d8).
⚠️ Report is 5 commits behind head on development.

Files with missing lines Patch % Lines
src/linear/lasso.rs 75.00% 5 Missing ⚠️
src/linear/lasso_optimizer.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #344      +/-   ##
===============================================
+ Coverage        45.59%   45.87%   +0.28%     
===============================================
  Files               93       93              
  Lines             8034     7973      -61     
===============================================
- Hits              3663     3658       -5     
+ Misses            4371     4315      -56     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@georeth
Copy link
Contributor Author

georeth commented Nov 28, 2025

I've checked it converges to the desired value by comparing with sklearn's LassoLars. (the existing test dataset is strong enough that coordinate descent (sklearn's Lasso) converges slow)

I am not very confident about whether the optimizer code assumes mean(y) == 0 when it computes search direction.

@Mec-iS
Copy link
Collaborator

Mec-iS commented Nov 28, 2025

if you are not sure, add a couple of tests. You can just translate the ones in sklearn.

@georeth
Copy link
Contributor Author

georeth commented Nov 28, 2025

There are 3 LASSO algorithms involved:

  • coordinate descent: fast if data is well-conditioned, but converge slowly otherwise. (sklearn's Lasso)
  • LassoLars: compute exact solution, but has numerical issue in rare corner cases, because it needs to compare floating numbers.
  • L1LS: Newton interior point method using approximation for speedup (smartcore's implementation)

sklearn's algorithms are not suitable for my use case, and that's why I want to make changes to smartcore.
I tested this PR against sklearn's LassoLars on random-generated data, and the current status of this PR good enough for me.

@georeth
Copy link
Contributor Author

georeth commented Nov 28, 2025

The L1LS paper has no assumption, and it treat LASSO as its special case, so I think it's correct.

@Mec-iS
Copy link
Collaborator

Mec-iS commented Nov 28, 2025

the docstring at the top of the module !// is for noting down these kind of things.
Write there your assumptions with references to the papers like in

//! that assumes that there is approximately a linear relationship between \\(X\\) and \\(y\\).

@georeth
Copy link
Contributor Author

georeth commented Nov 28, 2025

@Mec-iS
The mod doc is really helpful. I had used it to find the L1LS paper:

//! * ["An Interior-Point Method for Large-Scale l1-Regularized Least Squares", K. Koh, M. Lustig, S. Boyd, D. Gorinevsky](https://web.stanford.edu/~boyd/papers/pdf/l1_ls.pdf)

The doc also referenced a matlab implementation of L1LS (by the paper writer):

//! * [Simple Matlab Solver for l1-regularized Least Squares Problems](https://web.stanford.edu/~boyd/l1_ls/)

The matlab implementation doesn't normalize y at all: https://github.com/cvxgrp/l1_ls/blob/master/l1_ls.m

@Mec-iS
Copy link
Collaborator

Mec-iS commented Nov 28, 2025

are these changes breaking the API? the existing API will still be accessible or code that exists should be changed to keep using it?

@georeth
Copy link
Contributor Author

georeth commented Nov 28, 2025

It doesn't breaks the API, and it doesn't introduce a numerical diff.
The only diff I can think of is the (de)serialization of LassoParameter, since I introduced a new option.

@georeth
Copy link
Contributor Author

georeth commented Nov 28, 2025

Oh. If the user constructs a LassoParameters directly without using the builder (default().with_A(...).with_B(...)), the code breaks.

pub struct LassoParameters {
    pub alpha: f64,
    pub normalize: bool,
    pub tol: f64,
    pub max_iter: usize,
    // a new option here
}

This also applies to LassoSearchParameters, and LassoSearchParameters doesn't have a builder interface.

@georeth
Copy link
Contributor Author

georeth commented Nov 28, 2025

Should we set intercept to None instead of Some(0.0) if fit_intercept is false?

pub struct Lasso<TX: FloatNumber + RealNumber, TY: Number, X: Array2<TX>, Y: Array1<TY>> {
    coefficients: Option<X>,
    intercept: Option<TX>,
    _phantom_ty: PhantomData<TY>,
    _phantom_y: PhantomData<Y>,
}

I think coefficients and intercept are Option because SupervisedEstimator trait needs a new().

@Mec-iS
Copy link
Collaborator

Mec-iS commented Nov 28, 2025

0.0 is a value
None is NaN/null
So pick the right one for this case.
If the value is not used/not relevant, then use None.

Breaking changes need to be added to the CHANGELOG.md file

@Mec-iS Mec-iS merged commit 18de2aa into smartcorelib:development Nov 29, 2025
13 checks passed
@Mec-iS
Copy link
Collaborator

Mec-iS commented Nov 29, 2025

Thanks for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LASSO Implementation: Condition bug, missing normalization documentation, and intercept fitting option

2 participants