Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH - add support for intercept in SqrtLasso #214

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

PascalCarrivain
Copy link
Contributor

Context of the PR

This PR adds support for intercept in SqrtLasso estimator.
Closes #96

Checks before merging PR

  • added documentation for any new feature
  • added unittests
  • edited the what's new

@PascalCarrivain PascalCarrivain changed the title fix add support for intercept in SqrtLasso ENH - add support for intercept in SqrtLasso Dec 14, 2023
Copy link
Collaborator

@Badr-MOUFAD Badr-MOUFAD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @PascalCarrivain for the PR!
Here are some remarks

skglm/experimental/sqrt_lasso.py Outdated Show resolved Hide resolved
skglm/experimental/sqrt_lasso.py Outdated Show resolved Hide resolved
skglm/experimental/sqrt_lasso.py Outdated Show resolved Hide resolved
skglm/experimental/sqrt_lasso.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@mathurinm mathurinm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Badr-MOUFAD merge if happy!

Copy link
Collaborator

@Badr-MOUFAD Badr-MOUFAD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks @PascalCarrivain for the hard work 💪

Just a small bit before merging

We didn't add tests for the intercept (we might be vulnerable to bugs later)

I'm not aware of any package to check against it the new feature, any suggestions @mathurinm?

With that being said, we can leverage the equivalence between the problem with intercept and without as the datafit is a norm_2 hence setting its gradient w.r.t. the intercept to zero implies setting the residual to zero

$$ \begin{equation} \min_{w, w_0} \| y - (Xw - w_0 \mathbb{1}) \| + \lambda \| w \|_1 \Longleftrightarrow \begin{cases} \min_w & \| \bar{y} - \bar{X}w \| + \lambda \| w \|_1 \\\ \text{where} & w_0 = \text{mean}(y) - \langle \text{mean\_col}(X), \hat{w} \rangle \end{cases} \end{equation} $$

Comment on lines +19 to +22
if sqrt_lasso.fit_intercept:
np.testing.assert_equal(sqrt_lasso.coef_[:-1], 0)
else:
np.testing.assert_equal(sqrt_lasso.coef_, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about this refactoring?

Suggested change
if sqrt_lasso.fit_intercept:
np.testing.assert_equal(sqrt_lasso.coef_[:-1], 0)
else:
np.testing.assert_equal(sqrt_lasso.coef_, 0)
np.testing.assert_equal(sqrt_lasso.coef_[:n_features], 0)

PascalCarrivain added a commit to PascalCarrivain/skglm that referenced this pull request Mar 18, 2024
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.

ENH - Add support for intercept in SqrtLasso
3 participants