-
Notifications
You must be signed in to change notification settings - Fork 10
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
Hierarchical Causal Models #236
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #236 +/- ##
==========================================
+ Coverage 80.87% 81.27% +0.39%
==========================================
Files 50 51 +1
Lines 4135 4314 +179
Branches 845 981 +136
==========================================
+ Hits 3344 3506 +162
- Misses 668 670 +2
- Partials 123 138 +15 ☔ View full report in Codecov by Sentry. |
hi @adamrupe - can you add a checklist into the PR description with the tasks to complete for this PR before it needs review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- tox.ini: Language not supported
3237de1
to
ef54579
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a major refactor to address the software issues from the last round. The next steps for @adamrupe and @djinnome are:
- Read through the code and familiarize yourselves with the new interface
- Comment on / address all TODO's I left in the code (there aren't many)
- Tests
- Either implement tests for conversion to HSCM or delete the conversion code
- Test
augment_collapsed_model
- Check the notebook, which used to raise some exceptions, but I replaced those with the high-level
identify_outcomes
API. Please review to make sure that the places where there is no estimand produced because the graph has a single c-component are all correct - Create a high-level, real world example that demonstrates using all of the code in a story-driven workflow (i.e., do not explain the math, only explain which functions you implemented solve the problem). Use https://github.com/y0-causal-inference/y0/blob/main/notebooks/Counterfactual%20Transportability.ipynb as a golden standard for how a great notebook with applications looks
Along the way, please make sure that you check the CI/CD system for automated, objective feedback on code quality. @adamrupe if you're not familiar with how to do this, I am happy to show you
@cthoyt What's your recommendation for handling merge conflicts with jupyter notebooks? I need to do this before I can pull your updates. I'm also not familiar with the CI/CD system, so if you could talk me through it that would be great. |
@adamrupe before merging, copy your local notebook to your desktop. While merging, throw away everything from your repository's copy and overwrite it with remote. Then, you can think about manually inspecting your notebook on your desktop, and the new version from the remote repo side-by-side. The best way to avoid this kind of thing is never to leave changes unpushed when you finish working, and to always pull before you start working again The short explanation of how to use the CI/CD system is: you can always scroll to the bottom of this pull request (#236) and look at the feedback given by GitHub running our unit tests, linting, and code quality checks. This is what it looks like to me right now: ![]() You can click on any of the rows with the red x's, and then it will bring you to the page that ran the tests for you. Right now, you will be able to see all of the output from running Similarly, while you're still getting used to having code quality checks, you will probably see that the linting or type checking scripts also give errors, which you can view in the same way.. It's sort of the expectation in a team setting for coding that you make pushes often, and each time check out what kind of feedback CI gives. This will help you iteratively make your code better, with fully objective feedback that you don't have to wait on someone else to give you. Alternatively to CI/CD in GitHub, you can run There's documentation in the README on how to use all of the nice development tools built into this repo at https://github.com/y0-causal-inference/y0?tab=readme-ov-file#%EF%B8%8F-for-developers If you get caught up on any parts of this that aren't self-explanatory, I'm happy to plan a video chat tomorrow, or sometime next before 6PM germany time |
Awesome, thanks @cthoyt! That makes sense, and Jeremy and Richard have already shown me how to use |
@cthoyt I've refactored HSCMs and filled in the HCM.to_hscm() tests, so all tests are now passing. However, there is a depreciation warning I'm getting from another part of the codebase:
There is also a mypy error from
Since I didn't write this code, I didn't want to make edits to fix these, but they seem straightforward fixes. I'll add a test for |
@adamrupe you should be unblocked on the CI/CD pipeline now. looking forward to seeing a nice case study notebook, then we can finish this PR! |
Closes #278
This PR implements Hierarchical Causal Models (Weinstein and Blei, 2024)
This PR will be ready for review when the following algorithms have been tested and implemented.
augmentation variable to a collapsed HCGM, following Definition 6.
marginalizes out parent(s) of an augmentation variable (Section 5.2).