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

naoyak · 2016-12-30T04:50:39Z

jnothman

jnothman · 2016-12-31T14:07:04Z

sklearn/decomposition/sparse_pca.py

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

+import warnings


standard library imports should come first

jnothman · 2016-12-31T14:19:15Z

sklearn/decomposition/sparse_pca.py

@@ -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


can use .. deprecated sphinx markup

naoyak · 2016-12-31T23:49:11Z

jnothman · 2016-12-31T23:47:47Z

sklearn/decomposition/sparse_pca.py

@@ -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.


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

Gotcha, thanks.

jnothman · 2017-01-01T00:17:19Z

jnothman · 2017-01-01T00:18:05Z

jnothman · 2017-01-01T00:18:24Z

naoyak · 2017-01-01T01:08:14Z

jnothman · 2017-01-01T01:31:21Z

jnothman · 2017-01-01T01:31:32Z

naoyak · 2017-01-01T01:37:21Z

jnothman · 2017-01-01T02:11:08Z

naoyak · 2017-01-01T02:19:16Z

naoyak · 2017-01-01T03:53:24Z

sklearn/decomposition/tests/test_sparse_pca.py

@@ -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


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.

*transform

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

jnothman · 2017-01-01T09:40:47Z

jnothman

jnothman · 2017-01-01T09:47:35Z

sklearn/decomposition/tests/test_sparse_pca.py

@@ -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


*transform

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

jnothman · 2017-01-01T09:48:10Z

sklearn/decomposition/tests/test_sparse_pca.py

@@ -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)


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

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

jnothman

jnothman · 2017-01-01T09:49:38Z

sklearn/decomposition/sparse_pca.py

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


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

Please test this case.

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

naoyak · 2017-01-01T11:12:34Z

jnothman · 2017-01-01T11:26:51Z

tguillemot

tguillemot · 2017-01-03T10:57:20Z

sklearn/decomposition/tests/test_sparse_pca.py

@@ -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)


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).

Thanks for the tip

jnothman · 2017-01-04T08:42:13Z

tguillemot · 2017-01-04T08:54:17Z

Deprecate ridge_alpha param on SparsePCA.transform()

fe539bc

jnothman reviewed Dec 31, 2016

View reviewed changes

Use deprecated sphinx syntax for SparsePCA docs

732c46c

naoyak force-pushed the sparse-pca-transform branch from b296690 to 732c46c Compare December 31, 2016 23:48

jnothman reviewed Dec 31, 2016

View reviewed changes

naoyak force-pushed the sparse-pca-transform branch from 7f08889 to 409509b Compare January 1, 2017 03:50

naoyak commented Jan 1, 2017

View reviewed changes

naoyak changed the title ~~Deprecate ridge_alpha param on SparsePCA.transform()~~ [MRG] Deprecate ridge_alpha param on SparsePCA.transform() Jan 1, 2017

Added test for SparsePCA transform param deprecation

806901a

naoyak force-pushed the sparse-pca-transform branch from 409509b to 806901a Compare January 1, 2017 05:56

jnothman reviewed Jan 1, 2017

View reviewed changes

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

jnothman requested changes Jan 1, 2017

View reviewed changes

Check that ridge_alpha is not None on SparsePCA.transform()

442c404

naoyak mentioned this pull request Jan 2, 2017

[MRG + 1] Fix perplexity method by adding _unnormalized_transform method, Issue #7954 #7992

Merged

tguillemot approved these changes Jan 3, 2017

View reviewed changes

Check deprecation message in SparsePCA test

7cfe756

naoyak force-pushed the sparse-pca-transform branch from c60db80 to 7cfe756 Compare January 3, 2017 20:26

jnothman approved these changes Jan 4, 2017

View reviewed changes

jnothman merged commit 84349a7 into scikit-learn:master Jan 4, 2017

naoyak deleted the sparse-pca-transform branch January 4, 2017 08:45

raghavrv pushed a commit to raghavrv/scikit-learn that referenced this pull request Jan 5, 2017

[MRG+1] Deprecate ridge_alpha param on SparsePCA.transform() (scikit-…

81f5b3e

…learn#8137)

sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

[MRG+1] Deprecate ridge_alpha param on SparsePCA.transform() (scikit-…

2edc335

…learn#8137)

Przemo10 mentioned this pull request Mar 17, 2017

update fork (#1) #8606

Closed

Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG+1] Deprecate ridge_alpha param on SparsePCA.transform() (scikit-…

8312a92

…learn#8137)

NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG+1] Deprecate ridge_alpha param on SparsePCA.transform() (scikit-…

674c8d9

…learn#8137)

paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG+1] Deprecate ridge_alpha param on SparsePCA.transform() (scikit-…

3458f2e

…learn#8137)

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG+1] Deprecate ridge_alpha param on SparsePCA.transform() (scikit-…

fa8e1eb

…learn#8137)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

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

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

Conversation

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment