Conversation
This reverts commit 1e49335.
af8e489 to
f54659c
Compare
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Okay one small labeling change but otherwise I think looks good thanks @moibra05 🙏 🎉
mikewilli
left a comment
mikewilli
left a comment
There was a problem hiding this comment.
Left a few comments, but looks close. Do we need to do a data migration here as well or is this backwards compatible?
| raw_segment_data = JetstreamData(segment_data) | ||
| raw_data[window][AnalysisBasis.ENROLLMENTS][segment] = raw_segment_data | ||
| data = raw_segment_data.model_copy(deep=True) | ||
| data = raw_data[window][AnalysisBasis.ENROLLMENTS][segment] = JetstreamData( | ||
| segment_data | ||
| ) |
There was a problem hiding this comment.
I don't remember the specifics -- is doing the deep copy no longer important?
| data = raw_data[window][AnalysisBasis.ENROLLMENTS][segment] = JetstreamData( | ||
| segment_data | ||
| ) | ||
| data.separate_weekly_retention_data( |
There was a problem hiding this comment.
Other operations on data appear underneath if data checks, should this one as well?
| retention_data = self.get_retention_by_window( | ||
| RETENTION_2_WEEKS_WINDOW_INDEX, weekly_data, Metric.RETENTION | ||
| ) | ||
| def separate_weekly_retention_data(self, weekly_data): |
There was a problem hiding this comment.
A comment here would probably be helpful.
| retention_data = self.get_retention_by_window( | ||
| RETENTION_2_WEEKS_WINDOW_INDEX, weekly_data, Metric.RETENTION | ||
| ) | ||
| self.remove_retention_data() |
There was a problem hiding this comment.
We call this in the client code as well, should it be removed in one of those places?
| def test_append_retention_3_days_extracts_legacy_data(self): | ||
| retention = JetstreamDataPoint( | ||
| metric=Metric.RETENTION_3_DAYS_LEGACY, | ||
| statistic=Statistic.BINOMIAL, | ||
| branch="control", | ||
| point=0.65, | ||
| segment=Segment.ALL, | ||
| window_index="4", | ||
| ) | ||
|
|
||
| data = JetstreamData([]) | ||
| data.append_retention_3_days([retention]) | ||
|
|
||
| self.assertIn(retention, data) |
There was a problem hiding this comment.
Why'd we remove this test? Does this PR change the 3-days retention metric?
| point=0.65, | ||
| segment=Segment.ALL, | ||
| window_index="2", | ||
| def test_separate_weekly_retention_data_splits_retention_by_week(self): |
There was a problem hiding this comment.
Might be worth having a test that only passes week 1 for retention to make sure the most common case works.
3 participants
Because
This commit
Fixes #16421