Conversation
moibra05
requested review from
freshstrangemusic,
jaredlockhart and
yashikakhurana
as code owners
|
|
Preview environment has been deployed. Preview URL: https://pr14209-experimenter.preview.mozilla.cloud It may take up to 5 minutes for the environment to become available. You can monitor deployment status in Argo CD. |
|
Preview environment has been deployed. Preview URL: https://pr14209-experimenter.preview.mozilla.cloud It may take up to 5 minutes for the environment to become available. You can monitor deployment status in Argo CD. |
1 similar comment
|
Preview environment has been deployed. Preview URL: https://pr14209-experimenter.preview.mozilla.cloud It may take up to 5 minutes for the environment to become available. You can monitor deployment status in Argo CD. |
yashikakhurana
left a comment
yashikakhurana
left a comment
There was a problem hiding this comment.
Thank you @moibra05 tested on pr preview, great job, some minor suggestions
| window_metric_data[slug] = branch_metrics | ||
|
|
||
| return window_metric_data | ||
|
|
||
| metric_data = { | ||
| "overall": get_window_metric_data( | ||
| reference_branch, | ||
| self.get_window_results(analysis_basis, segment, "overall"), | ||
| "overall", | ||
| ), | ||
| "weekly": get_window_metric_data( | ||
| reference_branch, | ||
| self.get_window_results(analysis_basis, segment, "weekly"), | ||
| "weekly", |
There was a problem hiding this comment.
@moibra05 is there a way we can break it down in smaller methods?
There was a problem hiding this comment.
I broke up the method into several smaller ones and I intend to eventually move them all into their own file. created an issue for this #14212
| def get_weekly_dates(self): | ||
| weekly_dates = [] | ||
| if not self.is_rollout: | ||
| if self._enrollment_end_date: | ||
| date = self._enrollment_end_date | ||
| while (date + datetime.timedelta(days=7)) <= self.computed_end_date: | ||
| weekly_dates.append((date, date + datetime.timedelta(days=7))) | ||
| date += datetime.timedelta(days=7) | ||
|
|
||
| return weekly_dates |
There was a problem hiding this comment.
yeah like this clean small method
|
Preview environment has been deployed. Preview URL: https://pr14209-experimenter.preview.mozilla.cloud It may take up to 5 minutes for the environment to become available. You can monitor deployment status in Argo CD. |
1 similar comment
|
Preview environment has been deployed. Preview URL: https://pr14209-experimenter.preview.mozilla.cloud It may take up to 5 minutes for the environment to become available. You can monitor deployment status in Argo CD. |
yashikakhurana
left a comment
yashikakhurana
left a comment
There was a problem hiding this comment.
Thank you @moibra05 logic is in models property, clear breakdown. great job
| @@ -803,6 +801,9 @@ def get_context_data(self, **kwargs): | |||
| ) | |||
|
|
|||
| context["relative_metric_changes"] = relative_metric_changes | |||
| context["all_weekly_metric_data"] = experiment.get_weekly_metric_data( | |||
| analysis_basis, selected_segment, selected_reference_branch | |||
| ) | |||
|
|
|||
| return context | |||
|
|
|||
There was a problem hiding this comment.
yes complete simple view
2 participants
Because
This commit
Fixes #14056