fix(test): the ingestion-order property was comparing two truncated fits
`ingestion_order_does_not_change_the_answer` failed with a 1.2e-6 gap in
mu and read as a violation of the invariant. It was not. Both sides ran
`max_iter: 200` against `epsilon: 1e-10`, and the batched side stopped
at the cap with a step of 3.4e-9 — so the test compared two fits that
had not converged and attributed the difference to ingestion order.
Raised to 20_000, at which both converge and the property holds. Runtime
is unchanged at 0.05s, because converging is what the iterations were
for.
The test now asserts `report.converged` on both sides before comparing.
That is the part worth keeping: any test that compares two fits for
equality is measuring truncation unless it first establishes that both
reached a fixed point. `ingestion_equivalence.rs` already did this;
`properties.rs` did not.
Found because c12bc83 made `ConvergenceReport` `#[must_use]`, which is
the same failure #50 describes — a short fit is wrong by a little and
looks entirely plausible. The blanket `let _ =` that commit applied to
78 call sites was too blunt here: binding the report to `_` silenced the
one signal that would have caught this, in a test whose whole purpose is
to compare two fits.
Refs #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:
+20
-4
@@ -26,7 +26,10 @@ const KEYS: [&str; 8] = ["a", "b", "c", "d", "e", "f", "g", "h"];
|
|||||||
fn history_from(games: &[(usize, usize)]) -> History {
|
fn history_from(games: &[(usize, usize)]) -> History {
|
||||||
let mut h = History::builder()
|
let mut h = History::builder()
|
||||||
.convergence(ConvergenceOptions {
|
.convergence(ConvergenceOptions {
|
||||||
max_iter: 200,
|
// 200 was not enough: the batched side stopped at the cap with a
|
||||||
|
// step of 3.4e-9, so this test was comparing two truncated fits and
|
||||||
|
// attributing the gap to ingestion order.
|
||||||
|
max_iter: 20_000,
|
||||||
epsilon: 1e-10,
|
epsilon: 1e-10,
|
||||||
..ConvergenceOptions::default()
|
..ConvergenceOptions::default()
|
||||||
})
|
})
|
||||||
@@ -114,14 +117,21 @@ proptest! {
|
|||||||
fn ingestion_order_does_not_change_the_answer(games in pairs()) {
|
fn ingestion_order_does_not_change_the_answer(games in pairs()) {
|
||||||
let batched = {
|
let batched = {
|
||||||
let mut h = history_from(&games);
|
let mut h = history_from(&games);
|
||||||
let _ = h.converge().unwrap();
|
let report = h.converge().unwrap();
|
||||||
|
prop_assert!(
|
||||||
|
report.converged,
|
||||||
|
"batched side stopped at {} iterations with step {:?}; comparing \
|
||||||
|
two fits that have not converged measures truncation, not order",
|
||||||
|
report.iterations,
|
||||||
|
report.final_step
|
||||||
|
);
|
||||||
h
|
h
|
||||||
};
|
};
|
||||||
|
|
||||||
let incremental = {
|
let incremental = {
|
||||||
let mut h = History::builder()
|
let mut h = History::builder()
|
||||||
.convergence(ConvergenceOptions {
|
.convergence(ConvergenceOptions {
|
||||||
max_iter: 200,
|
max_iter: 20_000,
|
||||||
epsilon: 1e-10,
|
epsilon: 1e-10,
|
||||||
..ConvergenceOptions::default()
|
..ConvergenceOptions::default()
|
||||||
})
|
})
|
||||||
@@ -139,7 +149,13 @@ proptest! {
|
|||||||
.unwrap();
|
.unwrap();
|
||||||
}
|
}
|
||||||
|
|
||||||
let _ = h.converge().unwrap();
|
let report = h.converge().unwrap();
|
||||||
|
prop_assert!(
|
||||||
|
report.converged,
|
||||||
|
"incremental side stopped at {} iterations with step {:?}",
|
||||||
|
report.iterations,
|
||||||
|
report.final_step
|
||||||
|
);
|
||||||
h
|
h
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user