feat: Always give the present 10% of the distribution history's width#142
feat: Always give the present 10% of the distribution history's width#142
Conversation
|
@cbachhuber just an idea how to solve that we always have 10% for the current data |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #142 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 490 502 +12
=========================================
+ Hits 490 502 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This isn't a bad idea at all! It also doesn't suffer from the complications of my approaches. Should we go with this? |
| timestamps = [ | ||
| (snapshot.timestamp - latest_snapshot_time) / 60 for snapshot in status_history | ||
| ] | ||
|
|
||
| timestamps_extended = add_future_timestamp(timestamps, relative_extension=0.1) | ||
|
|
||
| data = {"Time (minutes)": timestamps_extended} | ||
| for user_status in UserStatus: | ||
| counts = [snapshot.counts[user_status] for snapshot in status_history] | ||
| data[user_status.value] = [ | ||
| snapshot.counts[user_status] for snapshot in status_history | ||
| ] | ||
| *counts, | ||
| counts[-1], | ||
| ] # repeat last value in future timestamp |
There was a problem hiding this comment.
I considered extracting all of this to a function that creates data. However, later in this function, we still need timestamps_extended, so I didn't extract that.
We could extract a function that computes data and also returns the time range. What do you think about that?
|
Also note 60ce7c9: I temporarily added the ability to speed up time via an env var for my experiments (sample usage: Do you think we should keep that, @BayerC? |
|
@BayerC since the majority of changes now comes from me, can you review this PR and merge if you're happy? |
cbachhuber
left a comment
There was a problem hiding this comment.
LGTM, approving to make this mergeable after @BayerC's review
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider handling or explicitly guarding against an empty
status_historyinshow_status_history_chart/add_future_timestamp, since both currently assume at least one timestamp and will fail withtimestamps[-1]orcounts[-1]otherwise. - The
relative_extension=0.1magic value is now part of the chart semantics; it may be clearer to promote this to a named constant (similar toMAX_NUMBER_OF_TICKS) so the 10% extension behavior is easier to discover and tweak.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider handling or explicitly guarding against an empty `status_history` in `show_status_history_chart` / `add_future_timestamp`, since both currently assume at least one timestamp and will fail with `timestamps[-1]` or `counts[-1]` otherwise.
- The `relative_extension=0.1` magic value is now part of the chart semantics; it may be clearer to promote this to a named constant (similar to `MAX_NUMBER_OF_TICKS`) so the 10% extension behavior is easier to discover and tweak.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
I dont see any time on the x-axis anympore is this intendet @cbachhuber
|
| ) | ||
| from open_cups.types import UserStatus | ||
|
|
||
| MAX_NUMBER_OF_TICKS = 10 |
There was a problem hiding this comment.
shouldnt move this more closered (1 line above) where it is used?
Its very specific to the function
There was a problem hiding this comment.
Right, my thinking was to put the 'config' to the top of the file. Similar as with the color values.
I'm absolutely open to moving this inside the function, if you prefer.
There was a problem hiding this comment.
I would prefer having it in the function since a more a named magic number than a config
There was a problem hiding this comment.
you could do in follow up, get this merged!
|
@BayerC wrote
In your screenshot, you actually see |
its fine i waited not long enough |

Closes #65
Closes #141