fix: ruff rule F821 (undefined name) #54
Conversation
thatlittleboy
added
the
waiting for test suite
thatlittleboy
force-pushed
the
ruff-F821
branch
3 times, most recently
from
a0dc862 to
c28fe5b
Compare
Codecov Report
@@ Coverage Diff @@
## master #54 +/- ##
==========================================
+ Coverage 53.39% 53.58% +0.19%
==========================================
Files 90 90
Lines 13028 12982 -46
==========================================
+ Hits 6956 6957 +1
+ Misses 6072 6025 -47
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
thatlittleboy
removed
the
waiting for test suite
|
This is a major PR, great work! Would it be possible to break it into a few smaller PRs, to make it a bit easier to review? |
okay (although, the changes are already split by the commits, so it'll be largely equivalent to reviewing the commits one-by-one) EDIT: I split out the Explanation and violin changes to their own PRs. The rest are small enough to remain here, I think. @connortann |
And also temporarily remove the SHAP interaction beeswarm plot code (it's not runnable at the moment, because we ensured that the ndim of SHAP values was 2 above, so it can never be 3 to trigger the interaction beeswarm plot here). The removal of code here is mostly just to appease ruff. Will create a separate issue for this to re-introduce SHAP interaction values plotting again later on. Maybe it should be implemented as a separate function.
|
I'll address the codecov issues in a separate PR. Please disregard coverage and review/merge the rest of the changes first. Thanks! |
Related to Issue #21, and is a continuation of PR #25 when ruff was initially introduced to the project.
Dependencies: PR #53 (deprecating MimicExplainer) must be merged in first, because that's one of the main offenders of F821. ✅
Changes
The changes are quite major considering it involves mostly dead / unreachable code. Requesting a thorough review, but any review (even partial!) would be appreciated. I'll leave review comments in the relevant places later on to make code review a little easier for you guys, but here's the tl;dr:
Explanation.hstackmethod was using a non-existent variable "axis" (probably copied from the other helper methods likesum). The implementation as it is didn't make any sense, so I modified it to what I think made the most sense, which is to hstack the inner arrays and return a new Explanation object.beeswarmplot function was using non-existent variables liketitle,sort,plot_typewhich came from thesummary_legacy()function, within an unreachable code block meant for SHAP interaction values. I've commented out the block for now to prevent ruff from complaining, but we should look into re-implementing this functionality. -> Issue Reimplement SHAP interaction values in beeswarm #56.I revamped✅ DONE in PR fix: violin plots tests #87violinplot code quite a bit. The originalviolinwas essentially a copy-paste from thesummary_legacy()function, so there's a lot of redundant code (including a call to a "non-existent"summary()function) checking for "bar" or "dot" plot types. These have all been removed. Violin plots don't support multioutputs (only bar plots support this), so that section has been removed as well.I'll note that the code forviolinis still in the "legacy" style. For e.g., it still acceptsshap_valuesas a numpy array. Whereasbeeswarmonly takes inExplanationobjects, which I think we should align to that convention. -> Issue Refactor_violinplot code and add examples #55.The plot tests for violin in✅ DONE in PR fix: violin plots tests #87test_violin.pyandtest_summary.pywere revamped. In particular, the original test was just an empty plot...