Over time #321
Over time #321
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #321 +/- ##
==========================================
+ Coverage 61.18% 61.35% +0.17%
==========================================
Files 56 56
Lines 4949 4953 +4
==========================================
+ Hits 3028 3039 +11
+ Misses 1921 1914 -7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Great stuff, thanks @yusufuyanik1! My only doubt is the name for the argument - imo either cumulative or delta makes sense (as they are opposites in this case), period change seems ambiguous. Other than that - good to go
| @@ -280,8 +280,8 @@ def over_time( | |||
| The column to group by, by default "ModelID" | |||
| every : Union[str, timedelta], optional | |||
| By what time period to group, by default "1d" | |||
| cumulative : bool, optional | |||
| Whether to take the cumulative value or the absolute one, by default False | |||
| show_changes : bool, optional | |||
There was a problem hiding this comment.
Not sure how much I like this name. I think cumulative is more technically correct - technically the distinction is between cumulative or delta. We could make it a literal between these? That's even more explicit
| df = ( | ||
| df.with_columns( | ||
| Delta=pl.col(metric).cast(pl.Int64).diff().over(grouping_columns) | ||
| PeriodChange=pl.col(metric).diff().over(grouping_columns) |
There was a problem hiding this comment.
PeriodChange seems unnecessarily ambiguous; it's just the delta between snapshot times right? Period implies that could be something different from day-to-day changes
@StijnKas can you check line 302 in Plots.py, are you happy with the y-axis labels?