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

add method to compute average treatment effects #73

Merged
merged 18 commits into from
Aug 7, 2024
Merged

Conversation

apoorvalal
Copy link
Contributor

@apoorvalal apoorvalal commented Aug 1, 2024

Thanks for the nice work on this package; it addresses a lot of pain-points I have with econML. An ATE estimator method was missing (possibly because that might not be the most obvious intended use case for the package, but the rest of the API / package ergonomics are nice enough that I decided to use it for that as well), so I added it (only to drlearner, since the variance-of-pseudooutcome construction for standard error is justified in that case but not for T-learner etc).

I've also added a notebook in the docs/examples directory that verifies that estimates line up with what one gets from econML and doubleML on a simulated dataset.

Checklist

  • Added a CHANGELOG.rst entry

@apoorvalal apoorvalal requested a review from kklein as a code owner August 1, 2024 23:24
@apoorvalal
Copy link
Contributor Author

apoorvalal commented Aug 1, 2024

build failures likely due to use of external packages in the new notebook. Should I add them as dev dependencies (or I can install them inside the notebook; unsure if that conforms to your style guide).

Edit: done

@kklein
Copy link
Collaborator

kklein commented Aug 2, 2024

@apoorvalal Thanks a lot for your kind words and effort! :)

I'll get to your PR asap - hopefully on the weekend!

Copy link
Collaborator

@kklein kklein left a comment

Choose a reason for hiding this comment

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

Looking great - just some minor comments. :)

metalearners/drlearner.py Outdated Show resolved Hide resolved
docs/examples/example_estimating_ates.ipynb Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
metalearners/drlearner.py Show resolved Hide resolved
metalearners/drlearner.py Outdated Show resolved Hide resolved
@kklein
Copy link
Collaborator

kklein commented Aug 3, 2024

A simple pixi update should suffice to get rid of the merge conflict. :)

@apoorvalal
Copy link
Contributor Author

addressed most of your (great) suggestions in this commit. I think the conflicts are likely straightforward to resolve (amended a the drlearner test file, added the feature desc to changelog, and the usual pixi lock conflict

@apoorvalal
Copy link
Contributor Author

apoorvalal commented Aug 6, 2024

Ready to merge pending (opaque) pixi trouble (edit: and mypy decided to get mad at a different commit]
I added you as a collaborator on my PR so you can directly commit to my ate branch rather than to have to send me PRs.

Copy link
Collaborator

@kklein kklein left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot for your contribution @apoorvalal ! :)

I'll create a new release for version 0.9.0 soon.

@kklein kklein merged commit 7ddf544 into Quantco:main Aug 7, 2024
11 of 12 checks passed
@kklein kklein deleted the ate branch August 7, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants