Skip to content

fix(model): a parameter a same-named function shadows is a seed, not a knob (#266, #256) - #276

Merged
wshlavacek merged 3 commits into
mainfrom
fix/266-shadowed-function-param
Aug 10, 2026
Merged

wshlavacek merged 3 commits into
mainfrom
fix/266-shadowed-function-param

Conversation

@wshlavacek

Copy link
Copy Markdown
Collaborator

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 .net side, months later, off a
pre-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 parameters block is the seed the slot holds
until the function first evaluates, never a knob. But primary_param_names
listed it, so an optimizer handed that list spent a coordinate on a column that
is identically zero, and set_param accepted a write get_param echoed back and
the next RHS evaluation discarded.

The blast radius is not where #266 said it was

#266's table says 0 of 140 tracked .net and 4 in generated benchmarks/
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:

corpus models carrying the shape
tracked .net 140 0
tracked SBML 327 107
generated .net under benchmarks/ 677 4

BIOMD0000000613 alone leaked 141 names. #256's own reproducer
(BIOMD0000000701) leaked 12, and its default sensitivity column set goes
36 → 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 that
reaches .net only through conversion. The gap was one input format's sweep
standing in for both, which is a different lesson than "pin the corpus".
tests/data/shadowed_function_param.net now carries the shape in the format
those 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/4 was already kept out of the list, because the .net
reader guesses is_expression from the value text — and #203 then told the user
it 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_internal and the derived flag comes off, which is also what keeps
the two flags disjoint: primary_param_names is 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/4 derived while
_classify_parameter_kinds, reading the same line, called it a constant. Those
two .net readers 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_parser against the
old binary.

Why this is classification only

is_internal is read in exactly one place that affects behaviour — the
set_param refusal. Everything else is the constructor, the pybind exposure and
a docstring, so no evaluation path can see it. Confirmed by A/B across two
builds over 1819 .net models:

  • classification: exactly 5 models move — the 4 known plus the new fixture;
    1814 byte-identical.
  • emitted C (generate_rhs_c) and trajectories: identical everywhere.

That last one also answers the _CODEGEN_VERSION question by measurement rather
than 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 — but
codegen re-derives its is_const from the expression text, so the emitted
source 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_parameters failed, and its own docstring
says why: "The other 12 functions carry the name of a parameter the SBML file
itself declared — an <assignmentRule> — and those are still listed; that is
issue #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 of
the model but a function" — true of a synthesized slot, and exactly what a reader
looking at their own parameters block would dispute.

Suite

python/tests: 4180 passed, 0 failed, 28 skipped, on this branch rebased
onto 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.

@wshlavacek
wshlavacek merged commit e7cb23b into main Aug 10, 2026
5 checks passed
@wshlavacek
wshlavacek deleted the fix/266-shadowed-function-param branch August 10, 2026 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant