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

Fix the test_accuracy function by modifying the assertion logic #867

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

kgao
Copy link
Collaborator

@kgao kgao commented Mar 29, 2024

  • The original code is testing the accuracy of different estimators by checking if the true Average Treatment Effect (ATE) falls within the calculated confidence interval. However, the check is done only once, using a single-point estimate (ate), which may not be sufficient to validate the estimator's performance. So it failed when the proportion of a true ATE within the confidence interval is NOT greater than 0.5 (50%).

  • The new logic: To check that 50% of the values are in the 90% confidence interval (which makes sense), but it's testing this with the ate, which returns a single value, so actually the threshold isn't important, it's a single point that is either in the interval or not. Instead, what we should do is generate W, D, and Y several times and check that most of the time the ate is in the bounds (like, generate 10 sets of W, D, Y and check that at least 8 of those times the true ate was inside the interval.

  • Also applied the logic for test_accuracy_iv (And reduced the sample size n=1000 to improve the test time)

@fverac
Copy link
Collaborator

fverac commented Mar 29, 2024

Nice fix!

Wonder if it makes sense to apply the same logic to the test_accuracy_iv method in test_discrete_outcome.py, since the logic there is also doing this weird proportion_in_interval stuff that doesn't really make much sense with ate (but might make more sense the new way you are doing it)

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.

This is a good start, but the logic is not quite right.

In addition, please also address @fverac's comment about making parallel changes to the corresponding IV test.

@kbattocchi kbattocchi enabled auto-merge (squash) April 10, 2024 14:38
@kbattocchi kbattocchi merged commit 52efc8e into main Apr 10, 2024
78 checks passed
@kbattocchi kbattocchi deleted the fix_test_discrete_outcome branch April 10, 2024 16:02
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.

3 participants