if we are to use the factory directly in the decorating lines, I would rather change its name, for instance, have:

@make_delegation_decorator('_final_estimator')
def predict(self, X):
    ...

replaced by:

@if_delegate_has_method(delegate='_final_estimator')
def predict(self, X):
    ...

WDYT?

I am not 100% decided about this either.

@amueller can you rebase on master and fix the conflicts to get travis to run?

rebased.
I like your naming suggestion.

Thanks for working on this. I like @ogrisel's naming suggestion, and I think @amueller is right that naming the decorators was an unnecessary indirection.

However, I probably should have tested the following: #2854 will transfer the docstring from the function defining Pipeline.transform to the descriptor that takes its place. That currently doesn't happen here, as shown by:

from sklearn.pipeline import Pipeline
help(Pipeline)

(comparing both PRs)

We could write a test along the lines of:

def foo():
   """str"""
   pass

assert foo.__doc__ == my_wrapper(foo).__doc__

In either case the function signature isn't shown which is unfortunate, and may need to be compensated for by writing out the signature within the docstring.

A related problem, that I didn't test for in #2854, and which might not be so easy to fix, is that none of those methods render in the sphinx documentation :(((

How about delegate_to?

Hum, that is odd. I checked the docstring on the decision function and it was transferred.
I'll add a test. The documentation is an interesting problem. I'll have to think about it. Do you know how that works for properties usually?

@larsmans It doesn't delegate to that object necessarily. For the gridsearch, the parameter is estimator but it is delegated to best_estimator_.

@jnothman
So I don't entirely understand what happens with the docstrings.

from sklearn.pipeline import Pipeline
from sklearn.svm import SVC
help(Pipeline([('svm', SVC)]).decision_function)

gives the right docstring, though help(Pipeline) does not.
Can you explain?
I'm a bit slow today, I think I got it now.

What I still don't understand: what is it that help(Pipeline) displays. It is not __doc__.
I just fixed my PR, but that didn't change Pipeline.__doc__ as that does not include the function definitions. I have no idea how to test this, as I don't know how to access this docstring.

it seems you can use Pipeline.__dict__['decision_function'].__doc__.... alright then

Fixed the docstrings in sphinx.
You can now do
Pipeline.decision_function.__doc__.

The somewhat odd fix with checking if the object is None would also be necessary for the @property based solution FYI, so that needs to be:

    @property
    def predict_proba(self):
            if self is not None:
                self.final_estimator.predict_proba
            return self._predict_proba

On the other hand, I think the @property based solution might not break the github source links.

@GaelVaroquaux It would be great if you had time to have a look. The actual implementation is really short and not that meta any more. The only downside that I can currently see is that introspection doesn't find the right line numbers for the function any more.

help(Pipeline([('svm', SVC)]).decision_function)
gives the right docstring, though help(Pipeline) does not.
Can you explain?

Sure. In the first case you retrieved the attribute (i.e. executed the __get__ of the delegation descriptor) and read it return value's __doc__. In the second case you viewed the __doc__ of the descriptor instance.

Perhaps in the future we will hack sphinx to show the correct signature and source link.

In the meantime I think this is a moderately serious bug that results in silent misbehaviour, and we should merge it ASAP. Although there are concerns about code quality, as expressed by @GaelVaroquaux, the perfect solution can wait, as the bug fix cannot.

I give @amueller's work a +1, and he has implicitly given my contributions to this a +1... @larsmans has given an earlier version of this a +1, so as far as I'm concerned this has +2. If an external party would like to give this another look over, that would be great, but otherwise we should aim to merge.

I think I like this solution as well.

I think we should merge. As @jnothman points out, this is a fix to a long-standing bug, and does not break or introduce any public changes. Changing the implementation if we are unhappy with it should therefore be easy.

and does not break or introduce any public changes.

except sphinx ;)

Thanks a lot for all the work you put into this @jnothman, and thanks for the reviews :)
Some of the other branches @jnothman had still have some more docstrings I think. I'll try to get them after the holidays.