Evaluate BNGL parameter expressions instead of dropping them

[arpitjain099]

Fixes #666.

Adds pybnf/petab/_bngl_expr.py, a parser and evaluator for the parameters-block sublanguage, and points BnglModel at it. The issue's example now resolves:

ids : ['NA', 'V', 'Kd', 'koff', 'kon']
vals: [('NA', 6.022e+23), ('V', 1e-12), ('Kd', 5.0), ('koff', 0.1), ('kon', 3.3211557622052476e-14)]
kon : 3.3211557622052476e-14

which is 0.1 / (5.0 * 6.022e23 * 1e-12) exactly.

On the semantics

Expressions are tokenized and parsed rather than handed to eval, and the reason is not only injection. Python would silently supply its own meanings for the operators:

	checked
2^3	8.0, not 1
-2^2	-4.0 (^ binds tighter than unary minus)
2^3^2	512.0 (right associative)
1/2	0.5
ln(_e)	1.0, and log10(1000) is 3.0

log on its own is rejected rather than treated as ln. BNG2.pl does not accept it, and quietly mapping it would turn a typo into a plausible number instead of an error, which is the failure mode this issue is about.

Resolution is lazy and by dependency, so declaration order does not matter ({'b': 'a*2', 'a': '3'} resolves fine), matching BNG2.pl. A circular definition names the cycle:

CircularParameterError: Parameter 'a' is defined in terms of itself: a -> b -> a

_bngl_expr is stdlib-only and imports nothing else from PyBNF, per the constraint in the issue about moving it to libpetab-python alongside the adapter.

A behaviour change to flag

test_expression_valued_parameter_is_not_evaluated in tests/test_petab_export.py pinned the old contract, so it fails against this change. Since #666 asks for exactly that reversal I have updated it to assert the computed value and renamed it, rather than leaving it red. It is worth a maintainer's eye, because the confinement it pinned is described as deliberate in ADR-0026 and the module docstring still says evaluation is out of scope; both probably want a note now, and I did not want to edit an ADR in a bug-fix PR.

Testing

23 cases in tests/test_petab_bngl_expr.py: the issue's example end to end, each semantic above, the real expression shapes quoted in the issue (loop3, aliasing, 1/lifetime, 10*krZapTcr, ln(Kd_...)), chained dependencies, cycles, and the error paths (unknown name, unknown function, truncated input, division by zero, stray character). Two fail on main with NotImplementedError.

For the wider suite I compared like for like, since this checkout cannot run everything:

baseline (main):        29 failed, 489 passed, 52 skipped
with this change:       29 failed, 511 passed, 52 skipped
new failures:           none

All 29 are pre-existing and want a BNG2.pl I do not have (PybnfError: The location of the BioNetGen simulator (BNG2.pl) is not specified). Three further modules fail to collect for a missing hypothesis, identically before and after, so they are excluded from both runs above.

ruff check is clean on the new module and both test files under the repo's config. bngl_model.py reports one PLW1510 on the pre-existing subprocess.run in is_valid, identical before and after.

The one thing I could not do is check the evaluator against BNG2.pl on a real model, as the issue suggests for anything uncertain, because there is no BNG2.pl here. The semantics above come from the issue and from BNG2.pl's documented behaviour rather than from a run, so they are worth confirming on your side before this lands.

I used an AI assistant while working on this. The runs quoted are ones I executed.

[wshlavacek]

Thanks for this. The parser approach is right, and flagging that you could not check against BNG2.pl was the useful part. I ran that check and pushed the results to your branch.

BNG2.pl disagreed in three places:

-2^2       BNG2.pl gives 4, the branch gave -4
2^3^2      BNG2.pl gives 64, the branch gave 512
rint(2.5)  BNG2.pl gives 3, the branch gave 2

So unary minus binds more tightly than the power operator, and the power operator groups from the left. BioNetGen defines rint as floor(x + 0.5), which rounds a half upward, while Python's round sends a half to the nearest even number. Two of the tests asserted the old answers, which would have hidden this later.

To reproduce, put this in a file, run BNG2.pl on it, then read the parameters block of ev.net:

begin model
begin parameters
  a  -2^2
  b  2^3^2
  c  rint(2.5)
end parameters
begin molecule types
  A()
  B()
end molecule types
begin seed species
  A() 1
end seed species
begin reaction rules
  A() -> B() 1
end reaction rules
end model
generate_network({overwrite=>1})
writeNET({evaluate_expressions=>1,prefix=>"ev"})

It prints 4, 64 and 3. Writing the network with evaluate_expressions turned on is the only export path that prints numbers instead of copying the original text back out.

I also matched the function table to Perl2/Expression.pm in the BioNetGen source. _pi and _e take no arguments, so they are written _pi() and _e() rather than as bare names. floor and ceil are commented out there as unsupported, so BNG2.pl rejects them. asinh, acosh, atanh, sum and avg were missing. The comparison operators, the logical operators and the if function could not be read at all, and two of our published models use them. I left TFUN out on purpose because it reads a data file while a simulation runs, so it is not a constant.

The last change is failure handling. Raising for the whole block on one bad definition loses more than the original bug did. On models/antigen_pulses_harmon2017/antigen_pulses_harmon2017_simplified.bngl the old code returned 9 of 17 parameters and the branch raised instead. get_free_parameter_ids_with_values now returns every parameter it can work out and warns about the rest by name. Across the 303 models with a parameters block in our model collections this moves coverage from 79.3% to 99.0%, an extra 1845 parameters, with no model raising. The 89 still left out are fitting placeholders ending in __FREE, which have no value until a fit supplies one.

The table of expressions and expected values is now checked from both sides. It is pinned in the test file so it runs without BioNetGen installed, and a second test rebuilds the same values from a real BNG2.pl when one is on the path. I confirmed that second test can fail by putting the old 512 answer back and watching it report the mismatch.

I added notes to architecture decision record 0026 in the four places where it says evaluating an expression is out of scope. You were right to leave that alone in a bug fix.