Skip to content

[MRG+1] y not needed in transform in Birch #9066

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

Merged
merged 3 commits into from
Jun 8, 2017

Conversation

agramfort
Copy link
Member

Birch had a y in transform method signature

@rth
Copy link
Member

rth commented Jun 8, 2017

It's also the case for HashingVectorizer, PCA and all classes inheriting from BaseRandomProjection and SparseCodingMixin . Wasn't that about compatibility with the transformers API?

@agramfort
Copy link
Member Author

ok this is taken care of now.

thx @rth

no transform should not have a y. It's not part of the API.

see for example : https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/pipeline.py#L315

@agramfort
Copy link
Member Author

all green :)

@GaelVaroquaux GaelVaroquaux changed the title [MRG] y not needed in transform in Birch [MRG+1] y not needed in transform in Birch Jun 8, 2017
@GaelVaroquaux
Copy link
Member

LGTM. +1 for merge

@amueller
Copy link
Member

amueller commented Jun 8, 2017

I think last time we saw this we said "oh let's wait on the API discussions" but I think we can remove this now.

1 similar comment
@amueller
Copy link
Member

amueller commented Jun 8, 2017

I think last time we saw this we said "oh let's wait on the API discussions" but I think we can remove this now.

@amueller amueller merged commit 04ac57e into scikit-learn:master Jun 8, 2017
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
* y not needed in transform

* y not needed in transform

* more
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
* y not needed in transform

* y not needed in transform

* more
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
* y not needed in transform

* y not needed in transform

* more
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
* y not needed in transform

* y not needed in transform

* more
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
* y not needed in transform

* y not needed in transform

* more
AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
* y not needed in transform

* y not needed in transform

* more
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
* y not needed in transform

* y not needed in transform

* more
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
* y not needed in transform

* y not needed in transform

* more
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants