[MRG] Does not store all cv values nor all dual coef in _RidgeGCV fit #15183

jeromedockes · 2019-10-11T14:25:02Z

in master the _RidgeGCV stores the LOO predictions and the dual coefficients for all hyperparameters during fit, which can take a lot of memory. This PR only stores the best score and coefficients when store_cv_values == False

thomasjpfan

LGTM. Please add whats new entry with Efficiency tag.

sklearn/linear_model/ridge.py

thomasjpfan · 2019-10-11T14:34:23Z

sklearn/linear_model/ridge.py

@@ -1048,6 +1048,16 @@ def _matmat(self, v):
        return res


+class _IdentityEstimator:


Slightly less hacky...now that it is a class. :)

sklearn/linear_model/ridge.py

Co-Authored-By: Thomas J Fan <thomasjpfan@gmail.com>

jeromedockes · 2019-10-11T15:05:12Z

thanks @thomasjpfan

rth

Thanks @jeromedockes ! Some of the added changes are not covered by tests, it would be good to add more tests for those (unless codecov has issues again).

rth · 2019-10-11T15:34:20Z

sklearn/linear_model/ridge.py

+    """Hack to call a scorer when we already have the predictions."""
+
+    def decision_function(self, y_predict):
+        return y_predict


This is never called by tests.

rth · 2019-10-11T15:34:45Z

sklearn/linear_model/ridge.py

+                alpha_score = scorer(
+                    _IdentityEstimator(), predictions.ravel(), y.ravel())
+                if self.store_cv_values:
+                    self.cv_values_[:, i] = predictions.ravel()


not covered by tests either.

This code path is not reached because

scikit-learn/sklearn/linear_model/ridge.py

Lines 1562 to 1564 in 86aea99

if self.store_cv_values:

raise ValueError("cv!=None and store_cv_values=True "

" are incompatible")

which means we can assume that self.store_cv_values is always false when scorer is defined?

no, it is always false when cv != None which means do not use GCV, but this uses a different estimator than the one affected here. The tests I added now should cover the missing lines.
But it is true that the meaning of the stored "cv values" is different when scorer is defined, see #13998

thomasjpfan · 2019-10-12T13:55:47Z

sklearn/linear_model/ridge.py

+                best_coef, best_score, best_alpha = c, alpha_score, alpha
+
+        self.alpha_ = best_alpha
+        self.best_score_ = best_score


If we want to include this, we should document it.

I added a note in the docstring. but note that this is a private class -- users don't have access to this best_score_.
adding a best_score_ attribute to public RidgeCV and RidgeClassifierCV estimators is discussed in #4667

jnothman · 2019-10-15T12:42:28Z

doc/whats_new/v0.22.rst

@@ -372,6 +373,12 @@ Changelog
  and `fit_intercept=True`.
  :pr:`15086` by :user:`Alex Gramfort <agramfort>`.

+- |Efficiency| :class:`linear_model.RidgeCV` now does not allocate a potentially


By convention, this should appear before the linear_model Fix entries.

NicolasHug · 2019-11-08T14:18:59Z

sklearn/linear_model/ridge.py

@@ -1048,6 +1048,16 @@ def _matmat(self, v):
        return res


+class _IdentityEstimator:


We are starting to need those hacks everywhere... We talked about this with @ogrisel but can't remember where

NicolasHug

Thanks for the PR @jeromedockes , a few comments

sklearn/linear_model/tests/test_ridge.py

NicolasHug · 2019-11-08T16:18:00Z

sklearn/linear_model/tests/test_ridge.py

+    def scorer(estimator, X, Y):
+        pred = estimator.decision_function(X)
+        return np.sum((pred - Y)**2)


why do you need this scorer?

NicolasHug · 2019-11-08T16:18:13Z

sklearn/linear_model/tests/test_ridge.py

+    assert_allclose(loo_pred, ridge_cv.cv_values_[:, 1])
+
+
+def test_ridge_gcv_decision_function_scoring():


What is this test testing?

I think that this test was intended to check the equivalence between score=None and score=scorer where scorer compute the mean squared error

doc/whats_new/v0.22.rst

sklearn/linear_model/_ridge.py

sklearn/linear_model/tests/test_ridge.py

glemaitre · 2019-11-15T18:49:25Z

@jeromedockes I pushed what I thought your tests were done for. Could you have a look if it was what you intended.

qinhanmin2014

We need to mention RidgeClassifierCV in what's new, otherwise this LGTM

qinhanmin2014 · 2019-11-17T13:42:18Z

doc/whats_new/v0.22.rst

@@ -495,6 +496,16 @@ Changelog
  requires less memory.
  :pr:`14108`, :pr:`14170`, :pr:`14296` by :user:`Alex Henrie <alexhenrie>`.

+- |Efficiency| :class:`linear_model.RidgeCV` now does not allocate a
+  potentially large array to store dual coefficients for all hyperparameters
+  during its `fit`, nor an array to store all LOO predictions unless


LOO predictions or mean squared errors

actually it depends of scoring: LOO predictions if scoring is not None otherwise mean squared errors.
But agreed that it should be added.

qinhanmin2014 · 2019-11-17T13:46:37Z

sklearn/linear_model/tests/test_ridge.py

+    # equivalent to `scoring=None`
+
+    def scorer(estimator, X, Y):
+        pred = estimator.decision_function(X)


perhaps predict will be more friendly here :)

qinhanmin2014 · 2019-11-18T03:44:29Z

sklearn/linear_model/_ridge.py

        for i, alpha in enumerate(self.alphas):
            G_inverse_diag, c = solve(
                float(alpha), y, sqrt_sw, X_mean, *decomposition)
            if error:
                squared_errors = (c / G_inverse_diag) ** 2
-                cv_values[:, i] = squared_errors.ravel()
+                alpha_score = -squared_errors.mean()


I think there's another bug: we need to devide sample_weight here, otherwise we get weighted error.

Why shouldn't sample weights be taken into account when computing the error? Note that this has always been the behaviour of the GCV estimator and is made explicit here

scikit-learn/sklearn/linear_model/_ridge.py

Line 1554 in 98cb91b

or another form of cross-validation, because only generalized

It's embarassing that such important thing is documented in private functions.
I think users will expect that RidgeCV() should be equivalent to GridSearchCV(Ridge(), cv=LeaveOneOut())

qinhanmin2014

e.g., if we add sample_weight to test_ridge_gcv_equivalence_prediction_metric, this test will fail

glemaitre · 2019-11-18T10:01:04Z

Good catch @qinhanmin2014

Playing a bit, RidgeClassifierCV is currently broken with problem of shape. I will quickly check where the issues are coming from.

qinhanmin2014 · 2019-11-18T10:03:16Z

Playing a bit, RidgeClassifierCV is currently broken with problem of shape. I will quickly check where the issues are coming from.

I have a draft locally, I'll upload it so you can play with it.

qinhanmin2014 · 2019-11-18T10:07:15Z

actually there's another issue regarding RidgeClassifierCV, let's fix RidgeCV first.

jeromedockes · 2019-11-18T13:01:46Z

@jeromedockes I pushed what I thought your tests were done for. Could you have a look if it was what you intended.

yes it is. Thanks a lot!

jeromedockes · 2019-11-18T13:03:38Z

should this pr be closed in favour of #15648 ?

jeromedockes · 2019-11-18T13:08:35Z

doc/whats_new/v0.22.rst

+
+- |Fix| In :class:`linear_model.RidgeCV`, the predicitons reported by
+  `cv_values_` are now rescaled in the original space when `scoring` is not
+  `None`. :pr:`13995` by :user:`Jérôme Dockès <jeromedockes>`.


#13995 is an issue, not the PR

qinhanmin2014 · 2019-11-18T13:10:08Z

should this pr be closed in favour of #15648 ?

We'll close when needed and your name will still be noted. Thanks for contributing.

qinhanmin2014 · 2019-11-18T13:10:41Z

And if you have time, please review #15648, thanks a lot :)

glemaitre · 2019-11-18T13:23:08Z

@qinhanmin2014 @jeromedockes is working as well in Inria with me so we can continue working directly in this PR. It would avoid duplicated comments and keep the history.

qinhanmin2014 · 2019-11-18T13:23:59Z

@qinhanmin2014 @jeromedockes is working as well in Inria with me so we can continue working directly in this PR. It would avoid duplicated comments and keep the history.

That's fine. I provide that PR for you to play with.

glemaitre · 2019-11-18T13:26:20Z

Can you guys redirect there: https://gitter.im/glemaitre/ridgecv

qinhanmin2014 · 2019-11-19T03:35:20Z

please ping when you want me to review.

cmarmo · 2020-08-06T14:41:20Z

@glemaitre, the issue this PR was meant to close has been closed in the meanwhile by #15182 (you are the author). Should this one be closed? What about #15648 also meant to close the same issue? Thanks!

glemaitre · 2020-08-18T07:16:20Z

We can close this PR. There is still some unsolved issue with Ridge:

*Denormalize predictions

Unscaled MSE
Deprecate other CV than gcv (which is not gcv :))

do not store all cv values nor all dual coef in _RidgeGCV fit

3aac2e5

thomasjpfan approved these changes Oct 11, 2019

View reviewed changes

jeromedockes and others added 2 commits October 11, 2019 16:53

Apply suggestions from code review

51b9add

Co-Authored-By: Thomas J Fan <thomasjpfan@gmail.com>

whatsnew entry

a78be8c

rth reviewed Oct 11, 2019

View reviewed changes

add tests for different scorers in _RidgeGCV

bfb6d98

thomasjpfan reviewed Oct 12, 2019

View reviewed changes

add note in _RidgeGCV docstring

c7baa7c

thomasjpfan changed the title ~~do not store all cv values nor all dual coef in _RidgeGCV fit~~ [MRG] Does not store all cv values nor all dual coef in _RidgeGCV fit Oct 15, 2019

jnothman reviewed Oct 15, 2019

View reviewed changes

move whatsnew entry

61782b3

thomasjpfan added this to the 0.22 milestone Oct 26, 2019

jnothman added the Waiting for Reviewer label Nov 5, 2019

NicolasHug reviewed Nov 8, 2019

View reviewed changes

glemaitre self-requested a review November 15, 2019 14:35

glemaitre added 2 commits November 15, 2019 15:52

git move ridge -> _ridge

89dde17

Merge remote-tracking branch 'origin/master' into pr/jeromedockes/15183

02e694e

glemaitre reviewed Nov 15, 2019

View reviewed changes

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

glemaitre reviewed Nov 15, 2019

View reviewed changes

glemaitre added 3 commits November 15, 2019 19:21

TST add additional tests

b4cf560

fix

98d2f07

TST check equivalence scoring none and mse

097e783

qinhanmin2014 approved these changes Nov 17, 2019

View reviewed changes

qinhanmin2014 reviewed Nov 17, 2019

View reviewed changes

qinhanmin2014 reviewed Nov 18, 2019

View reviewed changes

qinhanmin2014 requested changes Nov 18, 2019

View reviewed changes

qinhanmin2014 mentioned this pull request Nov 18, 2019

[MRG] FIX and ENH in _RidgeGCV #15648

Closed

jeromedockes commented Nov 18, 2019

View reviewed changes

glemaitre added 2 commits November 18, 2019 14:40

compute mse by hand applying sample weight

86554c5

Merge remote-tracking branch 'origin/master' into pr/jeromedockes/15183

3c637e9

qinhanmin2014 modified the milestones: 0.22, 0.23 Nov 26, 2019

github-actions bot added the module:linear_model label Mar 2, 2020

thomasjpfan modified the milestones: 0.23, 0.24 Apr 20, 2020

cmarmo removed the Waiting for Reviewer label Aug 6, 2020

glemaitre closed this Aug 18, 2020

		@@ -1048,6 +1048,16 @@ def _matmat(self, v):
		return res


		class _IdentityEstimator:

	if self.store_cv_values:
	raise ValueError("cv!=None and store_cv_values=True "
	" are incompatible")

		assert_allclose(loo_pred, ridge_cv.cv_values_[:, 1])


		def test_ridge_gcv_decision_function_scoring():

Uh oh!

[MRG] Does not store all cv values nor all dual coef in _RidgeGCV fit #15183

[MRG] Does not store all cv values nor all dual coef in _RidgeGCV fit #15183

Uh oh!

Conversation

jeromedockes commented Oct 11, 2019

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeromedockes commented Oct 11, 2019

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glemaitre commented Nov 15, 2019

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Nov 18, 2019

Uh oh!

qinhanmin2014 commented Nov 18, 2019

Uh oh!

qinhanmin2014 commented Nov 18, 2019

Uh oh!