dsgibbons / shap

A game theoretic approach to explain the output of any machine learning model.
https://shap-community.readthedocs.io/en/latest/
MIT License
25 stars 5 forks source link

MimicExplainer does not work -- deprecate? #33

Closed thatlittleboy closed 1 year ago

thatlittleboy commented 1 year ago

The MimicExplainer class currently defined in shap/explainers/mimic.py has alot of undefined names (flagged out by ruff linting).

which will result in errors if the code is executed at any point during runtime.

What I find surprising is that, the original MimicExplainer code was introduced by slundberg 5 years ago (!) and has practically remained unchanged since then:

And yet, a quick search of issues on slundberg/shap on Github shows no evidence of anyone complaining / raising a bug report.


From which, I conclude that really no one is using this piece of code. Shall we delete mimic.py, so we have less to maintain?

ps. (Honestly, I have no idea how to fix even if we wanted to keep it. E.g. what is link that is being passed into the convert_to_link() function in the image above?)

ps. There is also no tests for MimicExplainer currently.

dsgibbons commented 1 year ago

Some links that may be relevant:

There seems to be some effort going on at Microsoft for supporting SHAP - just no idea why they don't touch the main repository...

thatlittleboy commented 1 year ago

Thanks for the links and context, it didn't occur to me to check external links.
I'll add on that the SHAP docs don't include MimicExplainer, and it isn't included in the top-level imports (__all__) which probably explains why it hasn't been brought up by the SHAP users as a problem thus far.

Considering that there's still some buzz around this at Microsoft, I see two options

  1. Deprecate and delete the MimicExplainer code here, and only add it back in when we are ready to make the API public (most likely only when we have upstream support by Microsoft). I don't think we need a deprecation cycle because MimicExplainer isn't even in the public API. And direct future queries about MimicExplainer support in shap to this issue, and we'll discuss from there.
  2. Keep the MimicExplainer code around. Add a file exclusion of this file for ruff, black and probably any other linters we might add in the future. Because as it stands, the python file is not even executable (undefined names), so any tools that rely on executing the file will necessarily fail.

I'm personally more for 1 (don't keep code that is not being used; else it is just a burden to maintain). Wdyt @dsgibbons @connortann ?

dsgibbons commented 1 year ago

I'm happy with 1. so long as we keep an open issue for implementing MimicExplainer.