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

Instrumental example notebook. #555

Merged
merged 3 commits into from
Aug 5, 2024
Merged

Instrumental example notebook. #555

merged 3 commits into from
Aug 5, 2024

Conversation

dimkab
Copy link
Contributor

@dimkab dimkab commented Aug 2, 2024

The notebook probably needs a bit more polishing, but I would love comments at this stage.

@dimkab dimkab requested a review from eb8680 August 2, 2024 22:12
Copy link
Contributor

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

Looks great! My main suggestion is to include lines for both the true ATE and the biased naive estimate in the two ATE plots and to make sure both plots have the same scale on the X axis. That way it's clearer that the mean of your estimate is much closer to the true value than the naive estimate.

Other comments:

  • Consider adding plots showing posterior predictive fits, e.g. pairplots comparing the true and posterior predictive marginal distributions for each observed variable (X, Y and Z).
  • Consider adding section headers corresponding more closely to other notebooks, such as those in the mediation analysis and backdoor adjustment examples. The more structurally similar this example is to the others and to the systematic workflow in the final section of the tutorial, the easier it will be to understand for someone who's already read those.
  • Make sure to build (cd docs && make html) and inspect (python -m http.server -d docs/build/html/ <port> and open https://localhost:<port> in a browser) the docs locally to verify that things render correctly. It looks to me like the line wrapping of equation 1 might be messed up.

@eb8680 eb8680 linked an issue Aug 3, 2024 that may be closed by this pull request
@eb8680
Copy link
Contributor

eb8680 commented Aug 3, 2024

Oh, and also please add this to the Examples section in docs/source/index.rst and the "Learn more" section of README.rst, otherwise it won't actually be included in the docs. You should also add it to the notebook CI stage (#410) either as part of this PR or in a followup PR.

@dimkab
Copy link
Contributor Author

dimkab commented Aug 5, 2024

Thanks! I've followed all your suggestions, please see revised version.

@dimkab dimkab requested a review from eb8680 August 5, 2024 15:32
Copy link
Contributor

@eb8680 eb8680 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 to me!

@eb8680 eb8680 merged commit b4b96ec into master Aug 5, 2024
7 checks passed
@eb8680 eb8680 deleted the db-instrumental-example branch August 5, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add instrumental variables example
2 participants