-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add propensity weighting schemes and covariate balance plot functionality #311
Conversation
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 77.27% 80.09% +2.82%
==========================================
Files 21 21
Lines 1395 1648 +253
==========================================
+ Hits 1078 1320 +242
- Misses 317 328 +11 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Sory, not had time to look at this yet I'm afraid - a combination of work + illness. I might also not get time to look next week because of a deadline on a client project. Looking forward to when I can dive into this 👍 Because this will be a new feature addition, and probably trigger a minor version bump (semantic versioning), I'll ask for at least one other review. |
Added some explanation. View entire conversation on ReviewNB |
Again, have generally a more explanatory approach this time. View entire conversation on ReviewNB |
Added a note that these are the propensities we will seek to use. View entire conversation on ReviewNB |
Added more explict flagging. View entire conversation on ReviewNB |
Added full stop. View entire conversation on ReviewNB |
Added markdown separator. View entire conversation on ReviewNB |
Added bibtex View entire conversation on ReviewNB |
nice! View entire conversation on ReviewNB |
There are a number of paragraphs missing full stops at the very end |
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.
Can you run make uml
to update the UML diagrams?
Can we add a docstring to plot_balance_ecdf
Maybe add an assert for perc
is 0-1 in the InversePropensityWeighting.weighted_percentile
method
Excellent updates. The notebook is really nice at this point.
@@ -69,6 +76,14 @@ @article{acemoglu2001colonial | |||
year={2001} | |||
} | |||
|
|||
@incollection{forde2024nonparam, |
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.
missing "}" causing the docs to fail to build
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.
fixed this
p = pm.Deterministic("p", pm.math.invlogit(mu)) | ||
pm.Bernoulli("t_pred", p=p, observed=t_data, dims="obs_ind") | ||
|
||
def fit(self, X, t, coords): |
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.
So PropensityScore
will inherit a fit
method from ModelBuilder
. So I think we can delete this whole method. If not, can you add a note into the docstring to explain why ModelBuilder.fit()
is being overridden?
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.
Added a note here. I overwrite the method just because the base method assumes a y input, and i wanted to keep the t to emphasise we're modelling the treatment not the outcome variable...
Can you also please update from |
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
So I was getting all tests passing locally, but 12 failing remotely. But after rebuilding the environment I replicated the 12 failing tests. We've got an issue for it #323 |
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.
So I think we are very close to being able to merge this. Just a few things:
- Once Fix failing doctests #330 is merged, can you update from
main
and just double check all tests pass. - And I just realised, because this introduces nice new functionality, can you append to
README.md
(for the GitHub repo)and.index.rst
(for readthedocs)
Will merge from main this evening |
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
2 failing tests. These are the new ones that you've just introduced @NathanielF. This is fixable by adding |
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
Signed-off-by: Nathaniel <NathanielF@users.noreply.github.com>
In relation to this issue: #303
I'm opening the PR which includes functionality for fitting a propensity score model and analysing the experimental outcomes under different re-weighting schemes.
I've added the relevant classes to and models. I've also demonstrated their use an example notebook with a parameter recovery exercise and an application to real data.
I've added two plotting functions to experiment class to both analyse covariate balance and plot the overlap of the propensity scores and the uncertainty in the estimation of causal effects.