I was using it on random forests and decision trees as well, which do not have a decision_function.

As far as I know, the decision function is -1 for confident -1 class, +1 for confident +1 class, but it's not necessarily in the [-1, 1] range, and it represents the distance from the hyperplanes. For the random forest probabilities, would decision function be equal to p(x) * 2 - 1, or would calculating a distance from the decision boundaries of the random forest make sense? (i.e. for each tree node, how far away were we from the condition in the node).

If it's documented that it's not necessarily equivalent to the SVM's decision_function, then it might be OK to define decision_function like you suggest.

I changed some things so that decision_function retuns predict_proba * 2 - 1, if decision_function is not defined. I added a test for this case.

• I noticed that decision_function returns an array of shape (n_samples,) instead of (n_samples,1) if there is a single class. I don't know if this is intended functionality, but it's not in the doctest, so I removed it, I can put it back.
• I changed if_delegate_has_method to include a fallback method, so it can check for either predict_proba or decision_function, but I'm not sure if this is useful for other cases as well.

This pull request introduces 37 alerts and fixes 18 - view on lgtm.com

new alerts:

• 37 for Explicit export is not defined

fixed alerts:

• 6 for Mismatch between signature and use of an overridden method
• 3 for Missing call to init during object initialization
• 3 for Potentially uninitialized local variable
• 2 for Comparison using is when operands support eq
• 2 for Wrong number of arguments for format
• 1 for Conflicting attributes in base classes
• 1 for Mismatch between signature and use of an overriding method

Comment posted by lgtm.com

This pull request introduces 37 alerts and fixes 18 - view on lgtm.com

new alerts:

• 37 for Explicit export is not defined

fixed alerts:

• 6 for Mismatch between signature and use of an overridden method
• 3 for Missing call to init during object initialization
• 3 for Potentially uninitialized local variable
• 2 for Comparison using is when operands support eq
• 2 for Wrong number of arguments for format
• 1 for Conflicting attributes in base classes
• 1 for Mismatch between signature and use of an overriding method

Comment posted by lgtm.com

@jnothman any comments on the current state?

@jnothman , does it look better now?

@jnothman , I'm open to making a new decorator, but couldn't think of one that is useful in multiple places because I'm not very familiar with the entire codebase. Do you have a specific design in mind?

The remainder of the code is fixed.

@jnothman , I added a second decorator to utils/metaestimators.py. I can re-name it because it sounds very close to the original, but I think the name is explanatory enough.

@jnothman
For the first point, I agree that plural makes it seem like all names are needed, so all names are actually needed. The invocation of the new descriptor is like this:

@if_delegate_has_methods(['_first_estimator', 'estimator'],
                         ['decision_function', 'predict_proba'])
    def decision_function(self, X):

As far as I could understand, property is for defining class variables that have getters and setters. Which of the variables would be a property?

Properties are descriptors which will result in hasattr(..., ...) to return False if the property getter raises an attribute error. The descriptor is more-or-less equivalent to:

def decision_function(self):
    if not hasattr(self.estimator, 'decision_function') and not hasattr...:
        raise AttributeError
    def _func(X, y):
        # calculate the decision function
        return df
    return _func

@jnothman , I'm not sure if I understood correctly, so I made a sample. https://pastebin.com/6Y9XUwg0 is this a better solution?

@jnothman I put it back together as a single decorator

Please add an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

@jnothman I added it to what's new. Are pull requests are also referenced as :issue:`number`?

@jnothman , it's all set then.

I'm not entirely sure what the motivation for this is.
If the goal is to avoid having to duck-type predict_proba and decision_function, why don't we define decision_function in BaseEstimator?

Which base_estimator?

If you mean the BaseEstimator in base.py, I'm not sure if this fallback to using 2 * predict_proba - 1 in cases where decision_function is not available is useful for all cases.

I think understanding the difference between the probabilities [0, 0.1, 0.05, 0] and [0, 1, 0.5, 0] for each of the classes in a OneVsRest case is useful, but I'm not sure if this is the best way of going about implementing this. I was initially only thinking a normalize=True/False kwarg for predict_proba.

Why do you think it would be useful here and not in other cases? predict_proba has a more limited definition than decision_function (though some people might expect some sigmoid semantics).

Adding kwargs to functions is kind of frowned upon as it's hard to access those in compound estimators like pipelines. And anything that changes the predict_proba contract to not be normalized also seems a bit weird to me.

Your goal is basically to implement a rejection threshold, right? So the only reason for this PR is to not do ducktyping in your implementation of the rejection threshold?

I think we'll add thresholding methods soon (hopefully) and I'm pretty sure they will contain the ducktyping. I'm not sure why putting this logic in this particular place would be the right solution.

I'm not sure what other cases it would be useful in, but if you think it would, I can add that to BaseEstimator.

Yes, I was trying to implement a rejection threshold for OneVsRestClassifier, so I had to have another class that inherited OneVsRestClassifier and re-implemented predict_proba, which required knowledge of the internal workings of it.

I can add an option to normalize or not to the constructor of OneVsRestClassifier, which would also be accessible from a pipeline. Or, there could be another function that gives some per-class confidence.

My question was more why you didn't use decision_function. But I guess you take the probabilities at face value and so the actual values matter? Usually I'd ensure calibration or some particular trade-off on the validation set, so it doesn't matter whether you're using predict_proba or decision_function.

I was comparing multiple algorithms, and decision_function wasn't implemented for all of them. Now I understood what you meant by duck typing for rejection threshold implementation. I guess I could have done it outside of OneVsRestClassifier.

For the values, I was calibrating within the training set.

Anyways this was used for a publication which is already published, so feel free to close this PR without merging, because now it seems not very necessary for other people, or for me. Especially if there's going to be a rejection threshold implementation soon.

I would likewise find an optional normalize=False flag useful.

In my case I would like to plot probability thresholds vs. recall / precision scores for the respective classes, however, since these are in a normalised form I need to train independent classifiers (i.e. not use OneVsRestClassifier) to get the necessary probabilities.

It would also be interesting from the perspective of considering alternate probability thresholds.

Thanks.