Okay, I guess that's one option; the alternative is to not pass y if it's None, as is done in GridSearchCV for fit.

I am just now a bit confused. Pipeline always passes y=None to fit. Does that mean many clustering algorithms don't work with pipeline?!
Currently everything but the fit in some clustering algorithms and the score in KMeans has an optional y=None.

we should do it consistently, but I guess it is to late for that now?

I actually prefer the GridSearchCV approach, but it looks like we went the other way until now.

import numpy as np,sklearn
from sklearn.pipeline import Pipeline
from sklearn.cluster import MeanShift
X=np.random.rand(100,2)
p=Pipeline([('clf',MeanShift())])
p.fit(X)

breaks and makes me sad.

For the pipeline, it could be that previous steps in the pipeline use y so that it is not None, though. So you could do LDA, and then do clustering.

Don't be sad. As Gaël likes to say: it's not version 1 yet, stuff's allowed
to break and change :)

I can't say I have the answer here, yet, but I think you're thinking about
it the right way. Do we really require all estimators to take 2 positional
args to fit, then?

On 8 January 2015 at 13:35, Andreas Mueller notifications@github.com
wrote:

For the pipeline, it could be that previous steps in the pipeline use y
so that it is not None, though. So you could do LDA, and then do
clustering.

—
Reply to this email directly or view it on GitHub
#4064 (comment)
.

Yeah, I think this is the only way to make pipeline work as expected. Testing y is None is not good as in my example. The other option would be to use introspection or try but that is not the approach we have taken so far.

reviews on this one would be nice as it makes testing somewhat simpler.

This one will simplify future testing such as #4162 and #4136 by the way....

Firstly, doc/developers/index.rst needs fixing. Secondly, I think this change should encompass partial_fit: RBM, LSHForest, and IPCA are current offenders.

What is the benefit in partial_fit apart from making the tests easier to write?

What is the benefit in partial_fit apart from making the tests easier to write?

Consistency with fit, for one; the polymorphic design principle that Gaël expressed elsewhere; the ability to remove an if-else clause in sklearn.learning_curve; and anticipation of meta-estimators employing partial_fit in the future or in external projects where the developer may otherwise forget the unsupervised case...

Fair enough.
Where would you put this in the developers doc? The current y argument of Transformers is not documented anywhere as far as I can see :-/

Actually, there was a comment about fit in there, something that was not tested / enforced. I added comments about transform and score but I'm not sure that is the best place.

Oh, I didn't look at it closely enough. It actually gives an invocation
spec, not a def signature. But maybe it should:
http://scikit-learn.org/dev/developers/index.html#different-objects

On 29 January 2015 at 10:32, Andreas Mueller notifications@github.com
wrote:

Where?

—
Reply to this email directly or view it on GitHub
#4064 (comment)
.

Obscurely, it shows 1-ary score as an example.

On 29 January 2015 at 14:05, Joel Nothman joel.nothman@sydney.edu.au
wrote:

Oh, I didn't look at it closely enough. It actually gives an invocation
spec, not a def signature. But maybe it should:
http://scikit-learn.org/dev/developers/index.html#different-objects

On 29 January 2015 at 10:32, Andreas Mueller notifications@github.com
wrote:

Where?

—
Reply to this email directly or view it on GitHub
#4064 (comment)
.

Great. This LGTM apart from your test failure, which I take it will be fixed before the next reviewer gives a +1 ;)

Thanks for the review. Damn, I thought I just fixed travis. Will look at it again.

Fixed.

Any other reviews? @ogrisel maybe?

As the motivation of this change was to make it possible to call the clustering models in pipelines I would like to integrate a non-regression "integration" test in the spirit of #4064 (comment) in test_common. Note: clf is a bad name for such as step, model or estimator would be a better name. Or even, just use make_pipeline for that test.

Other than that +1 on my side.

Hum, I totally wanted to do that integration test with pipelines, seems I forgot about it. Not sure what should be tested, probably "everything". This PR has tests for the "API definition' of Pipeline, not doing the integration.
I'll try that tomorrow.

I was thinking of a bunch of sanity checks such as:

for est in all_classifiers():
   p = make_pipeline(est()).fit(X, y)
   e = est().fit(X, y)
   assert_equal(e.score(X, y), p.score(X, y))
   assert_array_equal(e.predict(X), p.predict(X))

and the similar clustering and regressors and maybe others as long as they implement fit and score.

I thought about which functions we want to test, I guess that this issue only exists for fit and score so your tests should be fine (plus a if hasattr(est, "score"). I thought about whether we want to test decision_function, predict_proba and predict, too.

at least transform should also be checked not enough coffee apparently, transform never accepts y

Coverage increased (+0.01%) to 95.01% when pulling a105327 on amueller:pipeline_scoring_y_none into a168a6c on scikit-learn:master.

added integration test, rebased. And now I'm not convinced that this is the right fix any more :-/ Maybe not passing y if it is None would be the better fix? And removing the y=None everywhere? Edit: this is the right fix, just not enough coffee.

I am a bit confused.
I just did
`> git grep "transform(self, X, y=None)" | grep -v "fit_transform"``
which yields

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/base.py:    def inverse_transform(self, X, y=None):
sklearn/decomposition/dict_learning.py:    def transform(self, X, y=None):
sklearn/decomposition/pca.py:    def transform(self, X, y=None):
sklearn/decomposition/pca.py:    def inverse_transform(self, X, y=None):
sklearn/feature_extraction/dict_vectorizer.py:    def transform(self, 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):
sklearn/random_projection.py:    def transform(self, X, y=None):
sklearn/tests/test_pipeline.py:    def transform(self, X, y=None):

That y is never used anywhere and not required by any API, is it?

Anyhow, this PR is good to go, I think. I removed the requirement on transform (which is not a requirement at all) from the developer docs.

Coverage increased (+0.01%) to 95.01% when pulling cd63acc on amueller:pipeline_scoring_y_none into a168a6c on scikit-learn:master.

Great, merging this PR. Thanks again @amueller !

Thanks for the reviews @ogrisel and @jnothman :)

Edit: this is the right fix, just not enough coffee.

:)

Rest assured this fix is in accordance with what's been in the docs for some time...