fix(classic-io): three defects in the classic-input round trip (segfault, silent empty parse, inflated degree bound) - #398
Open
ofloveandhate wants to merge 1 commit into
Open
Conversation
…396, #397) Pulling on "a records layer cannot reload the systems it wrote" turned up three independent bugs, each of which hid the next. #395 -- SEGFAULT. System::CoefficientBound ends in max(f_vals.array().abs().maxCoeff(), dh_dx.array().abs().maxCoeff(), bound) and with no functions both arrays are EMPTY, where Eigen's maxCoeff() is UNDEFINED BEHAVIOUR. `bertini.System().to_classic_input()` reached it -- the CONFIG section emits `coefficientbound:` -- and crashed the interpreter. DegreeBound(), four lines below in the same file, already guarded exactly this case and returns 0; CoefficientBound never did. Now guarded on BOTH dimensions, since dh_dx is functions x variables and so is empty when either is zero. #396 -- the round trip failed, and failed SILENTLY. to_classic_input() emits a complete Bertini 1 file (CONFIG ... END; then INPUT ... END;) because its purpose is running the same problem in Bertini 1; the grammar reads the INPUT-section BODY. So the two were never inverses. Worse, the Python binding DISCARDED parse()'s return value: bertini::parsing::classic::parse(str.begin(), str.end(), res); return res; // parse() said false and never touched res parse() answers false when the grammar matched nothing or did not consume the input, and leaves the result untouched -- so Python received a structurally valid, entirely EMPTY System and no error, and a caller round-tripping through text carried on with zero functions. Two fixes: StripClassicFileWrappers() in qi_files.hpp lets both System::System(string) and the binding accept a full classic file (a no-op for text that is already a bare body, so nothing that worked before changes), and the binding now raises. It also raises when the input "successfully" declares nothing at all -- an empty string, or an INPUT section with an empty body -- because matching zero declarations is a successful parse of the grammar but still leaves the caller holding a System they did not ask for. #397 -- and the one that actually bites. PowerOperator::Degree(VariableGroup) computed the total degree by SUMMING the per-variable degrees, which is valid only for a monomial: (x+y)^2 has degree 2 in x and 2 in y but TOTAL degree 2, and it reported 4. The overcount scales with the number of variables in the base -- (x^2+y^2+z^2+3)^2 reported 12 instead of 4. It stayed invisible because the same expression takes two paths: an integer exponent written in C++ or Python builds an IntegerPowerOperator, whose group degree is base_deg*exponent and always was right, while the classic parser builds this generic PowerOperator. Both print identically, and both give the correct UNGROUPED Degrees(). Only Degrees(Variables()) differed -- which is precisely what System::DegreeBound() calls, and DegreeBound feeds AMP. So a system round-tripped through classic input tracked under a DIFFERENT adaptive-precision regime than the one it was built from, silently. Any records/replay layer storing systems as classic text was replaying them under different precision decisions than the original run. Now the same shape as PowerOperator::Degree(v) directly above it, with the base's group degree in place of its degree in one variable. Tests in both languages, each naming its issue: the empty-system crash, the full-file round trip (comparing INPUT sections only -- coefficientbound is estimated at RANDOM points, so two emissions of one system legitimately differ there), the bare-body convention still parsing, unparseable and empty input raising instead of yielding an empty system, the total degree of a power of a sum, and DegreeBound() agreeing between a built and a parsed system. Verified: ctest 10/10 suites, pytest 942 passed 1 skipped.
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.
Fixes #395, #396 and #397.
Pulling on "a records layer cannot reload the systems it wrote" turned up three
independent bugs, each of which hid the next.
#395 — segfault
System::CoefficientBoundends inmax(f_vals.array().abs().maxCoeff(), dh_dx.array().abs().maxCoeff(), bound)and with no functions both arrays are empty, where Eigen's
maxCoeff()is undefinedbehaviour.
bertini.System().to_classic_input()reached it — the CONFIG section emitscoefficientbound:— and crashed the interpreter.DegreeBound(), four lines below in the same file, already guarded exactly this case andreturns 0.
CoefficientBoundnever did. Now guarded on both dimensions, sincedh_dxis functions × variables and so is empty when either is zero.
#396 — the round trip failed, and failed silently
to_classic_input()emits a complete Bertini 1 file (CONFIG ... END;thenINPUT ... END;) because its purpose is running the same problem in Bertini 1; the grammarreads the INPUT-section body. They were never inverses.
Worse, the Python binding discarded the parse result:
parse()answersfalsewhen the grammar matched nothing or did not consume the input, andleaves the result untouched — so Python received a structurally valid, entirely empty
System and no error. A caller round-tripping through text carried on with zero functions.
Two fixes.
StripClassicFileWrappers()(inqi_files.hpp) lets bothSystem::System(string)and the binding accept a full classic file — a no-op for text that is already a bare
body, so nothing that worked before changes. And the binding now raises, including when the
input "successfully" declares nothing at all (an empty string, or an
INPUTsection with anempty body): matching zero declarations is a successful parse of the grammar, but it still
leaves the caller holding a System they did not ask for.
#397 — and the one that actually bites
PowerOperator::Degree(VariableGroup)computed the total degree by summing theper-variable degrees, which is valid only for a monomial.
(x+y)^2has degree 2 inxand 2 in
ybut total degree 2, and it reported 4. The overcount scales with the numberof variables in the base:
x^4(x+y)^2(x^2+y^2)^2(x^2+y^2+z^2+3)^2It stayed invisible because the same expression takes two paths: an integer exponent written
in C++ or Python builds an
IntegerPowerOperator, whose group degree isbase_deg*exponentand always was right, while the classic parser builds this generic
PowerOperator. Bothprint identically, and both give the correct ungrouped
Degrees(). OnlyDegrees(Variables())differed — which is precisely whatSystem::DegreeBound()calls, andDegreeBound feeds AMP.
So a system round-tripped through classic input tracked under a different
adaptive-precision regime than the one it was built from, silently. Any records/replay
layer storing systems as classic text was replaying them under different precision decisions
than the original run.
Now the same shape as
PowerOperator::Degree(v)directly above it, with the base's groupdegree in place of its degree in one variable.
Tests
Both languages, each case naming its issue: the empty-system crash; the full-file round
trip; the bare-body convention still parsing; unparseable and empty input raising instead of
yielding an empty system; the total degree of a power of a sum; and
DegreeBound()agreeingbetween a built and a parsed system.
One deliberate asymmetry worth flagging to a reviewer: the round-trip test compares the
INPUT sections only.
coefficientboundis estimated by evaluating at random points, sotwo emissions of the very same system legitimately differ there —
to_classic_input()is notbyte-deterministic, and asserting that it is would be wrong.
Verified:
ctest10/10 suites,pytest942 passed / 1 skipped.ADR
None proposed. Each fix restores documented or obvious intent rather than making a new
tradeoff, and all three are carried by regression tests that fail loudly if undone.