-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore(experiments): Stats cleanup #27151
base: master
Are you sure you want to change the base?
Conversation
conversion rate) compared to all other variants, including the control. It uses samples | ||
drawn from the posterior Beta distributions of each variant's conversion rate. | ||
conversion rate) compared to all other variants, including the control. It uses Beta | ||
distributions to model conversion rates, which is the conjugate prior for binomial data. |
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.
What's a "conjugate prior for binomial data"? In the code comments I'd aim for the same level of simplicity as in our docs :)
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.
Explained with aeff274
self.assertEqual(significance, ExperimentSignificanceCode.LOW_WIN_PROBABILITY) | ||
self.assertEqual(p_value, 1) | ||
|
||
# Check credible intervals | ||
self.assertAlmostEqual(intervals["control"][0], 0.05, delta=0.05) |
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.
Uh oh, the tolerated delta here is definitely too wide 👍
@@ -58,16 +60,16 @@ def run_test(stats_version, calculate_probabilities, are_results_significant, ca | |||
|
|||
self.assertEqual(len(probabilities), 2) | |||
if stats_version == 2: | |||
self.assertAlmostEqual(probabilities[0], 0.15, delta=0.1) | |||
self.assertAlmostEqual(probabilities[1], 0.85, delta=0.1) | |||
self.assertAlmostEqual(probabilities[0], 0.149, delta=0.1) |
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.
What's the reason for keeping delta=0.1
here? The range seems too wide (0.049 -> 0.249, i.e. 4.9% to 24.9%)
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.
Not sure, must've skipped over it. Updated with e37b745
See #26713
Changes
Some cleanup while working on PostHog/posthog.com#10217:
places
instead ofdelta
for credible intervals.How did you test this code?
Tests should pass.