tests: cover the zero_blocks JSON reader - #52
Merged
Merged
Conversation
zero_blocks is well tested through C++ parameters, and none of that reaches the converter: parameter_handler::insert() takes a ready-made std::vector<block_ref> and bypasses json_parameter_converter.h entirely. A deck is the only way a user supplies this, and the deck path had no test at all. Two tests, both mutation-verified. The first pins the pair's ORDER, not merely that it parses. In the 'decoupled' system dR_x/dy is identically zero while dR_y/dx is 1, so ["x","y"] is a true claim and ["y","x"] is a false one, and verify_zero_blocks reports the false one. Transposing the converter's emplace_back, or dropping the entries entirely, each fail it; a test that only checked the honest case would pass under both. The second covers the converter's own error path, and the first version of it could not fail. Asserting that the message mentions "zero_blocks" is not enough: the solver validates the entries too and rejects a malformed pair the converter let through, with a message that also says zero_blocks. Deleting the parse-time check left that test green. It now also requires the word "pair", which only the parse-time check can produce -- by the time the solver sees it, the entry is already a well-formed two-element block_ref with a nonsense name in it. That distinction is the point of the check rather than a detail of it. "each entry must be a [row, column] pair" sends a user to the offending deck line; "names an unknown that is not declared in 'unknowns'" sends them to the unknowns list, which is correct. No production code changed. 27/27 in test_vector_newton, 284/284 overall. Second item of #21.
5 tasks
petlenz
added a commit
that referenced
this pull request
Sep 19, 2026
Review of #53 found three wrong figures in my own comments and one test that could not fail. Fixing both, plus the production comment the review showed was half the story. The comment table was wrong in two ways. "Condition ~2/eps" is ~4/eps -- sigma max ~2, sigma min ~eps/2 -- so every condition number I quoted was understated fourfold. Worse, the load-bearing row was simply false: I wrote that beyond cond 1e16 dx goes non-finite and allFinite catches it. Measured, at eps = 2e-16 the condition number is 2.6e16, dx is FINITE at 4.5e12, and the backward-error check is what fires. The boundary between the two guards is not a condition number at all -- it is fl(1 + eps) == 1.0 making the pivot exactly zero, which happens between eps = 1.5e-16 and 1e-16. The table now includes that row and the eps = 1e-13 the test actually runs, which it previously omitted. "The iterate runs to -1.0010e+10 on the first update" mis-attributed a real number: -1.0010e10 is where it ends after 50 iterations; the first update gives -1.0008e10. The assertion is now EXPECT_EQ(x, 0.0) rather than a 1e3 bound -- the solve cold-starts and the guard fires on iteration zero, so the iterate is exactly the seed, and an approximate bound would also admit a damped step. The tests could not tell WHICH guard stopped the solve: allFinite and the backward-error check produce an identical observable. A new test raises linear_tolerance past the measured 5.5e-07 and changes nothing else, so the same step must now be APPLIED. Verified by mutation -- rejecting any large but finite dx, as allFinite would if it could, fails it. eps is a parameter now rather than buried in the material, so the window can be swept instead of asserted in prose, which is how the wrong numbers survived. The switch case also moved to match the enum order; near_singular was appended, so decoupled keeps its value and #52's "mode": 3 still means what it says. One test is NOT here, on purpose. I wrote one for the '* rn' scaling -- drop it and an absolute threshold would abort a healthy solve whose residual is in stress units -- and it was vacuous. Inside the solver, J = [1 1; 1 1+1e-8] with R = (-1e9, -1.001e9) gives a backward error of EXACTLY 0, so neither form fires and it passed either way; a standalone program with the same inputs gives 1.19e-07. Backward-stable LU lands on either side of zero by rounding. The gap is recorded in the test file rather than papered over with a test that cannot fail. The production comment claimed the two guards differ because "finiteness alone would not" catch the near-singular case. True, but it omits the converse, which is the sharper half: once dx is inf, back_err is NaN, and NaN > tol is FALSE, so the backward-error check would pass an infinite step. They are independent because of IEEE comparison semantics. Also recorded: -ffast-math defeats BOTH legs at once, folding J*dx - R to zero and making allFinite() true for inf. Finally, linear_tolerance's doc called it a backward-error tolerance. It is not, in the textbook sense -- that form is normalized by |J|*|dx|, and since partialPivLu is backward-stable it sits at ~1e-17 even at cond 1e15 and would never fire. Switching to it would silently disable the guard. Renamed to what it is: a relative-residual tolerance. 302/302.
petlenz
added a commit
that referenced
this pull request
Sep 19, 2026
A third review of #53 found that the guard's discrimination is scale dependent, and the previous comment invited the wrong reading of it. Measured on ONE system at fixed cond ~4e13, varying only the problem scale, unmutated code: scale rel. residual fires forward error of the step 1e0 5.5e-07 yes 8.0e-04 1e2 2.4e-07 yes 8.0e-04 1e3 1.1e-16 NO 8.0e-04 1e6 1.2e-16 NO 8.0e-04 The step is equally wrong at every scale. The check fires only where the constants leave sub-ULP noise in the residual -- at scale 1e3, 1.001*1000 is exactly representable and the noise disappears. So the guard is not detecting ill-conditioning and never was; it detects whether J*dx reproduces R, which is a different question that happens to coincide at some scales. Both the header comment and the test now say so, and the test records that its constants are load-bearing for that reason rather than arbitrary. Two smaller things from the same review, folded in because the file was open. The parse-time pair check prevents undefined behaviour, not merely a worse message: without it, names[1] on a one-element entry is an out-of-bounds vector read, and the ["x"] and [[]] cases abort the binary with heap corruption instead of failing cleanly. The comment framed it purely as diagnostics quality, which invites someone to relax it. The zero_blocks message assertion on "zero_blocks" is weak on its own -- json_reader_registry wraps every reader failure as "failed to read parameter 'zero_blocks'", so it passes for an nlohmann type error too. Only the "pair" assertion discriminates, and that one is deliberately coupled to the converter's wording; both now say as much at the assertion site. Also from #52: the JSON document now derives its mode from system_mode::decoupled rather than a literal 3, and the honest sub-case asserts converged() rather than only that nothing threw. 302/302.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second item of #21.
zero_blocksis well tested through C++ parameters — and none of that reaches the converter.parameter_handler::insert()takes a ready-madestd::vector<block_ref>and bypassesjson_parameter_converter.hentirely. A deck is the only way a user supplies this, and the deck path had no test at all.This is the same shape as three defects this stack already produced: a parameter the code reads that nothing exercises.
zero_blocksis the one where getting it wrong is worst — the solve still converges to the right root and only the consistent tangent is corrupted.Two tests, both mutation-verified
The pair's order, not just that it parses. In the
decoupledsystemdR_x/dyis identically zero whiledR_y/dxis 1, so["x","y"]is a true claim and["y","x"]is a false one thatverify_zero_blocksreports.emplace_back(names[1], names[0])A test that only checked the honest case would pass under both.
The converter's error path — and my first version of this one could not fail. Asserting that the message mentions
zero_blocksis not enough: the solver validates the entries too, and rejects a malformed pair the converter let through with a message that also sayszero_blocks. Deleting the parse-time check left the test green:It now also requires the word "pair", which only the parse-time check can produce — by the time the solver sees it, the entry is already a well-formed two-element
block_refwith a nonsense name in it. With the check deleted, the test fails and prints exactly the message above as the reason.That distinction is the point of the check rather than a detail of it: "each entry must be a [row, column] pair" sends a user to the offending deck line; "names an unknown that is not declared" sends them to the
unknownslist, which is correct.Scope
No production code changed — the converter turned out to be correct. My first attempt at the order test failed and I checked whether the converter or the test was wrong before concluding: probing the built material showed
verify_zero_blocks = 1andzero_blocks = [[y, x]]arriving intact, so the bug was mine (verify_zero_blocksruns insidesolve(), and I was driving the graph withctx.update()).282/282 pass.
Remaining on #21: the backward-error guard's reachability, the extra factorization per solve, and the damping/line-search decision.