feat!: make a short fit an error and raise the default iteration cap
`ITERATIONS` was 30, and overrunning it returned `Ok` with `converged: false`. Both halves were wrong. The cap is a runaway guard, not a budget: the sweep exits as soon as the step falls below `epsilon`, so a high cap costs nothing on a history that converges. Measured on one needing four sweeps, `max_iter` 30 and 100_000 both finish in 4 iterations and ~130us. So 30 could never make anything faster — it could only stop a healthy history early, and it did: 160 events over 100 competitors already needs 42. Not scaled to the history, because iteration count tracks how loopy the graph is rather than how big it is. At a fixed 320 events over 40 slices, varying only the competitors sharing them: 3 competitors needs 2_789 sweeps, 10 needs 1_068, 100 needs 90, 400 needs 2. Three orders of magnitude on identical event and slice counts, so any formula in those two numbers would be badly wrong on some real shape. A single value set high enough that reaching it means oscillation is the honest version. With the cap raised, stopping at it means something is genuinely wrong, so `converge` now returns `InferenceError::NotConverged` rather than a flag on a success. A short fit is wrong by a little — every rating finite, the ordering sensible, nothing saying the numbers were still moving — and a flag has to be checked while `let _ = h.converge()` is the natural way not to. That is not hypothetical: it is how a real defect hid in this crate's own test suite. `converge_partial` returns the short fit for callers who want one. Only a single existing test needed it, which is the evidence that a capped fit is a deliberate choice rather than the common case. Also corrects the `ITERATIONS` docs, which claimed convergence cost is "roughly linear in the cap". It is linear in the iterations actually run. BREAKING CHANGE: `History::converge` returns `Err(NotConverged)` where it previously returned `Ok` with `converged: false`. Callers that want the old behaviour should use `History::converge_partial`. The default `max_iter` changes from 30 to 10_000, so a history that was silently truncated will now converge properly and its numbers will move. Closes #50 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
This commit is contained in:
+10
-3
@@ -62,10 +62,17 @@ impl Default for ConvergenceOptions {
|
||||
}
|
||||
|
||||
/// Post-hoc summary of a `History::converge` call.
|
||||
///
|
||||
/// From [`History::converge`](crate::History::converge) this always describes a
|
||||
/// converged fit — stopping at `max_iter` is
|
||||
/// [`InferenceError::NotConverged`](crate::InferenceError::NotConverged) there.
|
||||
/// From [`History::converge_partial`](crate::History::converge_partial) it may
|
||||
/// not be, and `converged` is what says so.
|
||||
#[derive(Clone, Debug)]
|
||||
#[must_use = "a ConvergenceReport carries `converged`, and a fit that stopped \
|
||||
at `max_iter` is wrong by a little rather than loudly broken — \
|
||||
check it, or bind it to `_` to say you have decided not to"]
|
||||
#[must_use = "from `converge_partial` this may describe a fit that stopped at \
|
||||
`max_iter`, which is wrong by a little rather than loudly \
|
||||
broken — check `converged`, or bind it to `_` to say you have \
|
||||
decided not to"]
|
||||
pub struct ConvergenceReport {
|
||||
pub iterations: usize,
|
||||
pub final_step: (f64, f64),
|
||||
|
||||
Reference in New Issue
Block a user