docs: implementation plan for filtered estimates

Five tasks: delete the inert online machinery, add filtered_log_evidence,
add the two learning-curve methods, pin the invariants, record the API break.

Two spec corrections fell out of writing it. The spec claimed filtered results
would be bit-identical before and after converge(); they cannot be. iteration
recomputes the colour partition only when from == 0, so a slice built by
repeated appends keeps insertion order until the first converge() reorders it,
and the scratch clone inherits whichever order it finds — same fixed point,
different path. Corrected to agreement within 1e-8 under tight convergence,
matching the house pattern in tests/ingestion_equivalence.rs. The spec also
declared filtered_pass as Vec<(T, Vec<(Index, Gaussian)>)>, which cannot carry
the evidence its own step 3 harvests; it returns Vec<(T, FilteredStep)>.

CHANGELOG.md is generated by git-cliff, so the spec's "CHANGELOG records the
API break" cannot be satisfied by editing the file — it regenerates. Task 5
records the break through the commit subject and verifies the generated output
instead. cliff.toml has no breaking-change parser at all, which the task is
told to report rather than work around.

An adversarial reviewer checked the plan against the source before this commit
and found four real defects, all in plan text, none in the design:

- Two prescribed mutations provably could not fail their named tests. The
  learning-curve mutation altered only what filtered_pass writes after a slice,
  while the test inspected filtered[0], which is computed from an empty message
  map. Fixed by asserting monotonic mu across the whole curve.
- The ingestion-order fixture used four distinct timestamps, giving one event
  per slice — the exact degenerate shape ingestion_equivalence.rs documents as
  the weak case, making the assertion true by construction. Fixed to several
  events per timestamp with shared competitors.
- filtered_learning_curves was never asserted for content, only for emptiness
  on an empty history.
- A doc comment restated learning_curves' claim that key(idx) is O(n) and the
  method O(n^2). KeyTable::key is self.reverse.get(idx.0) — O(1) — and the
  type's own doc says so. The claim predates reverse becoming a Vec. The plan
  now corrects the original at history.rs:323 rather than copying it.

The reviewer confirmed the central claim by tracing the call graph: N_INF is
{pi: 0, tau: 0} and Mul is a natural-parameter add, so it is an exact
multiplicative identity, and the only write to skill.backward in the crate is
in new_backward_info, reachable only from History::iteration and never from
iterate_to_convergence under either rayon cfg.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T5SYDExxL4vZgvunrcNSMc
This commit is contained in:
2026-08-27 16:04:16 +02:00
co-authored by Claude Opus 5
parent 4fde482e48
commit 187aede924
2 changed files with 1317 additions and 6 deletions
File diff suppressed because it is too large Load Diff
@@ -125,9 +125,18 @@ the present bug reads as plausible.
### The pass ### The pass
```rust ```rust
fn filtered_pass(&self) -> Vec<(T, Vec<(Index, Gaussian)>)> pub(crate) struct FilteredStep {
log_evidence: f64,
posteriors: Vec<(Index, Gaussian)>,
}
fn filtered_pass(&self) -> Vec<(T, FilteredStep)>
``` ```
`posteriors` doubles as the outgoing forward message: the scratch sweep never
writes `backward`, so it stays `N_INF`, and `Skill::posterior()` and
`forward_prior_out` are then the same product.
Walk `self.time_slices` in order, carrying Walk `self.time_slices` in order, carrying
`messages: HashMap<Index, Gaussian>` — the forward message out of each `messages: HashMap<Index, Gaussian>` — the forward message out of each
competitor's most recent appearance. For each slice: competitor's most recent appearance. For each slice:
@@ -146,7 +155,7 @@ competitor's most recent appearance. For each slice:
elapsed = skill.elapsed // copied from the real slice elapsed = skill.elapsed // copied from the real slice
``` ```
This mirrors `Competitor::receive_for_elapsed` (`src/competitor.rs:38`) This mirrors `Competitor::receive_for_elapsed` (`src/competitor.rs:39`)
exactly, including its `message != N_INF` fallback to the prior. exactly, including its `message != N_INF` fallback to the prior.
`skill.elapsed` is reused rather than recomputed: it is maintained by `skill.elapsed` is reused rather than recomputed: it is maintained by
`add_events_with_prior` across out-of-order ingestion, and production `add_events_with_prior` across out-of-order ingestion, and production
@@ -211,7 +220,7 @@ is the property a stored field cannot have, and it is asserted as a test.
| `src/time_slice.rs:110,120` | drop `online` param from `Event::within_priors` | | `src/time_slice.rs:110,120` | drop `online` param from `Event::within_priors` |
| `src/time_slice.rs:585,597,626,634` | drop `online` param from `TimeSlice::log_evidence`; `online \|\| forward` becomes `forward` | | `src/time_slice.rs:585,597,626,634` | drop `online` param from `TimeSlice::log_evidence`; `online \|\| forward` becomes `forward` |
| `src/history.rs:32,63,138,158,174,199,226` | delete the two `online` field declarations (`:32`, `:199`) and the five struct-literal copies | | `src/history.rs:32,63,138,158,174,199,226` | delete the two `online` field declarations (`:32`, `:199`) and the five struct-literal copies |
| `src/history.rs:90-92` | delete `HistoryBuilder::online()` | | `src/history.rs:90-93` | delete `HistoryBuilder::online()` |
| `src/history.rs:402,410` | drop the `self.online` argument | | `src/history.rs:402,410` | drop the `self.online` argument |
| `src/history.rs:1183-1189` | the `..._online` assertion becomes a `forward`-flag assertion; rename the binding to match what it tests | | `src/history.rs:1183-1189` | the `..._online` assertion becomes a `forward`-flag assertion; rename the binding to match what it tests |
@@ -242,9 +251,20 @@ coin flip under filtering, games two through five are not.
### Invariants ### Invariants
1. **Invariant to `converge()`** — `filtered_log_evidence()` and 1. **Invariant to `converge()`** — `filtered_log_evidence()` and
`filtered_learning_curves()` are bit-identical before and after `filtered_learning_curves()` agree before and after `converge()`. This
`converge()`. This is exactly what `skill.forward` fails, and what is exactly what `skill.forward` fails, and what makes a stored field
makes a stored field the wrong mechanism. the wrong mechanism.
Agreement is to tolerance, not bit-identity, and the reason is worth
recording. `iteration` calls `recompute_color_groups`
(`src/time_slice.rs:369`) only when `from == 0`, so a slice built by
repeated appends keeps insertion order until the first `converge()`
reorders it. The scratch clone inherits whichever order it finds, and
greedy coloring over a permuted input can group differently, giving a
different within-slice sweep order — same EP fixed point, different
path to it. Follow the house pattern in
`tests/ingestion_equivalence.rs`: converge tightly (`max_iter: 2_000`,
`epsilon: 1e-12`) and compare within `1e-8`.
2. **Invariant to ingestion order** — events added one at a time produce 2. **Invariant to ingestion order** — events added one at a time produce
the same filtered results as the same events batched. Extends the the same filtered results as the same events batched. Extends the
existing invariant in `tests/ingestion_equivalence.rs`. existing invariant in `tests/ingestion_equivalence.rs`.
@@ -286,6 +306,12 @@ inert.
- **`iterate_to_convergence` leaving test-only status.** Its doc comment - **`iterate_to_convergence` leaving test-only status.** Its doc comment
claims "only used by tests"; that comment must be updated, or it claims "only used by tests"; that comment must be updated, or it
becomes the next piece of load-bearing prose that is quietly false. becomes the next piece of load-bearing prose that is quietly false.
- **Event order is inherited, not normalised.** The scratch clone takes
the real slice's current event order, which differs pre- and
post-`converge()` for incrementally-ingested slices (see *Invariants*).
Results agree to within convergence tolerance rather than exactly.
Normalising the order in the scratch builder would buy bit-identity at
the cost of diverging from what the real sweep does; not worth it.
- **Divergence risk.** If `TimeSlice`'s sweep gains state that the - **Divergence risk.** If `TimeSlice`'s sweep gains state that the
scratch construction does not initialise, the pass silently reads a scratch construction does not initialise, the pass silently reads a
default. The scratch builder must construct `Skill` field-by-field default. The scratch builder must construct `Skill` field-by-field