Skip to content
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

Merged

Conversation

jklaise
Copy link
Member

@jklaise jklaise commented Aug 2, 2022

Whilst debugging #317 a few times I called the explain method without having called the fit method first which resulted in a cryptic stack trace:

Traceback (most recent call last):
  File "/opt/pycharm-professional/plugins/python/helpers/pydev/pydevd.py", line 1483, in _exec
    pydev_imports.execfile(file, globals, locals)  # execute the script
  File "/opt/pycharm-professional/plugins/python/helpers/pydev/_pydev_imps/_pydev_execfile.py", line 18, in execfile
    exec(compile(contents+"\n", file, 'exec'), glob, loc)
  File "/home/janis/PycharmProjects/public-fork-alibi/dev_notebooks/#317-minimal.py", line 37, in <module>
    explanation = explainer.explain(bad_instance)  # breaks here
  File "/home/janis/PycharmProjects/public-fork-alibi/alibi/explainers/anchors/anchor_tabular.py", line 815, in explain
    self.instance_label = self.samplers[0].instance_label
IndexError: list index out of range

This PR introduces a NotFittedError and a private _fitted attribute to AnchorTabular to determine when to raise this error resulting in a much nicer error message:

Traceback (most recent call last):
  File "/opt/pycharm-professional/plugins/python/helpers/pydev/pydevd.py", line 1483, in _exec
    pydev_imports.execfile(file, globals, locals)  # execute the script
  File "/opt/pycharm-professional/plugins/python/helpers/pydev/_pydev_imps/_pydev_execfile.py", line 18, in execfile
    exec(compile(contents+"\n", file, 'exec'), glob, loc)
  File "/home/janis/PycharmProjects/public-fork-alibi/dev_notebooks/#317-minimal.py", line 37, in <module>
    explanation = explainer.explain(bad_instance)  # breaks here
  File "/home/janis/PycharmProjects/public-fork-alibi/alibi/explainers/anchors/anchor_tabular.py", line 801, in explain
    raise NotFittedError(msg)
alibi.exceptions.NotFittedError: This AnchorTabular instance is not fitted yet. Call 'fit' with appropriate arguments first.

The reason for going with NotFittedError instead of AlibiNotFittedError is that the pre-fix Alibi makes for more verbose errors and should be unnecessary as all exceptions derive from AlibiException so that they can all be caught at once. My thinking is to open another PR to rename the 2 current exceptions with the Alibi prefix to have no prefix (but keep the old classes and inherit from them to prevent immediate breakage of existing user code).

@codecov
Copy link

codecov bot commented Aug 2, 2022

Codecov Report

Merging #732 (85f1da7) into master (d805829) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
alibi/exceptions.py 100.00% <100.00%> (ø)
alibi/explainers/anchors/anchor_tabular.py 90.28% <100.00%> (+0.11%) ⬆️
alibi/explainers/tests/test_anchor_tabular.py 74.34% <100.00%> (+1.12%) ⬆️

@jklaise jklaise changed the title Add NotFittedError and use it in AnchorTabular Add NotFittedError and use it in AnchorTabular Aug 2, 2022
Comment on lines +37 to +54
def __init__(self, object_name: str):
super().__init__(
f"This {object_name} instance is not fitted yet. Call 'fit' with appropriate arguments first."
)
Copy link
Member Author

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).

@mauicv
Copy link
Collaborator

mauicv commented Aug 5, 2022

This LGTM.

I guess one further idea would be to extend the behaviour to all explainers using the FitMixin interface. So something like:

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 _fitted check in the explain method on the base class as well. Possibly this would require further thought though and perhaps makes sense in a later PR.

@jklaise jklaise force-pushed the dev/not-fitted-error-anchor-tabular branch from 52dba4e to 85f1da7 Compare August 8, 2022 10:56
@jklaise jklaise merged commit 4c28d19 into SeldonIO:master Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants