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

[MRG+1] Deprecate ridge_alpha param on SparsePCA.transform() #8137

Merged
merged 5 commits into from Jan 4, 2017

Conversation

@naoyak
Copy link
Contributor

@naoyak naoyak commented Dec 30, 2016

Fixes #1975

Adds a DeprecationWarning if the ridge_alpha parameter is passed to SparsePCA.transform().

Copy link
Member

@jnothman jnothman left a comment

Thanks

@@ -10,6 +10,8 @@
from ..base import BaseEstimator, TransformerMixin
from .dict_learning import dict_learning, dict_learning_online

import warnings

This comment has been minimized.

@jnothman

jnothman Dec 31, 2016
Member

standard library imports should come first

@@ -148,7 +150,8 @@ def transform(self, X, ridge_alpha=None):
ridge_alpha: float, default: 0.01
Amount of ridge shrinkage to apply in order to improve
conditioning.
conditioning. Note that as of 0.19 this parameter is deprecated and

This comment has been minimized.

@jnothman

jnothman Dec 31, 2016
Member

can use .. deprecated sphinx markup

@naoyak naoyak force-pushed the naoyak:sparse-pca-transform branch from b296690 to 732c46c Dec 31, 2016
@naoyak
Copy link
Contributor Author

@naoyak naoyak commented Dec 31, 2016

Thanks @jnothman - made the changes

@@ -150,6 +152,10 @@ def transform(self, X, ridge_alpha=None):
Amount of ridge shrinkage to apply in order to improve
conditioning.
.. deprecated:: 0.18
This parameter will be removed in 0.20.
Specify `ridge_alpha` in the `SparsePCA` constructor.

This comment has been minimized.

@jnothman

jnothman Dec 31, 2016
Member

single backticks don't do anything in RST. Use double backticks.

This comment has been minimized.

@naoyak

naoyak Jan 1, 2017
Author Contributor

Gotcha, thanks.

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 1, 2017

Really, you should set ridge_alpha to 'deprecated' by default, so that if a user explicitly passes ridge_alpha=None it will still raise the warnin

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 1, 2017

Also, please check for a test where transform with ridge_alpha is used, and apply assert_warns in the test to silence the warning during tests but ensure it is raised

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 1, 2017

Travis failures are not your fault.

@naoyak
Copy link
Contributor Author

@naoyak naoyak commented Jan 1, 2017

@jnothman should the deprecated param be ignored, or should it still work for now? I've seen both patterns elsewhere in the codebase.

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 1, 2017

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 1, 2017

@naoyak
Copy link
Contributor Author

@naoyak naoyak commented Jan 1, 2017

Where do you see the deprecated param ignored? Yes, it should still work
for now.

I saw it ignored in #7992: 2e79d88#r91984196 - but without being familiar with that issue I'm not sure if the context is different.

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 1, 2017

@naoyak
Copy link
Contributor Author

@naoyak naoyak commented Jan 1, 2017

Sorry, I think that comment was an outdated diff. But it looks like current master for that perplexity method still throws away the deprecated arg. https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/decomposition/online_lda.py#L750

@naoyak naoyak force-pushed the naoyak:sparse-pca-transform branch from 7f08889 to 409509b Jan 1, 2017
@@ -70,6 +71,10 @@ def test_fit_transform():
spca_lasso.fit(Y)
assert_array_almost_equal(spca_lasso.components_, spca_lars.components_)

# Test calling tranform with deprecated ridge_alpha parameter

This comment has been minimized.

@naoyak

naoyak Jan 1, 2017
Author Contributor

Seeing the note about testing times with SparsePCA I stuck the deprecation test in here. Lmk if you think it's better to break it out.

This comment has been minimized.

@jnothman

jnothman Jan 1, 2017
Member

*transform

Here is fine. It's frustrating that this parameter was not already tested.

@naoyak naoyak changed the title Deprecate ridge_alpha param on SparsePCA.transform() [MRG] Deprecate ridge_alpha param on SparsePCA.transform() Jan 1, 2017
@naoyak naoyak force-pushed the naoyak:sparse-pca-transform branch from 409509b to 806901a Jan 1, 2017
@jnothman
Copy link
Member

@jnothman jnothman commented Jan 1, 2017

Copy link
Member

@jnothman jnothman left a comment

Otherwise LGTM

@@ -70,6 +71,10 @@ def test_fit_transform():
spca_lasso.fit(Y)
assert_array_almost_equal(spca_lasso.components_, spca_lars.components_)

# Test calling tranform with deprecated ridge_alpha parameter

This comment has been minimized.

@jnothman

jnothman Jan 1, 2017
Member

*transform

Here is fine. It's frustrating that this parameter was not already tested.

@@ -70,6 +71,10 @@ def test_fit_transform():
spca_lasso.fit(Y)
assert_array_almost_equal(spca_lasso.components_, spca_lars.components_)

# Test calling tranform with deprecated ridge_alpha parameter
ridge_alpha = 0.01
assert_warns(DeprecationWarning, spca_lars.transform, Y, ridge_alpha)

This comment has been minimized.

@jnothman

jnothman Jan 1, 2017
Member

I'd rather ridge_alpha=0.01 there, in part because really transform should accept a second positional arg.

This comment has been minimized.

@naoyak

naoyak Jan 1, 2017
Author Contributor

Also added an example where ridge_alpha=None to make sure it's handled properly

@jnothman jnothman changed the title [MRG] Deprecate ridge_alpha param on SparsePCA.transform() [MRG+1] Deprecate ridge_alpha param on SparsePCA.transform() Jan 1, 2017
Copy link
Member

@jnothman jnothman left a comment

but then I found more...

"deprecated since 0.19 and will be removed in 0.21. "
"Specify ridge_alpha in the SparsePCA constructor.",
DeprecationWarning)
else:

This comment has been minimized.

@jnothman

jnothman Jan 1, 2017
Member

This is going to break if ridge_alpha=None is passed.

This comment has been minimized.

@jnothman

jnothman Jan 1, 2017
Member

Please test this case.

This comment has been minimized.

@naoyak

naoyak Jan 1, 2017
Author Contributor

Thanks for the catch. In this case I have it reverting to self.ridge_alpha.

@naoyak
Copy link
Contributor Author

@naoyak naoyak commented Jan 1, 2017

@jnothman thanks for the detailed check, should be good now. Tests working again!

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 1, 2017

Yes, again LGTM

Copy link
Contributor

@tguillemot tguillemot left a comment

+1 as my comment is not really important.

@@ -70,6 +71,10 @@ def test_fit_transform():
spca_lasso.fit(Y)
assert_array_almost_equal(spca_lasso.components_, spca_lars.components_)

# Test that deprecated ridge_alpha parameter throws warning
assert_warns(DeprecationWarning, spca_lars.transform, Y, ridge_alpha=0.01)
assert_warns(DeprecationWarning, spca_lars.transform, Y, ridge_alpha=None)

This comment has been minimized.

@tguillemot

tguillemot Jan 3, 2017
Contributor

Just a niptick, it will be better to use assert_warns_message with a part of the message (just to be sure it's the right message).

This comment has been minimized.

@naoyak

naoyak Jan 3, 2017
Author Contributor

Thanks for the tip

@naoyak naoyak force-pushed the naoyak:sparse-pca-transform branch from c60db80 to 7cfe756 Jan 3, 2017
@jnothman jnothman merged commit 84349a7 into scikit-learn:master Jan 4, 2017
3 checks passed
3 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jnothman
Copy link
Member

@jnothman jnothman commented Jan 4, 2017

Thanks @naoyak!

@naoyak naoyak deleted the naoyak:sparse-pca-transform branch Jan 4, 2017
@tguillemot
Copy link
Contributor

@tguillemot tguillemot commented Jan 4, 2017

Thx @naoyak !!!

raghavrv added a commit to raghavrv/scikit-learn that referenced this pull request Jan 5, 2017
sergeyf added a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017
@Przemo10 Przemo10 mentioned this pull request Mar 17, 2017
Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.