Skip to content

Conversation

@damien2012eng
Copy link
Member

  • Add decision and waterfall plots to rsmexplain
  • add sample_ids as an option in the config file
  • add has_single_example flag to turn on/off plots

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (559bef4) 93.18% compared to head (cd038ef) 93.21%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #603      +/-   ##
==========================================
+ Coverage   93.18%   93.21%   +0.03%     
==========================================
  Files          32       32              
  Lines        4842     4851       +9     
==========================================
+ Hits         4512     4522      +10     
+ Misses        330      329       -1     
Impacted Files Coverage Δ
rsmtool/reporter.py 93.22% <ø> (ø)
rsmtool/utils/constants.py 100.00% <ø> (ø)
rsmtool/rsmexplain.py 92.03% <100.00%> (+0.89%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@damien2012eng damien2012eng force-pushed the add_more_plots_to_rsmexplain branch from dc18995 to ecccc33 Compare June 26, 2023 18:50
@damien2012eng damien2012eng force-pushed the add_more_plots_to_rsmexplain branch from ecccc33 to 8878bc9 Compare June 27, 2023 15:47
Copy link
Collaborator

@desilinguist desilinguist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@damien2012eng the changes look really good! Other than the suggestions I made, I also have the following comments:

  1. If you aren't displaying some plots based on has_single_example, then the description of that plot shouldn't be shown either or else it's confusing.
  2. I see the following at the bottom of the waterfall plot: ``<Figure size 640x480 with 0 Axes>`
  3. We should mention the additional plots for the single examples in the RSMExplain tutorial and also which plots are available when.

Copy link
Contributor

@tamarl08 tamarl08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! just had a couple of minor edits.

- Clearly show the plots that are unavailable instead of hiding them.
- Fix the weird matplotlib message being shown for waterfall plot.
@desilinguist desilinguist requested a review from tamarl08 June 28, 2023 20:33
@desilinguist
Copy link
Collaborator

@tamarl08 , @damien2012eng I have made a bunch of changes.

  1. I noticed that the tables with the Shap values could not be sorted because of the index label. We remove this label in rsmtool tables so I did the same here.
  2. For consistency, I upper-cased HAS_SINGLE_EXAMPLE for the variable passed as environment variable to the notebook. I also simplified the computation of has_single_example.
  3. I tweaked the plots notebook to:
    • clearly show the plots that are unavailable instead of hiding them.
    • fix the weird matplotlib message being shown for waterfall plot.
  4. I fixed the outdated tutorial documentation and added some useful links.

Can you both please review the changes and also test the single-example and multiple-example runs to make sure that everything looks okay in the notebook?

@damien2012eng can you also double check that when choosing the examples via sample_ids, the correct examples are being explained?

@damien2012eng
Copy link
Member Author

Hi @desilinguist I have run experiments with both single and multiple responses, and the reports look good to me. I also provided multiple ids, and then examined both the report and csv files, and confirmed that the correct responses had been explained.

Thanks for the improvement!

@damien2012eng damien2012eng merged commit a1f5a9d into main Jul 5, 2023
@delete-merged-branch delete-merged-branch bot deleted the add_more_plots_to_rsmexplain branch July 5, 2023 18:13
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.

4 participants