Skip to content

[MRG+1] Do not transform y #9180

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jun 22, 2017
Merged

[MRG+1] Do not transform y #9180

merged 12 commits into from
Jun 22, 2017

Conversation

raghavrv
Copy link
Member

@raghavrv raghavrv commented Jun 20, 2017 •
edited
Loading

This takes over @agramfort's PR #9075

For the PR specific to PLS see #9160

  • Fix failing CIs

@raghavrv raghavrv changed the title [WIP] Do not transform y [MRG] Do not transform y Jun 21, 2017
@raghavrv
Copy link
Member Author

@agramfort @amueller This should now ensure all CIs pass. Can you review this?

@raghavrv raghavrv added this to the 0.19 milestone Jun 21, 2017
@raghavrv raghavrv requested a review from jnothman June 21, 2017 09:33
@raghavrv raghavrv mentioned this pull request Jun 21, 2017
Closed
@raghavrv raghavrv requested a review from amueller June 21, 2017 14:08
@raghavrv
Copy link
Member Author

(The codecov seems to pull up test misses that are not relevant to this PR)

jnothman
jnothman reviewed Jun 21, 2017
View reviewed changes
sklearn/decomposition/fastica_.py
@@ -528,22 +531,29 @@ def fit(self, X, y=None):
self._fit(X, compute_sources=False)
return self

def transform(self, X, y=None, copy=True):
def transform(self, X, y='deprecated', copy=True):
Copy link
Member

Choose a reason for hiding this comment

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

Why deprecation here and not elsewhere?

Copy link
Member Author

@raghavrv raghavrv Jun 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

sorry for that will push a commit fixing this.

Copy link
Member

Choose a reason for hiding this comment

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

because here there is copy param so we cannot just remove y from the signature like above.

agramfort
agramfort reviewed Jun 21, 2017
View reviewed changes
doc/whats_new.rst Outdated
@@ -539,6 +539,11 @@ API changes summary
- ``utils.stats.rankdata``
- ``neighbors.approximate.LSHForest``

- Deprecate the ``y`` parameter in :meth:`transform`.
Copy link
Member

Choose a reason for hiding this comment

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

and :meth:inverse_transform

not sure this renders ok though as it should not point to anything specific

agramfort
agramfort reviewed Jun 21, 2017
View reviewed changes
doc/whats_new.rst Outdated
@@ -539,6 +539,11 @@ API changes summary
- ``utils.stats.rankdata``
- ``neighbors.approximate.LSHForest``

- Deprecate the ``y`` parameter in :meth:`transform`.
The method should not accept ``y`` parameter, as it's used at the prediction time.
:issue:`8174` by :user:`Tahar Zanouda <tzano>`.
Copy link
Member

Choose a reason for hiding this comment

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

change this

@agramfort
Copy link
Member

besides LGTM. Thx @raghavrv

agramfort
agramfort reviewed Jun 22, 2017
View reviewed changes
doc/whats_new.rst Outdated
- Deprecate the ``y`` parameter in `transform` and `inverse_transform`.
The method should not accept ``y`` parameter, as it's used at the prediction time.
:issue:`8174` by :user:`Tahar Zanouda <tzano>`, :user:`Alexandre Gramfort`_
and :user:`Raghav RV`_.
Copy link
Member

Choose a reason for hiding this comment

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

I doubt this renders fine for you and me

I think for me it should only be Alexandre Gramfort_ as below

@raghavrv raghavrv force-pushed the do_not_transform_y branch from 11d318a to 223a5b3 Compare June 22, 2017 13:02
@agramfort agramfort changed the title [MRG] Do not transform y [MRG+1] Do not transform y Jun 22, 2017
@agramfort
Copy link
Member

LGTM +1 for MRG

@raghavrv
Copy link
Member Author

thanks for the review!

jnothman
jnothman reviewed Jun 22, 2017
View reviewed changes
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.

I'm okay with this. But I am also sure we'll break someone's code; someone who passed y to transform because the signature asked for it.

sklearn/cross_decomposition/pls_.py Outdated
X : array-like of predictors, shape = [n_samples, p]
Training vectors, where n_samples in the number of samples and
p is the number of predictors.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Did we agree that we're best to just leave Y undocumented in PLS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry this is an unrelated change. I'll revert it. I want to leave PLS issue for another PR ( #9160 ) and the documentation to yet another. (#9023)

@raghavrv
Copy link
Member Author

I'm okay with this. But I am also sure we'll break someone's code; someone who passed y to transform because the signature asked for it.

We set a precedent for that here - https://github.com/scikit-learn/scikit-learn/pull/9066/files

Could I factor in your +1 for a merge? :)

@TomDLT
Copy link
Member

TomDLT commented Jun 22, 2017

LGTM

But I am also sure we'll break someone's code

@raghavrv
Copy link
Member Author

With Alex's and Tom's +1, in it goes! Thanks everyone.

@raghavrv raghavrv merged commit 763d93b into scikit-learn:master Jun 22, 2017
@raghavrv raghavrv deleted the do_not_transform_y branch June 22, 2017 21:24
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@raghavrv @dmohns
[MRG+1] Do not transform y (scikit-learn#9180)
b3ea177 
* we do not transform y

* more

* added Deprecation Warning to transform() to remove Y parameter

* more

* ENH ensure FunctionTransformer's transform/inverse_transform doesn't permit y

* Undo changes to pls_. It will be done in a separate PR (see scikit-learn#9160)

* flake8

* Update whatsnew

* Fully undo PLS changes
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@raghavrv @dmohns
[MRG+1] Do not transform y (scikit-learn#9180)
68906c3 
* we do not transform y

* more

* added Deprecation Warning to transform() to remove Y parameter

* more

* ENH ensure FunctionTransformer's transform/inverse_transform doesn't permit y

* Undo changes to pls_. It will be done in a separate PR (see scikit-learn#9160)

* flake8

* Update whatsnew

* Fully undo PLS changes
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
@raghavrv @NelleV
[MRG+1] Do not transform y (scikit-learn#9180)
927c8dd 
* we do not transform y

* more

* added Deprecation Warning to transform() to remove Y parameter

* more

* ENH ensure FunctionTransformer's transform/inverse_transform doesn't permit y

* Undo changes to pls_. It will be done in a separate PR (see scikit-learn#9160)

* flake8

* Update whatsnew

* Fully undo PLS changes
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@raghavrv @paulha
[MRG+1] Do not transform y (scikit-learn#9180)
0408e07 
* we do not transform y

* more

* added Deprecation Warning to transform() to remove Y parameter

* more

* ENH ensure FunctionTransformer's transform/inverse_transform doesn't permit y

* Undo changes to pls_. It will be done in a separate PR (see scikit-learn#9160)

* flake8

* Update whatsnew

* Fully undo PLS changes
AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
@raghavrv @AishwaryaRK
[MRG+1] Do not transform y (scikit-learn#9180)
82c4f60 
* we do not transform y

* more

* added Deprecation Warning to transform() to remove Y parameter

* more

* ENH ensure FunctionTransformer's transform/inverse_transform doesn't permit y

* Undo changes to pls_. It will be done in a separate PR (see scikit-learn#9160)

* flake8

* Update whatsnew

* Fully undo PLS changes
@grantzvolsky
Copy link

Is there an alternative way of doing a "supervised transformation" now that the "y" parameter is unavailable?

@TomDLT
Copy link
Member

TomDLT commented Nov 2, 2017

y is only unavailable for the transform method, not for the fit method.
Similarly, you won't find any y parameter in the predict method of classifiers/regressors.

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@raghavrv @maskani-moh
[MRG+1] Do not transform y (scikit-learn#9180)
9bc1d25 
* we do not transform y

* more

* added Deprecation Warning to transform() to remove Y parameter

* more

* ENH ensure FunctionTransformer's transform/inverse_transform doesn't permit y

* Undo changes to pls_. It will be done in a separate PR (see scikit-learn#9160)

* flake8

* Update whatsnew

* Fully undo PLS changes
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@raghavrv
[MRG+1] Do not transform y (scikit-learn#9180)
291a365 
* we do not transform y

* more

* added Deprecation Warning to transform() to remove Y parameter

* more

* ENH ensure FunctionTransformer's transform/inverse_transform doesn't permit y

* Undo changes to pls_. It will be done in a separate PR (see scikit-learn#9160)

* flake8

* Update whatsnew

* Fully undo PLS changes
@amueller amueller mentioned this pull request Aug 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman left review comments

@agramfort agramfort agramfort left review comments

@amueller amueller Awaiting requested review from amueller

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.19
Development

Successfully merging this pull request may close these issues.
6 participants
@raghavrv @agramfort @TomDLT @grantzvolsky @jnothman @tzano