feat(experiments): running time calculator for continuous
#29316
Conversation
📸 UI snapshots have been updated6 snapshot changes in total. 0 added, 6 modified, 0 deleted:
Triggered by this commit. |
…ing-time-calculator-continuous
…om/PostHog/posthog into running-time-calculator-continuous
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added support for continuous metrics in the experiment running time calculator, enabling users to estimate experiment duration for property-based metrics alongside count metrics.
- Added variance calculation for continuous metrics using
VARIANCE_SCALING_FACTOR_CONTINUOUS * averagePropertyValuePerUser ** 2in/frontend/src/scenes/experiments/RunningTimeCalculator/runningTimeCalculatorLogic.tsx - Modified query building to fetch property sum data for continuous metrics with
PropertyMathType.Sum - Updated UI in
RunningTimeCalculatorModal.tsxto conditionally display property value metrics for continuous experiments - Added comprehensive test coverage in
runningTimeCalculatorLogic.test.tsthat validates calculations against reference spreadsheet values
3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| math_property: metric.metric_config.math_property, | ||
| math_property_type: TaxonomicFilterGroupType.NumericalEventProperties, |
There was a problem hiding this comment.
style: Consider adding a null check for metric.metric_config.math_property as it's used directly without verification.
|
Size Change: 0 B Total Size: 9.73 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated10 snapshot changes in total. 0 added, 10 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
|
|
||
| return ((16 * variance) / ((minimumDetectableEffect / 100) * standardDeviation) ** 2) * numberOfVariants | ||
| return ((16 * variance) / (minimumDetectableEffectDecimal * standardDeviation) ** 2) * numberOfVariants |
There was a problem hiding this comment.
variance = standardDeviation ** 2, so effectively this cancels out in this calculation.
So it simplifies to (16 / MDE ** 2) * numberOfVariants. So variance has no effect on the calculation.
Is that intentional?
I think it would be good with some comments in this function as well to explain how it works. F.ex, where does 16 come from?
| ~{humanFriendlyNumber(averagePropertyValuePerUser, 0)} | ||
| </div> | ||
| </div> | ||
| )} | ||
| <div> | ||
| <div className="card-secondary">Estimated variance</div> | ||
| <div className="font-semibold">~{humanFriendlyNumber(variance, 0)}</div> |
There was a problem hiding this comment.
I think maybe it's more meaningful to show standard deviation to the user, as it has a much more intuitive meaning
|
Really like the new calculator! 🙌 |
Changes
Implement sample size / running time calculation for
continuousmetrics.Cross-checked with: https://docs.google.com/spreadsheets/d/11alyC8n7uqewZFLKfV4UAbW-0zH__EdV_Hrk2OQ4140/edit?gid=2067479228#gid=2067479228
How did you test this code?