Repository navigation
fix(model): a parameter a same-named function shadows is a seed, not a knob (#266, #256) - #276
Merged
Merged
Conversation
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.
Closes #266. Closes #256.
Those two are the same defect, reported from opposite ends: #256 from the SBML
side (found while fixing #227 and deferred with an explicit "blast radius that
wants its own measurement"), #266 from the
.netside, months later, off apre-push hook.
Every function gets a parameter slot to hold its evaluated value, and
evaluate_functions()overwrites that slot before every derivative evaluation.#227 flagged the slots bngsim synthesizes. It left the other branch of the same
binding loop — where the function's name matches a parameter the input already
declared, and the function binds to that slot. The engine overwrites it
identically, so the number in the
parametersblock is the seed the slot holdsuntil the function first evaluates, never a knob. But
primary_param_nameslisted it, so an optimizer handed that list spent a coordinate on a column that
is identically zero, and
set_paramaccepted a writeget_paramechoed back andthe next RHS evaluation discarded.
The blast radius is not where #266 said it was
#266's table says 0 of 140 tracked
.netand 4 in generatedbenchmarks/output, and concludes the shape is invisible to CI for want of a tracked file.
Measured across two binaries, that undercounts it by two orders of magnitude:
.net.netunderbenchmarks/BIOMD0000000613alone leaked 141 names. #256's own reproducer(
BIOMD0000000701) leaked 12, and its default sensitivity column set goes36 → 24.
So CI was never short of affected models — it had 107. The sweep that should have
caught this globs
*.net, and this is an SBML<assignmentRule>shape thatreaches
.netonly through conversion. The gap was one input format's sweepstanding in for both, which is a different lesson than "pin the corpus".
tests/data/shadowed_function_param.netnow carries the shape in the formatthose sweeps actually read, and the leak sweep asserts it found that file — a
green run otherwise means only that nothing on this machine has the shape.
Two things the issue did not name
The arithmetic-valued row was excluded for the wrong reason. A shadowed row
written as
recycle 1/4was already kept out of the list, because the.netreader guesses
is_expressionfrom the value text — and #203 then told the userit was a derived parameter, "not independent of its primaries". It is not
derived; nothing recomputes it from primaries, a function overwrites it. The fact
now rides
is_internaland the derived flag comes off, which is also what keepsthe two flags disjoint:
primary_param_namesis the residue of subtracting both,so a row carrying each would be reported under neither reason.
That carve-out also broke #181's "one rule, two readers" invariant. The build
explicitly exempted function-bound parameters from #261's "a constant written as
arithmetic is a knob" demotion, so the loader called
recycle 1/4derived while_classify_parameter_kinds, reading the same line, called it a constant. Thosetwo
.netreaders are supposed to partition the parameter block identically —that is what makes the model-based and text-based codegen paths emit the same
sensitivity RHS. No corpus model has the shape, so nothing could see it. The new
fixture fails
test_the_loader_agrees_with_the_codegen_net_parseragainst theold binary.
Why this is classification only
is_internalis read in exactly one place that affects behaviour — theset_paramrefusal. Everything else is the constructor, the pybind exposure anda docstring, so no evaluation path can see it. Confirmed by A/B across two
builds over 1819
.netmodels:1814 byte-identical.
generate_rhs_c) and trajectories: identical everywhere.That last one also answers the
_CODEGEN_VERSIONquestion by measurement ratherthan by argument. CONTRIBUTING says to bump it for a C++ change that alters
codegen_data(), and this one does alter a flag that reaches codegen — butcodegen re-derives its
is_constfrom the expression text, so the emittedsource did not move even for the one model whose flag flipped. No bump.
The test that failed was the marker
test_biomd701_default_is_its_primary_parametersfailed, and its own docstringsays why: "The other 12 functions carry the name of a parameter the SBML file
itself declared — an
<assignmentRule>— and those are still listed; that isissue #256, deliberately not #227's." #227 encoded the deferral as an assertion;
updating it (35 → 47 internal, 36 → 24 columns) is the fix landing.
set_param's refusal was reworded too. It said the name "is not a parameter ofthe model but a function" — true of a synthesized slot, and exactly what a reader
looking at their own
parametersblock would dispute.Suite
python/tests: 4180 passed, 0 failed, 28 skipped, on this branch rebasedonto
73b0f0a(post-#273, which touches the sensitivity path this change feeds).That is the first fully green run on this machine in a while — #266 was the
standing pre-push failure, so this branch pushed with the hook enabled rather
than
--no-verify.