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

CATE validation suite #777

Merged
merged 19 commits into from
Nov 8, 2023
Merged

CATE validation suite #777

merged 19 commits into from
Nov 8, 2023

Conversation

amarvenu
Copy link
Contributor

@amarvenu amarvenu commented Jun 5, 2023

Add new CATE validation class, DRTester, to perform doubly-robust based CATE validation methods. Implemented methods include BLP, calibration, and QINI coefficient. Relevant files are:

  • econml/validation/drtester.py (main class)
  • econml/tests/test_drtester.py (tests)
  • notebooks/CATE validation.ipynb (example usage notebook)

Tests cover 100% of implemented code (see attached image).

drtester_coverage

Looking forward to any comments.

Copy link
Collaborator

@vsyrgkanis vsyrgkanis left a comment

Choose a reason for hiding this comment

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

Great job! Very well documented and clean code. Several comments and requests, but overall seams in a great shape!

econml/validate/drtester.py Outdated Show resolved Hide resolved
econml/validate/drtester.py Show resolved Hide resolved
econml/validate/drtester.py Outdated Show resolved Hide resolved
econml/validate/drtester.py Outdated Show resolved Hide resolved
econml/validate/drtester.py Outdated Show resolved Hide resolved
econml/validate/drtester.py Outdated Show resolved Hide resolved
econml/validate/drtester.py Outdated Show resolved Hide resolved
econml/validate/drtester.py Outdated Show resolved Hide resolved
econml/validate/drtester.py Show resolved Hide resolved
econml/validate/drtester.py Outdated Show resolved Hide resolved
@amarvenu
Copy link
Contributor Author

amarvenu commented Aug 1, 2023

Updated code to address the given comments. Branch underlying PR has been updated. Modified test cases still have 100% code coverage:
cate_coverage_080123

Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Signed-off-by: Keith Battocchi <kebatt@microsoft.com>
Copy link
Collaborator

@kbattocchi kbattocchi left a comment

Choose a reason for hiding this comment

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

Sorry for the terrible delay in approving this; it will be a great addition to the library and it looks good to me, I've just made a few small changes to clean up some linting issues and such.

@kbattocchi kbattocchi merged commit a10e42d into py-why:main Nov 8, 2023
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.

4 participants