Add sample_weight to the calculation of alphas in enet_path and LinearModelCV by s-banach · Pull Request #23045 · scikit-learn/scikit-learn

s-banach · 2022-04-04T02:16:36Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Modifies _alpha_grid function in linear_model._coordinate_descent to accept a sample_weight argument.

The function _alpha_grid is called in two places, enet_path and LinearModelCV.
The new sample_weight argument is not used by enet_path, but it is used by LinearModelCV.

Any other comments?

Since my previous PR on this issue, _preprocess_data has been rewritten.

It seems like this single call to _preprocess_data suffices in all cases.

This tiny example was given in #22914. The test merely asserts that alpha_max is large enough to force the coefficient to 0.

lorentzenchr

A first round of review comments.

sklearn/linear_model/_coordinate_descent.py

sklearn/linear_model/tests/test_coordinate_descent.py

As per reviewer's suggestions: (1) Clarify eps=1. (2) Parameterize `fit_intercept`.

(1) Give the name `n_samples` to the quantity `X.shape[0]`. (2) Clarify that `y_offset` and `X_scale` are not used, since these are already applied to the data by `_preprocess_data`.

lorentzenchr · 2022-07-01T12:04:13Z

@TomDLT May I kindly ping you as your help would be much appreciated.

TomDLT

Looks good, although I did not check the math.

Main remark: The new test function tests that the computed alpha_max is larger or equal to the true alpha_max. To test that they are actually equal, we could test that alpha_max * 0.99 does not return all-zero coefficients.

We could also add a test that the computation still works without sample weights.

sklearn/linear_model/_coordinate_descent.py

s-banach · 2022-07-02T14:10:29Z

Main remark: The new test function tests that the computed alpha_max is larger or equal to the true alpha_max. To test that they are actually equal, we could test that alpha_max * 0.99 does not return all-zero coefficients.

I have attempted to update the test according to your recommendation.
It now checks that the max abs coefficient is greater than 1e-3 when alpha=0.99*alpha_max.

We could also add a test that the computation still works without sample weights.

My feeling is that test_enet_cv_sample_weight_consistency basically guarantees this already.
Let me know your thoughts.

s-banach · 2022-07-02T19:23:11Z

The main thing I'm confused about, is why it's even possible for _alpha_grid to be called before X and y are appropriately scaled by sample_weight. It seems that enet_path or LinearModelCV or something should be refactored such that the call to _preprocess_data can be removed from _alpha_grid.

TomDLT · 2022-07-05T18:03:03Z

My feeling is that test_enet_cv_sample_weight_consistency basically guarantees this already.

I don't think it guarantees that the alpha_max computation is correct. To do so, we could add a @pytest.mark.parametrize("sample_weight", [[10, 1, 10, 1], None]) to the new test.

The main thing I'm confused about, is why it's even possible for _alpha_grid to be called before X and y are appropriately scaled by sample_weight.

It seems weird indeed. It seems that _pre_fit is called either before _alpha_grid in enet_path, or after _alpha_grid in LinearModelCV.fit (in _path_residuals). We should clarify the situation.

s-banach · 2022-07-06T03:03:05Z

Per your suggestion, I parameterized the new test by sample_weight.

I know my opinion on this matter isn't very valuable, but I'll share anyway.
As you say, _alpha_grid can currently be called from two contexts: from a path method such as enet_path, or from a LinearModelCV.
Currently, LinearModelCV works by finding the best alpha using CV, then feeding that alpha into the non-CV version of the estimator. Instead, I think LinearModelCV should begin by using path to fit the full dataset. This will allow the user to see the full coef_path after fitting the model, and it may even improve the total runtime due to warm starting.
If this change is made, then _alpha_grid will only ever be called from within a path method. Thus _alpha_grid will not be asked to deal with sample_weight at all.

TomDLT · 2022-07-06T21:55:56Z

I agree it makes more sense to compute the alpha grid within the path function. We would need to have _path_residuals return the computed alphas, but this is not a problem because the function is private.

We might still need to have _alpha_grid deal with sample_weights in the sparse case.

jeremiedbb · 2022-11-24T13:15:27Z

We won't have time to review this one before the 1.2 release. Moving it to 1.3

jeremiedbb · 2022-11-24T13:15:56Z

(didn't mean to close :/ )

s-banach added 2 commits April 3, 2022 22:11

Update _alpha_grid to take sample_weight

8d4b501

It seems like this single call to _preprocess_data suffices in all cases.

Add a simple test for alpha_max with sample_weight

2f494db

This tiny example was given in #22914. The test merely asserts that alpha_max is large enough to force the coefficient to 0.

github-actions bot added the module:linear_model label Apr 4, 2022

s-banach mentioned this pull request Apr 4, 2022

Add sample_weight to the calculation of alphas in enet_path and LinearModelCV #22933

Closed

lorentzenchr reviewed Apr 4, 2022

View reviewed changes

s-banach added 3 commits April 4, 2022 09:06

Update test

fa2c821

As per reviewer's suggestions: (1) Clarify eps=1. (2) Parameterize `fit_intercept`.

Clarify _alpha_grid.

75e6584

(1) Give the name `n_samples` to the quantity `X.shape[0]`. (2) Clarify that `y_offset` and `X_scale` are not used, since these are already applied to the data by `_preprocess_data`.

Clarify notation

8b6cfc0

lorentzenchr added this to the 1.2 milestone Apr 21, 2022

lorentzenchr added the Waiting for Reviewer label Jun 17, 2022

TomDLT reviewed Jul 1, 2022

View reviewed changes

sklearn/linear_model/_coordinate_descent.py Outdated Show resolved Hide resolved

sklearn/linear_model/_coordinate_descent.py Outdated Show resolved Hide resolved

s-banach added 2 commits July 2, 2022 09:51

Use Xy if it is provided.

2ba4c57

Update test, check alpha_max is not too large

5d1f5e7

Fix test that alpha_max is not too large.

dce169c

Test alpha_max without sample_weight.

380c21f

jeremiedbb closed this Nov 24, 2022

jeremiedbb reopened this Nov 24, 2022

jeremiedbb modified the milestones: 1.2, 1.3 Nov 24, 2022

lorentzenchr added the Stalled label Jan 26, 2023

jeremiedbb modified the milestones: 1.3, 1.4 Jul 6, 2023

glemaitre removed this from the 1.4 milestone Dec 7, 2023

s-banach closed this by deleting the head repository Jan 23, 2024

snath-xoc mentioned this pull request Jun 19, 2024

Fix elasticnet cv sample weight #29308

Closed

1 task

snath-xoc mentioned this pull request Jul 9, 2024

Fix elasticnect cv sample weight #29442

Merged

1 task

Uh oh!

Conversation

s-banach commented Apr 4, 2022

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

lorentzenchr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lorentzenchr commented Jul 1, 2022

Uh oh!

TomDLT left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

s-banach commented Jul 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-banach commented Jul 2, 2022

Uh oh!

TomDLT commented Jul 5, 2022

Uh oh!

s-banach commented Jul 6, 2022

Uh oh!

TomDLT commented Jul 6, 2022

Uh oh!

jeremiedbb commented Nov 24, 2022

Uh oh!

jeremiedbb commented Nov 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TomDLT left a comment •

edited

Loading

s-banach commented Jul 2, 2022 •

edited

Loading