-
Notifications
You must be signed in to change notification settings - Fork 252
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
Add NotFittedError
and use it in AnchorTabular
#732
Add NotFittedError
and use it in AnchorTabular
#732
Conversation
Codecov Report
@@ Coverage Diff @@
## master #732 +/- ##
==========================================
+ Coverage 81.12% 81.15% +0.02%
==========================================
Files 105 105
Lines 11832 11847 +15
==========================================
+ Hits 9599 9614 +15
Misses 2233 2233
|
NotFittedError
and use it in AnchorTabular
def __init__(self, object_name: str): | ||
super().__init__( | ||
f"This {object_name} instance is not fitted yet. Call 'fit' with appropriate arguments first." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided to put the error message here and allow it to be parametrized with the object name. The message is generic enough to apply to any object that as a compulsory fit
method so I don't think it's necessary to define messages by hand in the code (which will inevitably become different from each other).
This LGTM. I guess one further idea would be to extend the behaviour to all explainers using the class FitMixin(abc.ABC):
def __init__(self):
self._fitted=False
def fit(self, X: Any) -> "Explainer":
self._fitted=True
self._fit(X)
@abc.abstractmethod
def _fit(self, X: Any) -> "Explainer":
pass Can go further and run the |
52dba4e
to
85f1da7
Compare
Whilst debugging #317 a few times I called the
explain
method without having called thefit
method first which resulted in a cryptic stack trace:This PR introduces a
NotFittedError
and a private_fitted
attribute toAnchorTabular
to determine when to raise this error resulting in a much nicer error message:The reason for going with
NotFittedError
instead ofAlibiNotFittedError
is that the pre-fixAlibi
makes for more verbose errors and should be unnecessary as all exceptions derive fromAlibiException
so that they can all be caught at once. My thinking is to open another PR to rename the 2 current exceptions with theAlibi
prefix to have no prefix (but keep the old classes and inherit from them to prevent immediate breakage of existing user code).