[MRG] Sample weights for ElasticNet #15436

lorentzenchr · 2019-11-01T23:21:11Z

Reference Issues/PRs

Partially solves #3702: Adds sample_weight to ElasticNet and Lasso, but only for dense feature array X.

rth

Thanks for working on this @lorentzenchr ! Haven't done a full review, but a few comments are below.

rth · 2019-11-16T12:05:02Z

sklearn/compose/tests/test_target.py

@@ -37,7 +37,7 @@ def test_transform_target_regressor_error():
        regr.fit(X, y)
    # fit with sample_weight with a regressor which does not support it
    sample_weight = np.ones((y.shape[0],))
-    regr = TransformedTargetRegressor(regressor=Lasso(),
+    regr = TransformedTargetRegressor(regressor=OrthogonalMatchingPursuit(),


Why this change?

Here, one needs an estimator that does not support sample_weight. As Lasso inherits sample_weight support from ElasticNet, I needed to change this.

sklearn/linear_model/_base.py

rth · 2019-11-16T12:12:01Z

sklearn/linear_model/_coordinate_descent.py

        # Ensure copying happens only once, don't do it again if done above
        should_copy = self.copy_X and not X_copied
        X, y, X_offset, y_offset, X_scale, precompute, Xy = \
            _pre_fit(X, y, None, self.precompute, self.normalize,
                     self.fit_intercept, copy=should_copy,
-                     check_input=check_input)
+                     check_input=check_input, sample_weight=sample_weight,
+                     order='F')


Maybe worth adding a comment about why order="F" is more efficient here

It's only more efficient code at this place, not better performance.

sklearn/linear_model/tests/test_base.py

rth

So this adds support for sample weight by rescaling X by sqrt(sample_weight). I know that it is done in other linear model, but how can we be sure that this is the right thing to do here with L1 regularization?

It would have been nice to have a test comparing results using sample_weight with Ridge when l1_ratio=0. For l1_ratio > 0 do you have any suggestions for checking in unit tests that the output is correct (beyond being consistent with respect to expected invariances) ?

For all the added conversions for array order and sparse format, those are optimizations orthogonal to adding sample_weights, right? If it might be better to separate that code into a separate private function, used when necessary with some justification that this does impact performance in a good way (it's hard to judge which order is optimal without actually benchmarking it for me). This part could also be a separate PR.

rth · 2020-02-05T17:58:35Z

sklearn/linear_model/_base.py

+        if sparse_X:
+            # As of scipy 1.1.0, new argument copy=False by default.
+            # This is what we want.
+            X = X.asformat(sparse_format)


There is a sklearn.utils.fixes._astype_copy_false that could be used for this,

Suggested change

X = X.asformat(sparse_format)

X = X.asformat(sparse_format, **_astype_copy_false(X))

rth · 2020-02-05T18:14:50Z

sklearn/linear_model/_base.py

@@ -191,6 +211,20 @@ def _rescale_data(X, y, sample_weight):
                                  shape=(n_samples, n_samples))
    X = safe_sparse_dot(sw_matrix, X)
    y = safe_sparse_dot(sw_matrix, y)
+
+    if order is not None:
+        sparse_format = "csc" if order == "F" else "csr"


I find this somewhat not obvious, when calling _rescale_data(..., order='C') on a sparse array. In fact this doesn't need to be part of _rescale_data it could be a separate private function enforcing order when needed.

I moved this in a private function _set_order in __coordinate_descent.py. It's the only place where it is used.

lorentzenchr · 2020-02-05T21:11:18Z

@rth You raise some good questions.

how can we be sure that this is the right thing to do here with L1 regularization?

Because rescaling by the square root works and I thought it is the minimal code change. If I were to code it from scratch, I would include sample weights in the loss function, it's gradient and hessian and pass it to the optimizer.

It would have been nice to have a test comparing results using sample_weight with Ridge when l1_ratio=0.

I will try to include such a test.

For l1_ratio > 0 do you have any suggestions for checking in unit tests that the output is correct?

One can check against glmnet. What do you think?

For all the added conversions for array order and sparse format, those are optimizations orthogonal to adding sample_weights, right?

The only solver for L1-penalty in scikit-learn is coordinate descent (see also #12966). It is written in Cython and expects F-ordered arrays which is optimal for this algo. All I did was to avoid memory copies and pass F-ordered arrays to _cd_fast. Though I might have missed something and I'm not an expert on memoryviews in Cython.
Therefore, I would prefer to keep this PR together.

rth · 2020-02-06T23:05:50Z

how can we be sure that this is the right thing to do here with L1 regularization?

Because rescaling by the square root works and I thought it is the minimal code change.

Indeed. Actually, if we take as definition of sample weights that a weight of 2 is equivalent to 2 repeated samples etc, #15651 (comment) then checking this invariances as you have done could be enough, and I'm actually fine with this without a comparison to glmnet or other external package. Other parameters (alpha, l1_ratio) should be invariant to the addition of sample weights, as far as I understand (to come back to #15657 (comment) discussion).

Also cc @agramfort

All I did was to avoid memory copies and pass F-ordered arrays to _cd_fast.
Therefore, I would prefer to keep this PR together.

OK, fair enough.

sklearn/linear_model/tests/test_coordinate_descent.py

agramfort · 2020-02-08T15:51:26Z

sklearn/linear_model/_coordinate_descent.py

+            if check_input:
+                if sparse.issparse(X):
+                    raise ValueError("Sample weights do not (yet) support "
+                                     "sparse matrices.")


I see tests with sample weights and sparse matrices why do you have this?

There is test_enet_sample_weight_sparse, whicht tests that ElasticNetc raises an error as sample weights are not supported with sparse X.
Secondly, there is test_enet_sample_weight_consistency which is decorated with @pytest.mark.parametrize('sparseX', [False]) such that this test could easily be extended once sparse X are supported with sample weights.

ok I had not paid attention to @pytest.mark.parametrize('sparseX', [False]) We tend to add code for tests when it becomes relevant. Do you plan to this next when this is merged?

I'd say that this is needed to close #3702 and that is is a useful feature. I'm tempted to try it in a new PR, but I'm afraid that it needs a larger code change than this one.

sklearn/linear_model/tests/test_coordinate_descent.py

Co-Authored-By: Alexandre Gramfort <alexandre.gramfort@m4x.org>

agramfort · 2020-02-12T12:44:24Z

2 more things on my side. You have left comments from @rth to address or rule out. I would not add code/test for things we don't use and we don't know if the new code is actually correct. I would therefore remove the sparseX option in the test. Let's had code / tests when we actually use it. thx @lorentzenchr

lorentzenchr · 2020-02-12T22:16:28Z

@agramfort Thanks for pointing out unaddressed issues. I hope it is in a good shape now.
I suspect and hope that the test error of doc-min-dependencies in unrelated to this PR.

agramfort

LGTM

@rth feel free to have another look

thx @lorentzenchr

agramfort · 2020-02-13T07:28:50Z

@lorentzenchr just don't forget to update what's new page.

agramfort · 2020-02-14T10:29:04Z

doc/whats_new/v0.23.rst

 :mod:`sklearn.linear_model`
 ...........................

+- |Feature| Support of `sample_weight` in :class:`linear_model.ElasticNet` and
+  :class:`linear_model:Lasso` for dense feature matrix `X`.


you don't have it for the CV estimators?

No. The what's new is correct. Sample weight support is added only to ElasticNet and Lasso. Unfortunately, the logic in _coordinate_descent.py is such that this feature is not automatically inherited by ElasticNetCV and others. I do not know if there are advantages with the current structure.
In any case, I'd prefer to put adding sample weights to ElasticNetCV in another PR. What do you think?

agramfort · 2020-02-14T13:31:43Z

ok. It's just that I hear something that we are consistent in linear models with certain options implemented only in certain estimators without clear reason. So it suggests we should have sample_weights added to CV classes very soon and ideally in the same release cycle

…

rth

LGTM as well, thanks @lorentzenchr !

lorentzenchr mentioned this pull request Nov 2, 2019

Bug in sparse in Ridge with sample weights #15438

Closed

lorentzenchr force-pushed the enet_sample_weights branch from 42d43fa to c1e837f Compare November 2, 2019 13:00

Christian Lorentzen added 6 commits November 6, 2019 18:12

MNT Simplify _check_sample_weight

f5fb1cc

TST Fix unsupported sample weights

70f5ffb

ENH Support sample weights for dense ElasticNet

d879789

TST raise ValueError when sparse enet with sample weights

c751b0e

Fix sparse logic in _rescale_data

2fd44e0

Remove check for negative sample weights

c83a4ca

lorentzenchr force-pushed the enet_sample_weights branch from c1e837f to c83a4ca Compare November 6, 2019 17:31

lorentzenchr changed the title ~~[WIP] Sample weights for ElasticNet~~ [MRG] Sample weights for ElasticNet Nov 6, 2019

Christian Lorentzen added 2 commits November 16, 2019 11:39

TST test _rescale_data_ for sparse input

c088e79

Merge branch 'master' into enet_sample_weights

9ee0042

rth reviewed Nov 16, 2019

View reviewed changes

rth added Enhancement Waiting for Reviewer labels Nov 16, 2019

Christian Lorentzen added 6 commits November 16, 2019 13:18

DOC align parameter doc of sample_weight

95c8e6c

DOC adds docstring to _rescale_data

2a970b2

Simplify _rescale_data with reviewer suggestion

6d77c55

TST invalid order argument in _rescale_data

1317b14

add doc and comment on order='F'

d0b0788

TST 2d target in _rescale_data

64ec8e4

rth mentioned this pull request Nov 19, 2019

RFC Sample weight invariance properties #15657

Open

Christian Lorentzen added 5 commits November 24, 2019 16:36

Unsupported arg copy in asformat prior to scipy 1.1.0

810fb1b

Merge branch 'master' into enet_sample_weights

ed02723

Merge branch 'master' into enet_sample_weights

c34f557

TST test sample_weight=number same as sw=None

7bb1e6c

Merge branch 'master' into enet_sample_weights

5aefb7d

rth reviewed Feb 5, 2020

View reviewed changes

agramfort reviewed Feb 8, 2020

View reviewed changes

Christian Lorentzen and others added 3 commits February 9, 2020 15:36

DOC fix a typo

056252b

Co-Authored-By: Alexandre Gramfort <alexandre.gramfort@m4x.org>

DOC correct a typo

43a65e7

Co-Authored-By: Alexandre Gramfort <alexandre.gramfort@m4x.org>

CLN remove unwanted line

3523741

Christian Lorentzen added 3 commits February 12, 2020 22:03

MAINT use _astype_copy_false

0b03077

RFC private _set_order function

2ac24b1

TST remove sparse in enet with sample weights

610ed29

agramfort approved these changes Feb 13, 2020

View reviewed changes

Christian Lorentzen added 2 commits February 13, 2020 22:48

Merge branch 'master' into enet_sample_weights

c15d778

DOC what's new

7c875c9

agramfort reviewed Feb 14, 2020

View reviewed changes

lorentzenchr mentioned this pull request Feb 16, 2020

[MRG] Sample weights for ElasticNetCV #16449

Merged

rth approved these changes Feb 16, 2020

View reviewed changes

rth merged commit 21686b7 into scikit-learn:master Feb 16, 2020

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020

ENH Sample weights for ElasticNet (scikit-learn#15436)

ac10b39

lorentzenchr deleted the enet_sample_weights branch February 29, 2020 10:03

panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020

ENH Sample weights for ElasticNet (scikit-learn#15436)

8ae773d

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020

ENH Sample weights for ElasticNet (scikit-learn#15436)

e5b18f5

lorentzenchr mentioned this pull request Aug 8, 2020

sample_weight for lasso, elastic etc #3702

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Sample weights for ElasticNet #15436

[MRG] Sample weights for ElasticNet #15436

lorentzenchr commented Nov 1, 2019 •

edited

Loading

rth left a comment

rth Nov 16, 2019

lorentzenchr Nov 16, 2019

rth Nov 16, 2019

lorentzenchr Nov 16, 2019

rth left a comment •

edited

Loading

rth Feb 5, 2020

rth Feb 5, 2020

lorentzenchr Feb 12, 2020

lorentzenchr commented Feb 5, 2020

rth commented Feb 6, 2020

agramfort Feb 8, 2020

lorentzenchr Feb 9, 2020

agramfort Feb 10, 2020

lorentzenchr Feb 10, 2020

agramfort commented Feb 12, 2020

lorentzenchr commented Feb 12, 2020

agramfort left a comment

agramfort commented Feb 13, 2020

agramfort Feb 14, 2020

lorentzenchr Feb 14, 2020

agramfort commented Feb 14, 2020 via email

rth left a comment

	X = X.asformat(sparse_format)
	X = X.asformat(sparse_format, **_astype_copy_false(X))

[MRG] Sample weights for ElasticNet #15436

[MRG] Sample weights for ElasticNet #15436

Conversation

lorentzenchr commented Nov 1, 2019 • edited Loading

Reference Issues/PRs

rth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rth left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorentzenchr commented Feb 5, 2020

rth commented Feb 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agramfort commented Feb 12, 2020

lorentzenchr commented Feb 12, 2020

agramfort left a comment

Choose a reason for hiding this comment

agramfort commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agramfort commented Feb 14, 2020 via email

rth left a comment

Choose a reason for hiding this comment

lorentzenchr commented Nov 1, 2019 •

edited

Loading

rth left a comment •

edited

Loading