fix(endgames): PSEG left previous_approximation_ a copy of final; bind the approximation accessors - #393
Open
ofloveandhate wants to merge 1 commit into
Open
Conversation
…d the accessors The power series endgame assigned previous_approximation_ = final_approximation_ at the BOTTOM of its convergence loop while testing the loop condition at the TOP, so the assignment ran one last time on the way out. After every successful PSEG run the two vectors were EQUAL. Cauchy never had this -- it returns from its acceptance gate before the corresponding assignment -- so the two endgames disagreed about their own post-run state. PSEG now matches Cauchy, in both the fixed-precision and adaptive drivers; the loop's own entry condition is kept so a FinalTolerance() >= 1 still refuses to enter. Two consequences beyond the accessor. ZeroDimSolver reports accuracy_estimate_user_coords as the distance between the final approximation and the previous one, so for power-series solves it was identically ZERO -- an exactly-perfect accuracy for every such path. And EndgameBase::approximate_error_ was never initialized, so ApproximateError() read an indeterminate value before any run; it is now infinity, which is the only safe sentinel here. NaN would be the more honest 'no estimate', but it loses every relational comparison, and the PSEG gate has the shape "error > tolerance" -- a NaN there makes the test false, skips the loop entirely and reports instant success. Binds the two endgame accessors that existed in C++ but were unreachable from Python: previous_approximation() and approximate_error(), beside the already-bound final_approximation(). Returned by value like the existing one, since the endgame overwrites its approximation vectors on the next run. Together they hand back the pair the convergence test compares plus the norm between them -- a second sample of the root at a known coarser accuracy, so a caller can judge how a derived quantity (a Jacobian's singular values, say) MOVES as the approximation improves instead of thresholding it at a single point. Tests assert the three accessors are a coherent triple, for both endgames at every tracker flavor and precision. The comparison is RELATIVE to the reported error, not anchored: the converged error is below any absolute tolerance one would write, so an anchored check passes against the unfixed endgame (measured -- gap 0 vs err ~1e-12 satisfies diff <= 1e-10*max(1,err)). Verified by reverting the fix: 8 targeted failures across every PSEG flavor, 428 tests clean with it. Deliberately does NOT assert a nonzero error -- fixed_multiple_cauchy at precision 16 converges with two successive approximations agreeing bitwise, so coherence is the invariant and a nonzero gap is not.
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
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.
Why
Deflation needs to decide the rank of a Jacobian at an approximate root, and a single
spectrum at a single point cannot do it — a tiny singular value is indistinguishable from a
perturbation artifact. What separates them is watching how the value moves as the
approximation improves. The endgame already computes a second, deliberately coarser sample of
the root and the distance to it, and then throws both away:
PreviousApproximation()andApproximateError()exist in C++ but have never been reachable from Python.Binding them turned up a bug that made one of them useless anyway.
The bug
The power series endgame assigned
previous_approximation_ = final_approximation_at thebottom of its convergence loop while testing the loop condition at the top, so the
assignment ran one last time on the way out. After every successful PSEG run the two vectors
were equal. Cauchy never had this — it returns from its acceptance gate before the
corresponding assignment — so the two endgames disagreed about their own post-run state.
Fixed in both PSEG drivers (fixed-precision
RunImpland adaptiveRunImplAMP) by breakingbefore the assignment, matching Cauchy's shape. The loop's own entry condition is deliberately
kept so a
FinalTolerance() >= 1still refuses to enter at all, exactly as before.This reached further than the accessor.
ZeroDimSolvercomputesaccuracy_estimate_user_coordsas the distance between the final approximation and theprevious one:
smd.accuracy_estimate_user_coords = (DehomogenizePoint(solutions_post_endgame_[i]) - DehomogenizePoint(ctx.endgame.PreviousApproximation<BaseComplexT>())).lpNorm<Infinity>();For power-series solves that was identically zero — an exactly-perfect accuracy reported
for every such path. The value flows into the records archive
(
records/solver_recording.hpp) and MPIpath_result. It is not part of any digest, so thepersistent-identity contract is untouched.
Also fixed
EndgameBase::approximate_error_was never initialized, soApproximateError()read anindeterminate value before any run — not acceptable for a member now exposed to Python.
It is initialized to infinity, deliberately not NaN. NaN is the more honest "no estimate",
but it loses every relational comparison, and the gates read it in both directions:
cauchy.hpphasapproximate_error_ < FinalTolerance()(NaN → false → not converged, fine),but
powerseries.hpphaswhile (approximate_error_ > FinalTolerance())— NaN there is false,the loop never runs, and the endgame reports instant success on whatever happened to be in
final_approximation_. Infinity fails the<gate and passes the>gate, and is not a falseclaim: with no approximation computed, the honest error bound is unbounded.
What is bound
previous_approximation()andapproximate_error()on every endgame flavor.final_approximation()was already bound. Returned by value like the existing accessor, sincethe endgame overwrites its approximation vectors on the next run (ADR-0051's owned-copy
doctrine for eigenpy-backed vectors).
Working end to end:
Tests
approximation_accessors_are_a_coherent_triple, added to both generic endgame test headers soit runs for every tracker flavor at every ambient precision, plus Python interface tests.
Two things the tests deliberately do not assert, both learned by measurement:
ApproximateError() > 0.fixed_multiple_cauchyat precision 16 converges with twosuccessive approximations agreeing bitwise on this system, whose root is exactly 1.
Coherence is the invariant; a nonzero gap is not.
diff <= 1e-10 * max(1, err)passes against the unfixedendgame — with the bug
gap = 0anderr ~ 1e-12, so it reduces to1e-12 <= 1e-10. Theconverged error is below any absolute tolerance one would think to write, so the check is
relative to the error itself, where the bug is a ratio of exactly 1.
Verified the regression test actually bites: reverting only the PSEG change and rebuilding
gives 8 targeted failures across every PSEG flavor and precision.
Verification
ctest --test-dir build/core— 10/10 suites pass (428 endgame cases)pytest python/test/— 926 passed, 1 skippedpython tools/py_doclint.py— clean, at baselineunder
core/include, so the zero-undocumented ratchet is unaffected.ADR
Assessed, and I do not think one is warranted. The one thing a reader might "helpfully" undo is
the loop now carrying both a
while (error > tolerance)condition and anif (error <= tolerance) break;, which looks redundant — but that is a bug fix carried by aregression test that fails loudly across 8 suites, which the ADR bar explicitly exempts. The
reasoning for both the break and the infinity sentinel is written inline at each site. Happy to
add one if you would rather have the guardrail.