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

Documentation for explainable for model with continuous variables #565

Open
wants to merge 98 commits into
base: master
Choose a base branch
from

Conversation

PoorvaGarg
Copy link
Contributor

@PoorvaGarg PoorvaGarg commented Aug 23, 2024

This pull request adds a tutorial for the module explainable in the context of a model with continuous variables and dynamical systems.

@PoorvaGarg PoorvaGarg marked this pull request as ready for review August 30, 2024 18:47
@PoorvaGarg PoorvaGarg marked this pull request as draft August 30, 2024 18:47
@PoorvaGarg PoorvaGarg marked this pull request as ready for review August 30, 2024 19:17
@PoorvaGarg PoorvaGarg added status:awaiting review Awaiting response from reviewer and removed status:WIP Work-in-progress not yet ready for review labels Aug 30, 2024
@SamWitty
Copy link
Collaborator

SamWitty commented Nov 4, 2024

Thanks for taking this on @rfl-urbaniak and @PoorvaGarg ! It's great progress towards a very interesting combination of previously very disconnected ideas. I have a fair number of comments because it's pretty long and detailed, but overall I really like where this is going. I've made a small number of editorial changes to the notebook itself, which can be found in #573 . The more substantive requests for changes are better left to discussion and then collaborative revision. With that, here are my comments:

Main Feedback:

  1. The notebook appears to be missing a clear and obvious explanatory question to motivate the analysis, written in plain English at first and then translated into specific operations later when mathematical terms are introduced. My suggestion would be to expand the text after “only one of them is responsible, and we are interested in being able to identify which one”, and move this earlier and more prominently in the tutorial.
  2. The notebook is missing a clear and obvious conclusion we should draw from this analysis. This conclusion should be emphasized strongly visually (e.g. text in bold or a small subsection), and point to specific visual evidence and concrete numeric quantities. Once the conclusion is made clear, it would be nice to talk a bit about the policy or scientific implications. In the conclusion the notebook states the following: “It is evident from the plot above that the counterfactual for lockdown has more probability mass in the top right quadrant (low overshoot in the necessity world and high overshoot in the sufficient world). This gives us a clearer picture into why lockdown has higher causal role in the overshoot being too high as compared to masking.” It does not give me a clear picture into why lockdown has higher causal role.
  3. The notebook doesn’t discuss assumptions, possible violations of those assumptions, and their implications. This is particularly important because the model implicitly assumes that the structural functions (in this case, the SIR model) are known except for the specific parameters. As explanations in this notebook are derived from counterfactuals requiring strong assumptions like those found in this tutorial, there are many models where this kind of analysis would not be appropriate, or would otherwise need to modified to account for more uncertainty in the structural functions.

Detailed Feedback:

“Now we incorporate the Bayesian SIR model into a larger model that includes the effect of two different policies, lockdown and masking, where each can be implemented with $50%$ probability (these probabilities won't really matter, as we will be intervening on these, the sampling is mainly used to register the parameters with Pyro).” Why do we need pyro.sample sites rather pyro.deterministic sites here?

Why do we need the MaskedStaticIntervention?

I separated the policy_model into policy_model and overshoot_query, which is emphasizing that there’s some repeated code that could be refactored out further. Specifically, the get_overshoot function in the introduction could be vectorized when introduced, and then reused later in the actual definition of the model.

“Trajectories and overshoot distribution in the but-for analysis” requests:

  1. Add axis labels to all plots
  2. Make it clear that the rows correspond to the same intervention configuration. Currently it’s implied by them being next to each other, but the intervention configuration label is only on the leftmost plot.
  3. Fix the range of the histograms to be shared across all. Currently, it’s hard to visually compare.
  4. Show the mean as a vline on each histogram, and show the overshoot threshold as a vline on each histogram. Once this is done, you’ll need a single histogram legend.

importance_infer: This is important enough that I think it justifies a little more explanation. As is, an unfamiliar audience may be left wondering a few things:

  1. Is importance sampling essential to the solution of this notebook, or is it just one of many plausible inference strategies from Pyro one could use?
  2. What do the return values represent?
  3. How do I get away with sampling importance weights a single time and then using them to answer many different questions downstream? (I believe the answer here is because we’ve broadcast the entire importance sampling procedure over all counterfactual worlds.)

Why do we need alternatives? Shouldn’t this always be equal to supports setminus antecedents?

Can we add some citations justifying “degree of responsibility”?

“The reader might have the impression that the numbers are relatively low: …” Could we suggest a more intuitive description of what these probabilities are? If they shouldn’t be interpreted as any conceptually meaningful probabilities, then we should emphasize that explicitly or consider removing.

“Counterfactual - necessity world” figure (and all following similar plots) requests:

  1. The overlapping histogram plots is a bit difficult to interpret. Something about the way colors blend together is hard to read. Could you change this to multiple subplots with shared axes or change the color overlap somehow?

“Filter for the relevant context” -> Something about conditioning on the context nodes. I don’t like the word “filter” here.

“Comparing how necessity interventions for the two antecedents affect the overshoot” I don’t know what this means. I think the terminology of “necessity world” and “sufficiency world” is a little confusing. Is there a way we could reuse the mathematical notation introduced earlier to denote which counterfactual world we’re considering at any given point in the narrative?

Heatmap suggestions/questions:

  1. We need a colorbar showing how the colors correspond to probability density.
  2. How should I interpret the vertical and horizontal lines? I think I get that “mean overshoot” is the point at the intersection, but I don’t know how to interpret “overshoot too high”. Is this saying that any configuration above and to the right of that intersection is too high?

@SamWitty SamWitty removed the request for review from eb8680 November 4, 2024 22:08
Copy link
Collaborator

@SamWitty SamWitty left a comment

Choose a reason for hiding this comment

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

See above comment for requested changes.

@SamWitty SamWitty added status:awaiting response Awaiting response from creator and removed status:awaiting review Awaiting response from reviewer labels Nov 4, 2024
* edit intro

* progress

* remove plate and more edits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation module:explainable status:awaiting response Awaiting response from creator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants