[MRG+1] Deprecate Y parameter on transform() #8403

tzano · 2017-02-19T20:33:41Z

As further discussed on thread #8174, the parameter Y in method transform() will be deprecated and eventually removed in a future version .

The method transform() has been used in the following:

sklearn/cluster/birch.py:    def transform(self, X, y=None):
sklearn/cluster/k_means_.py:    def transform(self, X, y=None):
sklearn/decomposition/base.py:    def transform(self, X, y=None):
sklearn/decomposition/dict_learning.py:    def transform(self, X, y=None):
sklearn/decomposition/fastica_.py:    def transform(self, X, y=None, copy=True):
sklearn/decomposition/pca.py:    def transform(self, X, y=None):
sklearn/feature_extraction/dict_vectorizer.py:    def transform(self, X, y=None):
sklearn/feature_extraction/hashing.py:    def transform(self, raw_X, y=None):
sklearn/feature_extraction/text.py:    def transform(self, X, y=None):
sklearn/kernel_approximation.py:    def transform(self, X, y=None):
sklearn/kernel_approximation.py:    def transform(self, X, y=None):
sklearn/kernel_approximation.py:    def transform(self, X, y=None):
sklearn/neighbors/approximate.py:    def transform(self, X, y=None):
sklearn/preprocessing/data.py:    def transform(self, X, y=None, copy=None):
sklearn/preprocessing/data.py:    def transform(self, X, y=None):
sklearn/preprocessing/data.py:    def transform(self, X, y=None):
sklearn/preprocessing/data.py:    def transform(self, X, y=None):
sklearn/preprocessing/data.py:    def transform(self, X, y=None, copy=None):
sklearn/preprocessing/data.py:    def transform(self, X, y=None, copy=None):
sklearn/preprocessing/data.py:    def transform(self, K, y=None, copy=True):
sklearn/preprocessing/data.py:    def transform(self, X, y=None):
sklearn/random_projection.py:    def transform(self, X, y=None):

I didn't change this one, as Y is used.

sklearn/preprocessing/_function_transformer.py:    def transform(self, X, y=None):

I have added deprecation message

        if y != 'deprecated':
            warnings.warn("The parameter y on transform() is "
                          "deprecated since 0.19 and will be removed in 0.21. ",
                          DeprecationWarning)

codecov-io · 2017-02-19T20:49:19Z

Codecov Report

Merging #8403 into master will decrease coverage by -0.03%.
The diff coverage is 77.17%.

@@            Coverage Diff             @@
##           master    #8403      +/-   ##
==========================================
- Coverage   95.47%   95.45%   -0.03%     
==========================================
  Files         342      342              
  Lines       60907    60975      +68     
==========================================
+ Hits        58154    58202      +48     
- Misses       2753     2773      +20

Impacted Files	Coverage Δ
sklearn/tests/test_base.py	`97.6% <ø> (ø)`	✅
sklearn/tests/test_pipeline.py	`99.61% <100%> (ø)`	✅
sklearn/decomposition/base.py	`98.14% <100%> (+0.14%)`	✅
sklearn/decomposition/tests/test_pca.py	`100% <100%> (ø)`	✅
sklearn/cluster/k_means_.py	`97.43% <66.66%> (-0.23%)`	❌
sklearn/preprocessing/data.py	`98.02% <68.18%> (-1.24%)`	❌
sklearn/kernel_approximation.py	`98.18% <70%> (-1.82%)`	❌
sklearn/cluster/birch.py	`99.14% <75%> (-0.43%)`	❌
sklearn/decomposition/fastica_.py	`93.33% <75%> (-0.46%)`	❌
sklearn/neighbors/approximate.py	`98.46% <75%> (-0.5%)`	❌
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 067adad...9e2986c. Read the comment docs.

jnothman

if y != 'deprecated' will raise an error if y is an array
this may be worth testing. Perhaps we should add a common test which checks that either you get an "invalid keyword" TypeError for passing y=something or you get a DeprecationWarning and it functions as usual.

jnothman · 2017-02-20T00:37:08Z

sklearn/feature_extraction/dict_vectorizer.py

@@ -289,6 +291,10 @@ def transform(self, X, y=None):
        Xa : {array, sparse matrix}
            Feature vectors; always 2-d.
        """
+        if y != 'deprecated':


Please remove the dosctring entry for y above (or mark it deprecated)

jnothman · 2017-02-20T00:37:29Z

sklearn/feature_extraction/hashing.py

@@ -133,6 +135,11 @@ def transform(self, raw_X, y=None):
            Feature matrix, for use with estimators or further transformers.

        """
+        if y != 'deprecated':


Please update docstring.

jnothman · 2017-02-20T00:37:38Z

sklearn/feature_extraction/text.py

@@ -478,6 +479,11 @@ def transform(self, X, y=None):
            Document-term matrix.

        """
+        if y != 'deprecated':


Please update docstring.

jnothman · 2017-02-20T00:38:24Z

sklearn/random_projection.py

@@ -402,6 +402,11 @@ def transform(self, X, y=None):
            Projected array.

        """
+        if y != 'deprecated':


Please update docstring.

jnothman · 2017-02-20T00:38:36Z

sklearn/tests/test_base.py

@@ -300,7 +300,7 @@ def __init__(self, df=None, scalar_param=1):
        def fit(self, X, y=None):
            pass

-        def transform(self, X, y=None):
+        def transform(self, X, y='deprecated'):


just drop y in tests

jnothman · 2017-02-20T00:38:41Z

sklearn/tests/test_pipeline.py

@@ -67,12 +67,12 @@ def set_params(self, **params):


 class NoInvTransf(NoTrans):
-    def transform(self, X, y=None):
+    def transform(self, X, y='deprecated'):


just drop y in tests

jnothman · 2017-02-20T00:38:45Z

sklearn/tests/test_pipeline.py

        return X


 class Transf(NoInvTransf):
-    def transform(self, X, y=None):
+    def transform(self, X, y='deprecated'):


just drop y in tests

tzano · 2017-02-20T20:34:59Z

Thanks @jnothman

if y != 'deprecated' will raise an error if y is an array

In this case, should we do something like:

    if isinstance(y, np.ndarray) or y != 'deprecated':
        warnings.warn("The parameter y on transform() is "
                      "deprecated since 0.19 and will be removed in 0.21",
                      DeprecationWarning)

jnothman · 2017-02-20T22:37:11Z

Better assured doing isinstance(..., six.string_types) as elsewhere in the codebase

…

On 21 Feb 2017 7:35 am, "Tahar" ***@***.***> wrote: Thanks @jnothman <https://github.com/jnothman> if y != 'deprecated' will raise an error if y is an array In this case, should we do something like: if isinstance(y, np.ndarray) or y != 'deprecated': warnings.warn("The parameter y on transform() is " "deprecated since 0.19 and will be removed in 0.21", DeprecationWarning) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8403 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz63q7GsfUtHqQM7joGxFwaIFY7CLbks5refj1gaJpZM4MFlT2> .

jnothman

LGTM

jnothman · 2017-02-21T00:23:54Z

sklearn/decomposition/base.py

@@ -125,6 +128,11 @@ def transform(self, X, y=None):
        IncrementalPCA(batch_size=3, copy=True, n_components=2, whiten=False)
        >>> ipca.transform(X) # doctest: +SKIP
        """
+        if isinstance(y, np.ndarray) or y != 'deprecated':


missing not

tzano · 2017-02-21T07:10:20Z

Thanks @jnothman. I have fixed the issue you mentioned. Let me know if we need to add something else.

lesteve · 2017-02-21T18:24:36Z

I didn't change this one, as Y is used.
sklearn/preprocessing/_function_transformer.py: def transform(self, X, y=None):

I haven't looked in detail why y is used but I do believe this should be deprecated like all the other y arguments. Granted this may be a bit more subtle than the other cases but it still needs to be done.

tzano · 2017-02-21T19:05:23Z

Thanks @lesteve . @jnothman already suggested not to deprecate it for now. Hope he has a moment to chime in.

jnothman · 2017-02-21T20:41:19Z

in some ways i would rather it deprecated. but given how generic it is, it might be okay to leave it there to support behaviour akin to cross-decomposition models.

…

On 22 Feb 2017 6:05 am, "Tahar" ***@***.***> wrote: Thanks @lesteve <https://github.com/lesteve> . @jnothman <https://github.com/jnothman> already suggested not to deprecate it for now. Hope he has a moment to chime in. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8403 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz62N43v47YmEGjMIWnRX-7WrzXIPbks5rezV1gaJpZM4MFlT2> .

lesteve · 2017-02-21T21:23:39Z

~~Indeed I missed it in the first PR you opened #8177 (comment) indeed. It is not clear to me what the reason behind was though.~~

Sorry did not get the last message, ignore this. Fair enough for not deprecating y in sklearn/preprocessing/_function_transformer.py

lesteve · 2017-02-21T21:26:17Z

Can you add an entry in doc/whats_new.rst about the change?

tzano · 2017-02-21T22:19:45Z

Thanks @lesteve . doc/whats_new.rst has been updated.

lesteve · 2017-02-23T16:49:20Z

Not sure what you did with your git history, but you will need to tidy it up before we can merge this PR.

tzano · 2017-02-23T19:07:09Z

Thanks @lesteve. Ignore the last changes.
Let me know if I need to fix other things for this PR.

lesteve · 2017-02-24T07:37:28Z

doc/whats_new.rst

@@ -255,6 +255,12 @@ API changes summary
      selection classes to be used with tools such as
      :func:`sklearn.model_selection.cross_val_predict`.
      :issue:`2879` by :user:`Stephen Hoover <stephen-hoover>`.
+
+   - Deprecate the ``Y`` parameter from ``transform`` method signature in 


Can you format this correctly. Respect the indentation whatever the number of spaces is. I don't think you need to list all the classes.

Also this is a small y not a capital one

lesteve · 2017-02-24T07:37:49Z

doc/whats_new.rst

@@ -245,7 +245,7 @@ API changes summary
     (``n_samples``, ``n_classes``) for that particular output.
     :issue:`8093` by :user:`Peter Bull <pjbull>`.

-    - Deprecate the ``fit_params`` constructor input to the
+   - Deprecate the ``fit_params`` constructor input to the


Refrain from making changes that are not related to your PR.

lesteve · 2017-02-24T07:38:57Z

doc/whats_new.rst

+
+   - Deprecate the ``Y`` parameter from ``transform`` method signature in 
+      :class:`sklearn.cluster.birch` :class:`sklearn.cluster.k_means_` :class:`sklearn.decomposition.base` :class:`sklearn.decomposition.dict_learning` :class:`sklearn.decomposition.fastica_` :class:`sklearn.decomposition.pca` :class:`sklearn.feature_extraction.dict_vectorizer` :class:`sklearn.feature_extraction.hashing` :class:`sklearn.feature_extraction.text` :class:`sklearn.kernel_approximation` :class:`sklearn.neighbors.approximate` :class:`sklearn.preprocessing._function_transformer` :class:`sklearn.preprocessing.data` :class:`sklearn.random_projection`
+      The method  should not accept ``Y`` parameter, as ``transform()`` is used at the prediction time. ``Y`` parameter is deprecated on this version and will be removed in 0.21.


This line is way too long.

No need to say when it will be removed.

lesteve · 2017-02-24T13:32:01Z

doc/whats_new.rst

-*   1	Andrew Ash
-*   1	Pietro Zambelli
-*   1	staubda
+* 312 Olivier Grisel


Please review your diffs and do not change anything not related to your PR.

Pushed by mistake, I am reverting changes.

agramfort · 2017-06-13T15:56:51Z

I took the commits in #9075 closing.

thanks @tzano

added Deprecation Warning to transform() to remove Y parameter

7d751b3

tzano changed the title ~~Deprecate Y parameter on transform()~~ [WIP] Deprecate Y parameter on transform() Feb 19, 2017

updated Deprecation Warning message

9bfa823

tzano added 3 commits February 19, 2017 23:55

updated Deprecation Warning message

152cc73

added default val to transform()

4bdf2c2

added missing warning message

d729953

jnothman reviewed Feb 20, 2017

View reviewed changes

tzano added 2 commits February 21, 2017 02:57

updated docstring + added test case

4596d3c

fix travis problem

4a21c0e

jnothman reviewed Feb 21, 2017

View reviewed changes

jnothman changed the title ~~[WIP] Deprecate Y parameter on transform()~~ [MRG+1] Deprecate Y parameter on transform() Feb 21, 2017

tzano added 2 commits February 21, 2017 09:17

added missing cases

128c5e7

added missing cases

935ba33

tzano added 2 commits February 22, 2017 01:08

added changes log entry to doc/whats_new.rst

2640aa3

added changes log entry to doc/whats_new.rst

9c6677a

updated whats_new.rst

83a6c27

tzano force-pushed the p-8174 branch from a972410 to 83a6c27 Compare February 23, 2017 19:01

lesteve requested changes Feb 24, 2017

View reviewed changes

lesteve reviewed Feb 24, 2017

View reviewed changes

tzano force-pushed the p-8174 branch 2 times, most recently from 1851028 to 83a6c27 Compare February 24, 2017 13:57

updated doc/whats_new.rst

9e2986c

jnothman mentioned this pull request Jun 12, 2017

[WIP] we do not transform y #9075

Closed

agramfort closed this Jun 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] Deprecate Y parameter on transform() #8403

[MRG+1] Deprecate Y parameter on transform() #8403

tzano commented Feb 19, 2017 •

edited

Loading

codecov-io commented Feb 19, 2017 •

edited by codecov bot

Loading

jnothman left a comment

jnothman Feb 20, 2017

jnothman Feb 20, 2017

jnothman Feb 20, 2017

jnothman Feb 20, 2017

jnothman Feb 20, 2017

jnothman Feb 20, 2017

jnothman Feb 20, 2017

tzano commented Feb 20, 2017 •

edited

Loading

jnothman commented Feb 20, 2017 via email

jnothman left a comment

jnothman Feb 21, 2017

tzano commented Feb 21, 2017

lesteve commented Feb 21, 2017

tzano commented Feb 21, 2017

jnothman commented Feb 21, 2017 via email

lesteve commented Feb 21, 2017 •

edited

Loading

lesteve commented Feb 21, 2017

tzano commented Feb 21, 2017

lesteve commented Feb 23, 2017

tzano commented Feb 23, 2017

lesteve Feb 24, 2017

lesteve Feb 24, 2017

lesteve Feb 24, 2017

lesteve Feb 24, 2017

tzano Feb 24, 2017

agramfort commented Jun 13, 2017

[MRG+1] Deprecate Y parameter on transform() #8403

[MRG+1] Deprecate Y parameter on transform() #8403

Conversation

tzano commented Feb 19, 2017 • edited Loading

codecov-io commented Feb 19, 2017 • edited by codecov bot Loading

Codecov Report

jnothman 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzano commented Feb 20, 2017 • edited Loading

jnothman commented Feb 20, 2017 via email

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzano commented Feb 21, 2017

lesteve commented Feb 21, 2017

tzano commented Feb 21, 2017

jnothman commented Feb 21, 2017 via email

lesteve commented Feb 21, 2017 • edited Loading

lesteve commented Feb 21, 2017

tzano commented Feb 21, 2017

lesteve commented Feb 23, 2017

tzano commented Feb 23, 2017

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

agramfort commented Jun 13, 2017

tzano commented Feb 19, 2017 •

edited

Loading

codecov-io commented Feb 19, 2017 •

edited by codecov bot

Loading

tzano commented Feb 20, 2017 •

edited

Loading

lesteve commented Feb 21, 2017 •

edited

Loading