feat: experiment defaults to a fixed relative MDE. #31025
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
The PR removes the dynamic calculation of the Minimum Detectable Effect in favor of a fixed constant, simplifying the running time logic and related tests.
- RunningTimeCalculatorLogic: Replaces dynamic MDE logic with the DEFAULT_MDE constant in
/frontend/src/scenes/experiments/RunningTimeCalculator/runningTimeCalculatorLogic.tsx. - Utils Cleanup: Eliminates the
getMinimumDetectableEffectfunction and its related types/logic in/frontend/src/scenes/experiments/utils.ts. - Test Adjustments: Removes redundant MDE test cases in
/frontend/src/scenes/experiments/utils.test.ts. - Experiment Logic Update: Strips out dynamic MDE computation from
/frontend/src/scenes/experiments/experimentLogic.tsx.
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
rodrigoi
force-pushed
the
experiments/updating-to-relative-MDE
branch
from
cb01d22 to
670d4ec
Compare
|
Size Change: -382 B (0%) Total Size: 13.2 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
andehen
previously approved these changes
Apr 10, 2025
rodrigoi
force-pushed
the
experiments/updating-to-relative-MDE
branch
from
4c42872 to
350c535
Compare
rodrigoi
changed the title
rodrigoi
changed the title
There was a problem hiding this comment.
PR Summary
This PR simplifies experiment design by replacing dynamic Minimum Detectable Effect (MDE) calculations with a fixed 30% relative value.
- Added
minimumSampleSizePerVariantcalculation inexperimentLogic.test.tswith significantly reduced sample size values (from thousands to tens) - Implemented variant count multiplication in sample size calculation in
runningTimeCalculatorLogic.tsxto properly account for multivariate experiments - Removed all
getMinimumDetectableEffectfunction references and related imports across the codebase - Updated test cases to reflect the new fixed MDE approach with adjusted expected values for running time calculations
- Added constant
DEFAULT_MDE = 30inexperimentLogic.tsxto replace the dynamic calculation
5 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Problem
Following up on this comment from #30792
Changes
We are removing
getMinimumDetectableEffect, their tests and all usages in favor of a constant we'll use as default.Does this work well for both Cloud and self-hosted?
How did you test this code?