refactor(scoring): share the window start, trim the repeated coverage reasoning - #591
Conversation
The horizon-back-from-made-date rule was written out separately in baseline_window_covered and baseline_draws, so a change to how an incident window opens could reach one and miss the other. Both call window_start now. The coverage reasoning was given in full four times over. Keep it where stream_coverage_start defines it and cross-reference from the rest. Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
![seabbs-review-bot[bot]](https://avatars.githubusercontent.com/in/4624046?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR extracts a small window_start(kind, made_date, horizon) helper to remove duplicated ternary logic from baseline_window_covered and baseline_draws, and trims several overlong comments elsewhere. The refactor itself is behaviour-preserving and covered by existing tests, but the new docstring on window_start overstates what the function relates to — it claims truth and baseline centres share this anchor, which isn't true for truth_at.
Automated first pass by seabbs-review-bot (Claude sonnet), triggered by: first pass. Not a human review. Comment @seabbs-review-bot to ask for another pass: @seabbs any time, the author's agent once it has pushed changes. Add the no-review label to opt this PR out. Ping @seabbs with any questions.
| ## own scores and lose only their relative skill. | ||
| ## Where a scoring window opens: the horizon back from `made_date` for an | ||
| ## incident stream, and `made_date` itself for a level stream, whose value | ||
| ## is the occupancy standing there rather than a change over a window. Both |
There was a problem hiding this comment.
issue This sentence ('Both the truth and the baseline centre are measured from this day') is inaccurate. window_start is only called from baseline_window_covered and baseline_draws — truth_at never calls it. For a level stream, truth_at (line 525) reads cum_at(h, target_date, grid_date), anchored at target_date, not made_date. For an incident stream, truth_at's window opens at made_date itself (line 526), not made_date - Day(horizon), which is what window_start actually returns for that kind. Drop the sentence, or reword it to describe only the baseline's use, e.g. 'The baseline centre is measured from this day; see truth_at for the truth window, which opens elsewhere.'
seabbs-review-bot
Bot
added
the
llm-reviewed
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #591 +/- ##
=======================================
Coverage 93.98% 93.98%
=======================================
Files 15 15
Lines 3493 3493
=======================================
Hits 3283 3283
Misses 210 210 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
📖 Documentation preview is ready! View the docs for this PR at: http://epiforecasts.io/BVDOutbreakSize/previews/PR591/ This preview will be updated automatically when you push new commits. |
2 participants
Follow-up to #586, which merged before these two review fixes reached it.
Both come from
seabbs-review-bot's review on that PR, and both were pushed to its branch after it had already merged, so they are not onmain.One definition of where a window opens
The horizon-back-from-made-date rule was written out separately in
baseline_window_coveredand again inbaseline_draws. The two agreed, so nothing was wrong, but a change to how an incident window opens could reach one and miss the other. Both now callwindow_start(kind, made_date, horizon).Trim the repeated reasoning
The coverage reasoning was given in full four times: on
baseline_window_covered, onstream_coverage_start, inscore_release's docstring, and again in an inline comment there. It now lives wherestream_coverage_startdefines it, and the rest cross-reference it.Two sentences were deliberately kept rather than cut with the rest: that an uncovered window is scored with no baseline rather than a degraded one, and why a saturated or zero centre would flatter the fits it is compared against. Those state the consequence of the check rather than repeat its rule, and they are what a reader needs in order to understand why a row is absent.
Testing
73 pass across the baseline and stream-coverage items, 0 fail. Behaviour is unchanged:
window_startreturns exactly what both call sites computed inline, and the rest of the diff is comments.This was opened by a bot. Please ping @seabbs for any questions.