raise AttributeError if predict_proba is not available #98
Conversation
kmike
force-pushed
the
predict_proba_exception
branch
from
36ba92a
to
daa0569
Compare
|
Python 2.7 and Python 3.5 tests are failing on Travis, but failures don't look related to the PR; Python 3.4 passes on Travis. |
|
I've relaunched Travis, lets see what happens |
|
weird, it now fails with the same error as https://travis-ci.org/scikit-learn-contrib/lightning/jobs/171807463, but in a different environment |
|
OK I fixed a bug that I introduced in my recent pull request, lets see if now tests pass |
|
all tests pass now :-) |
|
This looks good to me. I'll give @mblondel some time to give his opinion. Thanks for your contribution! |
| @@ -52,7 +59,7 @@ def predict_proba(self, X): | |||
| prob += 1 | |||
| prob /= 2 | |||
| else: | |||
| raise NotImplementedError("predict_(log_)proba only supported when" | |||
| raise NotImplementedError("predict_proba only supported when" | |||
There was a problem hiding this comment.
I've kept NotImplementedError here as a defensive measure, but probably it is better to remove it.
There was a problem hiding this comment.
Why do you think it would be better to remove it?
There was a problem hiding this comment.
Because code flow should never get to this exception - AttributeError is raised earlier. If for some reason _predict_proba is used instead of predict_proba then with unsupported loss function an exception is still raised (prob variable is not defined); it is harder to interpret that NotImplementedError though. So maybe keeping it is fine :)
|
LGTM. Could you also take a look at #18? |
|
2 LGTM = merge :-) |
|
Thanks! @mblondel sorry; I may take a look at it at some point in future, but not right now. |
|
Sure, no worries. I was just trying to motivate someone to fix a long-standing issue :b |
|
@mblondel : are you OK if I make a quick release ASAP? |
Try to look inside estimators that wrap other estimators: predict_proba can be a property in wrapped estimator but a method in wrapper estimator, so the AttributeError will be raised in runtime. But we don't want to catch it in runtime because it might be unrelated. This fixes a failure that appeared after scikit-learn-contrib/lightning#98 merge (see https://travis-ci.org/TeamHG-Memex/eli5/jobs/172586529)
|
@fabianp Sure, thanks for doing this! |
In scikit-learn when predit_proba method is not available, AttributeError is raised instead of NotImplementedError. In this PR: