refactor: unify convergence defaults, validate builders, clear dead code
Convergence configuration had two disagreeing sources of truth and one
misleading report:
- `EpsilonOrMax::default()` capped at 10 iterations while
`ConvergenceOptions::default()` allowed 30, and which applied depended on
whether inference went through `run_chain` or a `Schedule`. The schedule
default now derives from `ConvergenceOptions`.
- A graph with no iterating factors reported `converged: false` with an
infinite step, despite being at its fixed point after the setup pass. It
now reports converged with a zero step.
- `TimeSlice::iterate_to_convergence` hard-coded an epsilon and a
20-iteration cap matching neither. It reads `self.convergence` and is
scoped to `#[cfg(test)]`, which is all it was ever used by.
`HistoryBuilder::p_draw` and `::convergence` now validate their arguments
like `score_sigma` already did, instead of accepting a negative `p_draw` or
an `alpha` of zero — the latter leaves every EP update unapplied, so
inference silently returns the priors.
Removing the `#[allow(dead_code)]` masks let the compiler report what they
were hiding: four `OwnedGame` fields that were stored and never read, two
`ColorGroups` helpers and three `SkillStore` helpers used only by tests, and
`iterate_to_convergence` above. Test-only items are now `#[cfg(test)]` and
the unread fields are gone.
Also exported `HistoryBuilder`, which was public but unreachable — callers
could chain `History::builder()` but could not name the type — and added
`Rating::{prior, beta, drift}` and `Index::get`, so handles the API hands
out can be read back.
Two goldens moved, both convergence residuals rather than exact values:
`iterate_to_convergence` now runs to 30 iterations instead of 20, landing
nearer the symmetric truth of 25.0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnsaJg74eNSva3PJjK2eej
This commit is contained in:
+31
-5
@@ -32,8 +32,17 @@ pub struct EpsilonOrMax {
|
||||
|
||||
impl Default for EpsilonOrMax {
|
||||
fn default() -> Self {
|
||||
// Matches today's hard-coded tolerance and iteration cap.
|
||||
Self { eps: 1e-6, max: 10 }
|
||||
// Derived from `ConvergenceOptions` so there is one source of truth for
|
||||
// the tolerance and iteration cap. These previously disagreed: this
|
||||
// default capped at 10 iterations while `ConvergenceOptions` allowed 30,
|
||||
// and which applied depended on whether inference went through
|
||||
// `run_chain` or a `Schedule`.
|
||||
let defaults = crate::ConvergenceOptions::default();
|
||||
|
||||
Self {
|
||||
eps: defaults.epsilon,
|
||||
max: defaults.max_iter,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -50,10 +59,16 @@ impl Schedule for EpsilonOrMax {
|
||||
}
|
||||
|
||||
let mut iterations = 0;
|
||||
let mut final_step = (f64::INFINITY, f64::INFINITY);
|
||||
let mut converged = false;
|
||||
// With no iterating factors the graph is already at its fixed point:
|
||||
// the setup pass above is all there is to do. Reporting `converged:
|
||||
// false` with an infinite step for that case gave callers a false
|
||||
// negative.
|
||||
let mut final_step = (0.0, 0.0);
|
||||
let mut converged = true;
|
||||
|
||||
if n_setup < factors.len() {
|
||||
final_step = (f64::INFINITY, f64::INFINITY);
|
||||
converged = false;
|
||||
for _ in 0..self.max {
|
||||
let mut step = (0.0_f64, 0.0_f64);
|
||||
|
||||
@@ -113,7 +128,8 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn report_marks_converged_when_no_iterating_factors() {
|
||||
// No iterating factors → 0 iterations, converged stays false (loop never ran).
|
||||
// A graph of only setup factors has nothing to iterate, so it is at its
|
||||
// fixed point after the setup pass: 0 iterations, and converged.
|
||||
let mut vars = VarStore::new();
|
||||
let out = vars.alloc(N_INF);
|
||||
let mut factors = vec![BuiltinFactor::TeamSum(TeamSumFactor {
|
||||
@@ -122,5 +138,15 @@ mod tests {
|
||||
})];
|
||||
let report = EpsilonOrMax::default().run(&mut factors, &mut vars);
|
||||
assert_eq!(report.iterations, 0);
|
||||
assert!(report.converged);
|
||||
assert_eq!(report.final_step, (0.0, 0.0));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn default_matches_convergence_options() {
|
||||
let schedule = EpsilonOrMax::default();
|
||||
let options = crate::ConvergenceOptions::default();
|
||||
assert_eq!(schedule.max, options.max_iter);
|
||||
assert_eq!(schedule.eps, options.epsilon);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user