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 alpha_per_target parameter to RidgeCV #6624

Merged
merged 17 commits into from Aug 6, 2020

Conversation

wmvanvliet
Copy link
Contributor

@wmvanvliet wmvanvliet commented Apr 5, 2016

Ever since #2068 was merged, the Ridge class can take multiple alpha values: namely one for each target. This PR updates RidgeCV to match. It adds an alpha_per_target flag which instructs RidgeCV to estimate separate alpha values for each target.

This can be done nearly for free when using GCV (by passing cv=None). edit: When using a custom CV object (so not using GCV), this would be slow as molasses since it needs to grid search over a (#alphas x #targets) space. Therefore, alpha_per_target = True is only supported when using GCV and throws an error otherwise.

Fixes #17574

@amueller
Copy link
Member

can you rebase please?

@wmvanvliet
Copy link
Contributor Author

Done!

@amueller
Copy link
Member

If it's ready for review, please change the title to [MRG], otherwise it's unlikely anyone will look at it ;)
I think we should bail on alpha_per_target if a cv object is passed, that is throw an error.

I find the cv object case here a bit strange anyhow, I have half a mind to deprecate it, but that would be a different PR.

@wmvanvliet wmvanvliet changed the title [WIP] Add alpha_per_target parameter to RidgeCV [MRG] Add alpha_per_target parameter to RidgeCV Oct 21, 2016
@nblauch
Copy link

nblauch commented Mar 16, 2019

hi! i think this is a very useful feature and would love to see this merged. is there anything i could do to help?

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Sorry this got forgotten, @wmvanvliet! It's looking pretty good!

sklearn/linear_model/tests/test_ridge.py Outdated Show resolved Hide resolved
@wmvanvliet
Copy link
Contributor Author

Thanks for digging this back up!

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Thanks! If lucky we might get another reviewer soon.

Please add an entry to the change log at doc/whats_new/v0.21.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

@jnothman
Copy link
Member

Also, preferable to merge master rather than rebase these days

@wmvanvliet
Copy link
Contributor Author

Also, preferable to merge master rather than rebase these days

That's news to me. Why is that? It used to be that a PR would become super messy if you did that.

@jnothman
Copy link
Member

That's news to me. Why is that? It used to be that a PR would become super messy if you did that.

I don't think it ever made the pr messy, although you can make errors in either operation. GitHub's squash and merge feature makes the commit history pretty when merging, while force pushed are not so easy for GitHub to track

@agramfort
Copy link
Member

sorry to come late here but I would be happy to know if this actually helps running time vs doing a for loop over targets with a list of RidgeCV. Did you do the test?

@wmvanvliet
Copy link
Contributor Author

Just ran the test. Here are the results:

from sklearn.datasets import make_regression
from sklearn.linear_model import RidgeCV
X, y = make_regression(n_samples=100, n_features=1000, n_targets=100)
alphas = list(range(1, 100))

First, with a for-loop:

%%timeit
for y_ in y.T:
    m = RidgeCV(alphas=alphas).fit(X, y_)

2.82 s ± 56.8 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Next, with alpha_per_target=True:

%%timeit
m = RidgeCV(alphas=alphas, alpha_per_target=True).fit(X, y)

63.1 ms ± 2.61 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

@wmvanvliet
Copy link
Contributor Author

(this is on my trusty MacBook Pro 2011)

@wmvanvliet
Copy link
Contributor Author

@agramfort Could you be a second reviewer for this? Would be cool to have this merged at some point.

@agramfort
Copy link
Member

@wmvanvliet I suspect you need to update some docstrings of attributes such as alpha_

self.dual_coef_ = C[best]

if self.alpha_per_target:
self.dual_coef_ = np.vstack([C[j][:, i]
Copy link
Member

Choose a reason for hiding this comment

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

We should test dual_coef_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

As in the shape of dual_coef_ is what we expected.

sklearn/linear_model/ridge.py Outdated Show resolved Hide resolved
sklearn/linear_model/ridge.py Outdated Show resolved Hide resolved
sklearn/linear_model/ridge.py Outdated Show resolved Hide resolved
sklearn/linear_model/tests/test_ridge.py Outdated Show resolved Hide resolved
@jnothman
Copy link
Member

CIs are now failing :(

@wmvanvliet
Copy link
Contributor Author

Can anyone tell me why codecov is flagging all kinds of things unrelated to this PR?

@jnothman
Copy link
Member

Yeah, I've noticed codecov doing a bad job of identifying the diff. I wonder if it's related to your force pushing... Try merging master in...?

With scikit-learn#2068, the Ridge class can take multiple alpha values, namely one
for each target. This updates RidgeCV to also be able to estimate
separate alpha values for each target.
Copy link
Member

@cmarmo cmarmo left a comment

Choose a reason for hiding this comment

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

Thanks @wmvanvliet. Some minor comments.

doc/whats_new/v0.21.rst Outdated Show resolved Hide resolved
sklearn/linear_model/_ridge.py Outdated Show resolved Hide resolved
@wmvanvliet
Copy link
Contributor Author

done!

Comment on lines 1134 to 1137
# alpha_per_target cannot be used in classifier mode. All subclasses
# of _RidgeGCV that are classifiers keep alpha_per_target at its
# default value: False, so the condition below should never happen.
assert not (is_clf and alpha_per_target)
Copy link
Member

Choose a reason for hiding this comment

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

We normally do not validate in init. This can be moved to the fit method while raising an ValueError.

sklearn/linear_model/_ridge.py Show resolved Hide resolved
`alphas` parameter list) for each target separately. When set to
`True`, after fitting, the `alpha_` attribute will contain a value for
each target. When set to `False`, a single alpha is used for all
targets. This flag has no effect when there is only one target.
Copy link
Member

Choose a reason for hiding this comment

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

Currently, the flag looks like it will change the shape of the attributes. (They are no longer floats). We can check for one target and have a test to make sure the attributes (alpha_, etc) are still floats.

@@ -186,6 +186,15 @@ Changelog
- |Enhancement| :class:`isotonic.IsotonicRegression` now accepts 2darray with 1 feature as
input array. :pr:`17379` by :user:`Jiaxiang <fujiaxiang>`.

:mod:`sklearn.linear_model`
..........................
Copy link
Member

Choose a reason for hiding this comment

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

One more dot and sphinx will be happy... :)

@wmvanvliet
Copy link
Contributor Author

...and we're green again!

@cmarmo
Copy link
Member

cmarmo commented Jul 20, 2020

@thomasjpfan , let us know if all your comments have been correctly addressed.. we have a four year old PR here ready to be merged! :) Thanks!

@wmvanvliet
Copy link
Contributor Author

Reading issue #17574, do you think we should allow for a different scorer to be passed for each target? If you have a large number of targets, this would indeed be faster than fitting separate models. On the other hand, the Ridge function is quite complicated already. And you could achieve the effect manually by parsing cv_values.

Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I made a tiny suggestion, but I'm in favor of merging once this is done.

@thomasjpfan : do you want to re-review before we merge?

sklearn/linear_model/_ridge.py Outdated Show resolved Hide resolved
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

best_score_ shape needs to be updated in the docstring.

Currently, this breaks when alpha_per_target=True and there is only one target:

from sklearn.datasets import load_diabetes
from sklearn.linear_model import RidgeCV
X, y = load_diabetes(return_X_y=True)
clf = RidgeCV(alphas=[1e-3, 1e-2, 1e-1, 1], alpha_per_target=True).fit(X, y)

@wmvanvliet
Copy link
Contributor Author

Good catch @thomasjpfan! Test case added.

@wmvanvliet
Copy link
Contributor Author

Is the CircleCI error related to this PR?

@cmarmo
Copy link
Member

cmarmo commented Jul 29, 2020

Hi @wmvanvliet, no, the CircleCI failure is not related to your PR. Something went wrong but I can't reproduce it.
Do you mind triggering a build with an empty commit or a sync with upstream? Thanks for your patience!

@cmarmo
Copy link
Member

cmarmo commented Jul 31, 2020

Thanks @wmvanvliet! And it's green again! :)
@thomasjpfan ... approved? :)

@wmvanvliet
Copy link
Contributor Author

@thomasjpfan?

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan changed the title [MRG] Add alpha_per_target parameter to RidgeCV ENH Add alpha_per_target parameter to RidgeCV Aug 6, 2020
@thomasjpfan thomasjpfan merged commit fd653f8 into scikit-learn:master Aug 6, 2020
@thomasjpfan
Copy link
Member

Thank you for working on this @wmvanvliet !

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow different alphas for different targets in RidgeCV
8 participants