Open PoorvaGarg opened 3 months ago
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:
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:
range
of the histograms to be shared across all. Currently, it’s hard to visually compare.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:
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:
“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:
This pull request adds a tutorial for the module
explainable
in the context of a model with continuous variables and dynamical systems.