quantile estimation v1 #2223
quantile estimation v1 #2223
Conversation
|
Your preview environment pr-2223-bttf has been deployed. Preview environment endpoints are available at: |
|
|
||
| @property | ||
| def variance_init(self) -> float: | ||
| multiplier = scipy.stats.norm.ppf(0.975, loc=0, scale=1) |
There was a problem hiding this comment.
Doesn't 0.975 need to be computed from some alpha from the configuration? Maybe QuantileStatistic also needs to take alpha as an input and use it here and use it when computing the n for the issue of requesting a quantile < 0 or > 1 (the other comment above?)
There was a problem hiding this comment.
Nit: Is there a reason you picked ccr over alpha for the name here? I feel like I'd like to start moving just towards using alpha everywhere across engines, but lmk if you disagree.
There was a problem hiding this comment.
updated to alpha
|
A few nits, a few slightly bigger questions. I think once there's a few unit tests in place we can land this in anticipation of merging it with the SQL PR. |
| n: int # number of events here | ||
| nu: float | ||
| ccr: float | ||
| q_hat: float # sample estimate |
There was a problem hiding this comment.
nit: Can we call these quantile_* instead? I'd rather be a touch more verbouse, but up to you.
There was a problem hiding this comment.
I don't think you made this change?
There was a problem hiding this comment.
apologies, forgot to save my file, will be in latest push.
|
|
||
| @property | ||
| def variance_init(self) -> float: | ||
| multiplier = scipy.stats.norm.ppf(0.975, loc=0, scale=1) |
There was a problem hiding this comment.
Nit: Is there a reason you picked ccr over alpha for the name here? I feel like I'd like to start moving just towards using alpha everywhere across engines, but lmk if you disagree.
…ile, and renaming q_* to quantile_*
| mu_1 = mu_0 + self.prior_effect.mean | ||
| v = 0.5 * self.prior_effect.variance | ||
| self.prior_a = GaussianPrior(mean=mu_0, variance=v, pseudo_n=1) | ||
| self.prior_b = GaussianPrior(mean=mu_1, variance=v, pseudo_n=1) |
There was a problem hiding this comment.
Should the pseudo_n from the self.prior_effect GaussianPrior be passed through here?
There was a problem hiding this comment.
I only ask because it seems like there isn't a way to pass through a flat, improper prior to this test class, right? Should we set up a way to do that?
You can kind of hack it into the existing GaussianPrior where if prior_effect.pseudo_n = 0, then we just pass this through like I suggest. Or we can be more explicit, where you can also specify no prior and that modifies this method as well.
There was a problem hiding this comment.
good idea, I implemented this. When we use non-flat priors in the future, we still want to use pseudo-n = 1.
…ior_effect in GaussianEffectABTest
| self.assertEqual(result.chance_to_win, 0.5) | ||
| self.assertEqual(result.expected, 0) | ||
|
|
||
| def test_inexact_log_approximation(self): |
There was a problem hiding this comment.
Why do we need to replicate this test here? Maybe copy paste?
There was a problem hiding this comment.
good catch, i removed this
packages/stats/gbstats/gbstats.py
Outdated
| @@ -211,20 +216,40 @@ def get_configured_test( | |||
| ), | |||
| ) | |||
| else: | |||
| assert isinstance( | |||
| stat_a, type(stat_b) | |||
| ), "stat_a and stat_b must be of same type." | |||
There was a problem hiding this comment.
Sorry to nit, I should have pointed this out initially, but this will pass even so long as stat_a is the same class or a subclass of stat_b. e.g is stat_a is a SampleMeanStatistic and stat_b is a Statistic, this will pass.
So I think we should instead just do assert type(stat_a) is type(stat_b)
…corporating n* approximation in stats engine
There was a problem hiding this comment.
Looks good to me! There were two further nits that I just encountered when reading your PR again (sorry for missing these earlier), but you can accommodate them by first merging in my PR to your branch: #2288
…s to clusterd statistic (#2288)
This code has 3 primary classes:
Quantile Statistic - creates effect estimate and CI for quantile nu when data are iid (e.g., event-level data).
Quantile Statistic - creates effect estimate and CI for quantile nu when data are clustered (e.g., user-level data).
GaussianBayesianABTest - creates posterior distribution using prior on treatment effect and frequentist effect estimate.