Skip to content

refactor(scoring): one implementation of the harmonisation-break correction - #587

Merged
seabbs merged 4 commits into
mainfrom
refactor/consolidate-break-correction
Aug 24, 2026
Merged

refactor(scoring): one implementation of the harmonisation-break correction#587
seabbs merged 4 commits into
mainfrom
refactor/consolidate-break-correction

Conversation

@seabbs-bot

@seabbs-bot seabbs-bot commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Closes #575.

What

scripts/score_releases.jl and src/data.jl each carried a copy of the harmonisation-break net - gross arithmetic.
This leaves one implementation.
src/data.jl gains confirmed_break_steps, which returns the (grid day, correction) pairs, and confirmed_break_correction becomes the half-open window sum over it.
The scorer keeps only what is genuinely its own: mapping a stream label onto which confirmed stream it reads (break_stream_kind) and mapping grid days onto calendar dates.

The blocker recorded in #575 was that sharing would mean moving stream_history and STREAM_ASSEMBLED into src/.
It does not.
The shared piece needs to know which confirmed stream to read, not how to resolve every scored stream, so the stream dispatch stays in the script and src/ gains nothing that has to know about assembled streams.

No scored number changes.

Equivalence

The consolidated break_correction was compared against the pre-consolidation implementation, run verbatim side by side:

  • 175,780 (stream, window) combinations on the current manifest: every stream the scorer knows, every window over the 187-day grid. Zero differences.
  • 106,600 combinations over ten synthetic fixtures covering the edge cases the test files pin: a break day on the first vintage, a break day the history has no vintage for, a gross above its own net step, an empty history, an empty declaration, a missing gross vector, and a manifest with no declaration at all. Zero differences, including which inputs throw.
  • truth_at on the current manifest, every window and both confirmed streams: equal to the raw cumulative difference less the correction throughout.

A full scripts/score_releases.jl run before and after the change produces byte-identical scoring tables.

Audit of the two other concerns raised alongside #575

Both were checked against the code and the data; neither is a bug, so neither is "fixed" here.
Each is pinned by a regression test instead, which is the part of this PR that is not a pure refactor.

Are cumulative streams scored on the level rather than the increment?
No.
forecast_archive stores confirmed_new, confirmed_deaths_new, recovered_new and onset_reports_new, and forecast_stream returns the new count over the horizon for every kind = :cumulative stream.
truth_at matches them: the cumulative-count streams take cum_at(target) - cum_at(made) and only the three occupancy stocks take a level.
On the current manifest, the (2026-07-08, 2026-07-22] window scores confirmed cases at 841 against a cumulative total of 2905, and confirmed deaths at 470 against 1269.
test_scoring.jl now pins the basis of every scored stream, so a stream added on the wrong one fails rather than scoring a cumulative total against an increment forecast.

Are the harmonisation jumps applied correctly?
Yes, everywhere they should be.
On the one break day the manifest lists, 2026-07-22, the correction is 272 cases and 174 deaths (a net of 369 and 236 against printed 24h counts of 97 and 62), and both implementations agree on both streams.
It reaches the forecast truth, the persistence baseline centre and the baseline's own step pool; the per-horizon and per-release tables aggregate scored rows, so they inherit it.
The window is half open on the left in both, so a release made on the break day is not charged for it.
A break day cannot be silently skipped on the current manifest: check_break_gross errors at load if a listed day matches no vintage in either confirmed history.
In the current release set the correction is visible in the scored output.
The 21-day window from 2026-07-01 scores confirmed cases at 1173, not the raw 1445, and the 14-day window from 2026-07-08 at 841, not 1113.
Confirmed deaths over the same 14-day window score at 470, not 644.

The stale committed scoring tables, and a fallback that hides them

data/forecast_overlay.csv in the repository records observed = 1113.0 for confirmed cases at made_date 2026-07-08, horizon 14, target 2026-07-22.
That is the raw cumulative difference. A live run of the scorer gives 841 for the same window, the notified count with the 272-case harmonisation taken out.
Those tables were last written on 2026-07-29 and so predate the correction landing in #524. The 1113 is almost certainly the number that prompted the report that the jumps were not being applied: the code is right and the committed artefact is three weeks stale.

They are meant to be only a fallback. docs.yml re-runs scripts/score_releases.jl before rendering, so the published report uses fresh, corrected scores. The most recent build on main logged Wrote ... scored forecasts, and filtering out the script text GitHub echoes in the run-block header leaves zero real fallbacks across the recent Documenter runs, so the report is correct today and the fallback has not fired.

The hazard is that it is silent. .github/workflows/docs.yml:196 is

(julia --project=docs -e 'using Pkg; Pkg.instantiate()' &&
 julia --project=docs scripts/score_releases.jl) ||
  echo "score refresh failed; using committed scoring overlays"

On any failure the render continues against CSVs that are known to be wrong, with one log line to say so. The scorer makes around a hundred gh release-asset downloads per run, and this repo saw transient CDN 429/503 failures on other jobs today, so the mode is reachable rather than theoretical. A report that quietly published observed = 1113 for a window whose truth is 841 would look like a scoring bug rather than a skipped refresh.

My recommendation is to make it fail loudly. The rescore is cheap next to the fits and the render, and a hard failure is recoverable by re-running the job, whereas a silent substitution of stale data is not detectable from the rendered page. Refreshing or deleting the committed CSVs helps too, but on its own it only moves the staleness rather than removing the silence.

Out of scope for this PR, which is behaviour-neutral. Worth its own issue.

Tests

  • test/test_break_day_correction.jl: the correction on the confirmed-death stream, which had no coverage of its own, with its net, gross, window edge and scored truth pinned on a fixture; and the scorer against confirmed_break_correction over every window of a two-break-day fixture, since the scorer takes calendar dates and the helper grid days and that is the one thing that could drift apart.
  • test/test_break_day_correction.jl, live manifest: two items that assert identities rather than values. The correction over (d - 1, d] equals net - gross for each listed day, the scorer and the helper agree and stay non-negative over every window of the grid, and a scored truth is its cumulative difference less whatever harmonisation the window carries. These hold for any manifest, including one listing no break days, so a data update cannot turn them red on its own.
  • test/test_scoring.jl: truth_at returns an increment for every cumulative stream and a level for every occupancy stock, plus the pinned stream-basis map.

Scoped to this checkout's own test/ root, since the repository has sibling worktrees an unscoped run would collect from.
task format is clean and the test/package/ quality set is 187 pass, 323 pre-existing broken, 0 fail.

This was opened by a bot. Please ping @seabbs for any questions.

@seabbs-bot
seabbs-bot requested a review from seabbs as a code owner August 21, 2026 14:07

@seabbs-review-bot seabbs-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This consolidates the harmonisation-break net - gross arithmetic into one function (confirmed_break_steps in src/data.jl), with confirmed_break_correction and the scorer's break_day_corrections both now delegating to it instead of carrying separate copies. I traced both call paths against the original logic and they preserve the prior behaviour exactly, and the new cross-checking tests (comparing the scorer against the package helper over the whole grid, and against a hand-computed net-minus-gross) give this real coverage. Verdict: sound refactor, worth merging as is.

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.

Comment thread test/test_scoring.jl Outdated
@seabbs-bot

Copy link
Copy Markdown
Collaborator Author

Merge note: this PR conflicts with #586

Verified with git merge-tree off the shared base ca73f75e. One conflict region, in scripts/score_releases.jl, immediately after stream_coverage_start. test/test_scoring.jl is touched by both and auto-merges clean.

Resolution

  1. Keep fix(scoring): prove the persistence baseline only sees the past, withhold it where it degenerates #586's baseline_window_covered in full, with its docstring.
  2. Then keep refactor(scoring): one implementation of the harmonisation-break correction #587's ## Which confirmed stream ... comment and function break_stream_kind(obs, stream) including its two guard lines.
  3. Drop fix(scoring): prove the persistence baseline only sees the past, withhold it where it degenerates #586's ## The stream's own gross-count vector ... comment and its function break_gross_vector(obs, stream) line. That is the pre-refactor(scoring): one implementation of the harmonisation-break correction #587 header which refactor(scoring): one implementation of the harmonisation-break correction #587 replaced.

The trap. The .our side of the conflict ends on function break_gross_vector(obs, stream), but the shared body immediately below the marker is already #587's :cases/:deaths version. Resolving by taking #586's side wholesale leaves a function named break_gross_vector whose body returns :cases/:deaths, while break_day_corrections still calls break_stream_kind. That is an UndefVarError rather than a silent wrong answer, so CI would catch it, but it costs a round trip. Keep the name from #587.

This was opened by a bot. Please ping @seabbs for any questions.

@seabbs

seabbs commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

@seabbs-bot resolve ths conflicts

@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.00%. Comparing base (1e600fe) to head (1b0147c).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #587   +/-   ##
=======================================
  Coverage   93.99%   94.00%           
=======================================
  Files          15       15           
  Lines        3497     3502    +5     
=======================================
+ Hits         3287     3292    +5     
  Misses        210      210           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@seabbs seabbs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

seabbs-bot and others added 4 commits August 24, 2026 13:19
`scripts/score_releases.jl` and `src/data.jl` each carried the
harmonisation-break `net - gross` arithmetic. `src/data.jl` now owns it as
`confirmed_break_steps`, with `confirmed_break_correction` the half-open
window sum over it, and the scorer keeps only what is its own: which
confirmed stream a label reads (`break_stream_kind`) and the grid-day to
calendar-date mapping.

Sharing did not need `stream_history` in `src/` after all. The shared piece
is keyed on which confirmed stream to read, not on resolving every scored
stream, so the stream dispatch stays in the script.

No scored number changes. A full `scripts/score_releases.jl` run before and
after writes byte-identical tables, and the two implementations agree over
175,780 (stream, window) combinations on the current manifest and 106,600
over the synthetic fixtures the test files pin.

Adds regression tests for the two scoring behaviours audited alongside this:
the basis every stream is scored on (an increment for the cumulative counts,
a level for the occupancy stocks) and the manifest's break-day correction
reaching the truth for both confirmed streams.

Closes #575.

Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
…'s data

The manifest-driven items asserted values that only hold for the manifest as
it stands. `data/observations.toml` advances with each situation report and
gains break days as they are found, so a data update could turn them red with
no code having changed, and a strictly positive correction on every listed day
in both confirmed streams is a property of today's data rather than of the
scorer.

The behaviour is now pinned on fixtures built in the test: the confirmed-death
stream's own net, gross, window edge and scored truth, which had no coverage of
its own, and the scorer against `confirmed_break_correction` over every window
of a two-break-day fixture. The scorer takes calendar dates and the helper grid
days, so that mapping is the one thing that could drift apart.

The two live-manifest items stay, as identities rather than values: the
correction over `(d - 1, d]` is `net - gross` for each listed day, the two
paths agree and stay non-negative over every window of the grid, and a scored
truth is its cumulative difference less whatever harmonisation the window
carries. Those hold for any manifest, including one listing no break days.

Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
The comment called its two break days "the current manifest". They are a
constructed declaration, and the manifest lists one day. That was the last
comment in the file claiming a fixture is the live data, which is the
distinction the items around it now draw.

Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
The basis test compared the stream maps against a literal copy of
themselves, so it caught a stream declared on the wrong basis but not one
scored on the wrong basis: nothing in it reached truth_at.

Loop the maps instead and put each stream through truth_at from two
different window openings. An increment moves when the window opens
earlier and a level does not, which separates the two without pinning a
value per stream, and a stream added to a map is covered on the day it is
added.

Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
@seabbs-bot
seabbs-bot force-pushed the refactor/consolidate-break-correction branch from 6bb5453 to 1b0147c Compare August 24, 2026 12:54
@seabbs
seabbs merged commit 2cfe810 into main Aug 24, 2026
28 checks passed
@seabbs
seabbs deleted the refactor/consolidate-break-correction branch August 24, 2026 14:31
@github-actions

Copy link
Copy Markdown
Contributor

📖 Documentation preview is ready!

View the docs for this PR at: http://epiforecasts.io/BVDOutbreakSize/previews/PR587/

This preview will be updated automatically when you push new commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llm-reviewed Reviewed by the wait-for-review loop

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consolidate the harmonisation-break correction: src/ and score_releases.jl each carry a copy

2 participants