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

transform should not accept y #8174

Closed
jnothman opened this Issue Jan 8, 2017 · 16 comments

Comments

Projects
None yet
4 participants
@jnothman
Copy link
Member

jnothman commented Jan 8, 2017

I suspect that whenever y=None is included in the transform method signature (after mandatory X), this is done in error. The y parameter should be deprecated and eventually removed. It only confuses the user about the isolation of y from transformation.

@jnothman

This comment has been minimized.

Copy link
Member Author

jnothman commented Jan 8, 2017

$ git grep def.transform.*y=None
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/_function_transformer.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):
sklearn/tests/test_base.py:        def transform(self, X, y=None):
sklearn/tests/test_pipeline.py:    def transform(self, X, y=None):
sklearn/tests/test_pipeline.py:    def transform(self, X, y=None):
@Akshay0724

This comment has been minimized.

Copy link
Contributor

Akshay0724 commented Jan 8, 2017

Hello @jnothman, I'd like to work on this issue.

Are you talking about modification like this--

def transform(self,X,y=None):
if y is not None:
warning.warn("Parameter y is Deprecated for this method as it is not required and will be removed in future version")

@jnothman

This comment has been minimized.

Copy link
Member Author

jnothman commented Jan 8, 2017

@jnothman

This comment has been minimized.

Copy link
Member Author

jnothman commented Jan 8, 2017

@GaelVaroquaux

This comment has been minimized.

Copy link
Member

GaelVaroquaux commented Jan 8, 2017

@jnothman

This comment has been minimized.

Copy link
Member Author

jnothman commented Jan 8, 2017

@GaelVaroquaux please clarify examples. IMO:

  • I can't think of any case y is used in transform atm
  • It is certainly not forwarded by Pipeline or FeatureUnion
  • We have inconsistency as to whether y=None is included in transform's signature
  • But I think including it in transform's signature when it is generally unused confuses users and people writing their own transformers.
  • The documentation has transform with a single positional arg as the prototype.
@jnothman

This comment has been minimized.

Copy link
Member Author

jnothman commented Jan 8, 2017

@Akshay0724

This comment has been minimized.

Copy link
Contributor

Akshay0724 commented Jan 9, 2017

Hello @jnothman, I think it is authentic before starting working on a PR is to ask weather some one else is working on or not. I had started working on this issue and was going to make a PR, in the mean time @tzano directly make a PR.

@jnothman

This comment has been minimized.

Copy link
Member Author

jnothman commented Jan 9, 2017

@Akshay0724

This comment has been minimized.

Copy link
Contributor

Akshay0724 commented Feb 17, 2017

Is work is still do be done in this issue.

@jnothman

This comment has been minimized.

Copy link
Member Author

jnothman commented Feb 19, 2017

@GaelVaroquaux

This comment has been minimized.

Copy link
Member

GaelVaroquaux commented Feb 19, 2017

@jnothman

This comment has been minimized.

Copy link
Member Author

jnothman commented Feb 19, 2017

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Mar 3, 2017

I argued for this before, but @GaelVaroquaux wanted to wait on a ruling on what we're gonna do in terms of a new interface before deprecating anything. I'm still totally for it.

@jnothman

This comment has been minimized.

Copy link
Member Author

jnothman commented Mar 4, 2017

@jnothman

This comment has been minimized.

Copy link
Member Author

jnothman commented Jan 14, 2018

Fixed

@jnothman jnothman closed this Jan 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.