fix(experiments): apply new count method and fix continuous #27639
Conversation
|
Size Change: 0 B Total Size: 1.16 MB ℹ️ View Unchanged
|
posthog/hogql_queries/experiments/test/test_trends_statistics_continuous.py
Outdated
Show resolved
Hide resolved
posthog/hogql_queries/experiments/experiment_trends_query_runner.py
Outdated
Show resolved
Hide resolved
45355e5 to
ddfb8cf
Compare
📸 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. |
e32516c to
1000c41
Compare
| ) | ||
| credible_intervals = calculate_credible_intervals_v2_count([control_variant, *test_variants]) | ||
| case _: | ||
| raise ValueError(f"Unsupported metric type: {self._get_metric_type()}") |
There was a problem hiding this comment.
I agree that we shouldn't return results for unsupported metric types. However, there are likely some experiments with unsupported metric types that are currently returning results but will start throwing errors after this PR is merged. Have you thought about how to handle any complaints from these users?
There was a problem hiding this comment.
_get_metric_type() is defaulting to count, so we won't throw errors. So this is a safe guard for future work to make sure all metric types are handled. Does that make sense?
|
Nice work 🙏 Meta feedback on this PR: I think it would benefit from a clearer problem description. Now:
Better: ProblemCurrently, we're applying the continuous calculation for any Trend metric which contains the "math" field. This is incorrect, we should only apply it to the valid continuous math types, and throw an error for the rest. Changes... |
| @@ -2363,3 +2366,45 @@ def test_validate_event_variants_no_exposure(self): | |||
| } | |||
| ) | |||
| self.assertEqual(cast(list, context.exception.detail)[0], expected_errors) | |||
|
|
|||
| def test_get_metric_type(self): | |||
There was a problem hiding this comment.
Thanks for adding this
|
|
||
| # Test: ~$105 mean with narrow interval due to old implementation | ||
| self.assertAlmostEqual(intervals["test"][0], 103, delta=3) | ||
| self.assertAlmostEqual(intervals["test"][1], 107, delta=3) |
There was a problem hiding this comment.
Oh, I intentionally didn't change the v1 values previously.
There was a problem hiding this comment.
Yeah, I was a little confused here. But as the old implementation is also getting the "total" as input (what it gets from the query runner), not the mean, the test cases should be updated to reflect that. And hence the assertions had to be updated.
Does that make sense? I think the values in the assertions make more sense now as well.
There was a problem hiding this comment.
Yep, the explanation makes sense. Just flagging that I intentionally didn't change the behavior / return values for v1. I'm not strongly opposed to doing so, but the original intent was to keep v1 exactly how it was.
There was a problem hiding this comment.
I see. To be clear, the implementation for v1 has not changed here. Only the input for the test cases that were added here 1979d74. And the reason is to reflect what the behavior is and has been in production: the queries does not return the mean, but the total.
1000c41 to
4df1c30
Compare
4df1c30 to
0e4ce77
Compare
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Problem
if self.query.count_query.series[0].math:. This is also set for count metrics. F.ex, when selectingTotal count, the value here istotal.Changes
Note: I introduced a new
ExperimentMetricType. This is only used in theposthog.hogql_queries.experimentsmodule at the moment, so it lives there for now. But it can easily be pulled out into f.exposthog.schemaif/when we want to use this more broadly, f.ex in the front-end.How did you test this code?