Repository navigation
bngl: warn when a source's first run starts from a state from_bngl drops (#1366) - #1388
Merged
wshlavacek merged 7 commits intoNov 3, 2026
Merged
Conversation
…ops (lanl#1366) The actions after a BNGL file's generate_network are dropped as the author's experiment. When some of them set a parameter or a concentration the first simulate or parameter_scan starts from, and the loaded model holds another value, a ConversionWarning now names them. The scan follows save/reset as BNG2.pl does; each change is judged in the model's own arithmetic, so a setConcentration to the species' declared amount is not named. The loading docs and from_bngl docstring now say which actions before generate_network apply, and the benchmark README says the suites run each network from its seed state, not the published protocol.
…d no BNG2.pl (lanl#1366) On a runner without BioNetGen the twelve network tests skipped, and took the Ubuntu leg's skip count past its ceiling (227 > 225). The judging needs only a network: a hand-written .net of the same model serves. The three tests that go through from_bngl itself still need BNG2.pl.
…review) Review of lanl#1388. Each change has a test that fails on the PR head. - bifurcate is a run. The scan stopped only at simulate and parameter_scan, so a file whose first run is a bifurcate had nothing to say. The bistable toggle.bngl (and ExampleModel4_v6.bngl) declares Yinit 0, then runs setConcentration("Y()",50) and bifurcate as its only run: it loaded from Y=0 with no warning. It now warns on Y() (X() is set to its declared 0). - save/reset use labels, as BNG2.pl does. One slot served every label, so saveConcentrations("zero"); resetConcentrations() restored the "zero" state where BNG2.pl, finding no unlabeled save, puts back the model as declared (a false warning), and a reset to a label saved before a later save lost a real change. - The scan starts at the top of the file. A resetConcentrations() with no save after generate_network puts back the declared seed, undoing a setConcentration placed before generation that the loaded network holds. That reset is now named. - Each target is judged by what the run meets: a parameter's last setParameter, a species' last setConcentration and the addConcentration calls after it. setParameter("k",2); setParameter("k",1) on a k declared 1 warned about k=2; setConcentration("A(b)",0); addConcentration("A(b)",100) on an A of 100 named both. - A fixed species' "$S()" pattern is matched, as get_concentration reads it, and an exact name is looked up before the component-order scan over every species. - The changes come in source order (they came species first). - The warning names the caller's line through Model.load too, with _simulator._stacklevel_outside_bngsim rather than stacklevel=3, which named _model.py's own line there. It says "first run", not "first simulate" (wofsy_goldstein's is a scan), and "loaded without them", not "as declared" (actions before generate_network are applied). - The tests said wofsy_goldstein sets alpha from 0.01 to 0.1; it declares 10. On the repo's 1,070 .bngl files: 97 flagged, 43 warn, 58 actions named (was 95, 41, 56). The new ones are the two toggle files; one other file names the same actions, now in source order. Nothing else moved.
… review) Found by a blind review of lanl#1388. - generation_source cut only at a generate_network. BNG2.pl generates the network at the first run of a file that never calls one, but the loader kept every setParameter and setConcentration in such a file, those between later runs included, so the model held the state of none of its runs. kinetics_mb1n.bngl loaded with its last phase's Agconc 604.8 and const 0 (no ligand at all); its first run has 7.46 and 1. The cut is now the first generation call or run, in the loader and in the dropped-actions scan alike. 15 of the repo's 1,070 .bngl files load a different network: exactly those with no generate_network and a state change after their first run. None of bng_parity, the NF suite or ode_fullnet loads through generation_source. - simulate_psa and simulate_protocol are BNG 2.9.3 runs (both call simulate), so they are experiment verbs now: before generation they were kept and run during loading, and the scan read past them. - benchmarks/README said every suite runs the network from its seed state. The ODE suite runs BNG2.pl over the whole file and integrates the .net it leaves, which BNG2.pl rewrites before each simulate after a state change: its egfr_path is the equilibrated network with EGF_tot, where all 18 species move in 120 s. 3 of 18 move from the seed state (models/net/ode/egfr_path.net). The paragraph now says which suites start from the seed state and which does not. - The docs and from_bngl docstring said runs are among the actions left out before generate_network; with the cut at the first run there are none, and setOutputDir and quit are left out too. The warning's corpus counts are unchanged: 97 flagged, 43 warn.
…1366 review) The blind review's re-check of the previous commit: - "where BNG2.pl generates the network itself" is not true when the first run is network-free: BNG2.pl generates only for a network method, and 179 of the 228 corpus files with no generate_network have an NF first run. The cut is still the state that run starts from, which is what the docs, docstrings, changelog and tests now say. - A simulate that names a suffix or prefix writes its own .net and leaves <name>.net alone, so the ODE suite's gene_expr_3stage (both runs suffixed) integrates its seed state (ran it the suite's way: DNA(promoter~0) 1, the rest 0). The README says so, rather than that every multi-phase file is integrated from a later phase. It also says which SSA models read the curated nets, and that from_bngl keeps the actions before generation. Left as is: a run before an explicit generate_network (no corpus file has one) now loads the first run's state, which the warning is about, and still forwards that later call's max_iter/max_agg/max_stoich as the bound.
The "Python suite counts · ubuntu-latest" leg skipped 236 tests, over its ceiling of 235. Main skips 233 there, and this PR added three tests gated on BNG2.pl, which that runner does not have. Two of them only test how from_bngl wires the warning: that it names the changes, and that protocol=True gives none. They now load the hand-written .net of the same model in place of the BNG2.pl run (load_bngl monkeypatched), so they run on every leg. The corpus test, which is about networks BNG2.pl generates, stays gated: one skip on that leg, 234.
This was referenced Nov 3, 2026
wshlavacek
added a commit
that referenced
this pull request
Nov 6, 2026
…solve Every nightly since 0b7b342 (runs 41865647650 and 42064543593, and this PR's run 42187705330) alerts on BIOMD0000000268|ode: n_jac_evals 102 -> 134, x1.31 against the x1.25 bound, with steps 918 -> 944 and error-test failures 23 -> 35. The row still passes (0/44044 cells, err/movement 3.4e-8 -> 4.6e-8), and the suite's work totals hold at x1.000 on every counter. The last clean nightly ran b1d5f7a. Of the merges from there to 0b7b342, only #1471 can move a plain ODE run on the analytical Jacobian: - #1300's core changes act only with sensitivities. Without them it records which slots start below 0, and nothing reads that. - #1394 declines the analytical Jacobian outright, for finite differences. This row keeps it (n_jac_rhs_evals 0). - #1388, #1484 and the release commit change BNGL loading, protocols and the version string. #1471 replaced SUNLinSol_Dense with bngsim's dense solver, which reads a row the Newton matrix holds alone as x_i = b_i/a_ii. Ten of this model's 44 rows are such rows at every sampled time: their species' rows of the Jacobian are 0, so their Newton rows are rows of the identity. Six of those species are read by other rows (H2O2 by 12), so partial pivoting can move them, and there the pivoted back-solve and x_i = b_i agree only up to rounding. The two nightlies either side of 0b7b342 agree with that. Of the rows that PASS in both, 30 of the 1136 on the dense solver changed their work, and none of the 116 on KLU did. Of the 26 changed rows that stayed on the analytical Jacobian, Jacobian evaluations rose in 12, fell in 10 and held in 4; over them, steps went 43185 -> 43021 and Jacobian evaluations 5453 -> 5444. Rounding, not a slowdown. This is by elimination and that split; no Linux build bisected it. #1471's own nightly run did not run rr_ode. On this Intel Mac, main's core gives 918 steps and 102 Jacobians for the row. Only this row is copied, verbatim from run 42187705330, rather than the whole verdicts.json: the rest of that file would bring in a different runner CPU's wall times and unrelated changes between failing states. The tally is unchanged. Replaying that run's verdicts, and main's runs 41865647650 and 42064543593, against this baseline gives no alert.
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.
Addresses #1366, items 1, 3 and 4. Item 2, replaying the protocols in the suites, is a maintainers' call and needs #1364 (#1377) first, so it is not in this PR.
What #1366 asks, and what this does
Model.from_bngldrops a BNGL file's actions aftergenerate_networkas the author's experiment (intended, documented). The issue asks whether loading such a file should say so when it matters (3), whether the docs describe it right (4), and whether the benchmark READMEs should say what the suites simulate (1).3, the warning. A
ConversionWarningnow names the dropped actions that set a parameter or a concentration the firstsimulateorparameter_scanstarts from, where the loaded model holds another value._bngl_loader.state_changes_before_first_run):generate_network, wheregeneration_sourcecuts;save/resetConcentrationsandsave/resetParametersas BNG2.pl does.dropped_state_changes):setParameterto the parameter's own value is not named;setConcentrationto the species' own amount, the value evaluated in the model's own arithmetic, so"Ag_tot_1"compares as the number it is;addConcentrationof 0;protocol=Truereturns the actions instead, without the warning.Why the second step. The text scan alone flags 95 of the repo's 1,068
.bnglfiles. Of the 94 that load, 53 set everything to what the model already holds:setConcentration("Ag(DNP)","Ag_tot_1")on a species seeded withAg_tot_1,setParameter("ART",0)on anARTdeclared 0. A warning on those would be wrong more often than right. The 41 that warn hold 56 actions that change a value by the model's own numbers. Five of them are species written with their components in another order than the network's canonical name (LPS(MD2,TLR4,CD14,LPS)forLPS(CD14,LPS,MD2,TLR4)), which the check matches.This corrects the issue's table. Two of the six "first simulated state differs" files do not differ:
egfr_pathdeclaresEGF 0, so itssetConcentration("EGF",0)is a no-op. The run that matters is the opposite: it addsEGF_totonly after equilibrating, so the ODE suite's 120 segfr_pathrun has no ligand. 3 of its 18 species move (Grb2 and Sos settle); withEGF_totadded, all 18 do.mwcdeclarescoopRTandcoopB0, the values its actions set.egfr_net_red(egf(r)0 before equilibrating) andwofsy_goldstein(alpha0.01 → 0.1; itsbetais already 0.01) do differ, and warn.4, the docs.
loading-models.mdand thefrom_bngldocstring said onlygenerate_networkruns. Actions before it are applied, as BNG2.pl applies them, except runs and file writes. Both now say so, and describe the warning.1, the benchmark README. A paragraph in
benchmarks/README.md's Benchmarking section says the suites run each network from its seed state over the suite's horizon, not the published protocol. It explains why parity and timing still compare like with like, and gives theegfr_pathcase.Tests (
test_bngl_dropped_state_changes.py, 19).net, no BNG2.pl)addConcentrationof 5 is named; controls, not named: a parameter to its value, a species to its amount, to the expression it was seeded from, with its components in another order, adding 0; only the changing actions are named; an unnamed species counts as a changefrom_bngl(3, need BNG2.pl)protocol=True;protocol=Truedoes not warn;wofsy_goldsteinwarns onalphaand notbeta, andegfr_pathdoes not warnBefore this PR,
from_bnglgave no warning at all, so each test that expects one fails on main (K1 below).Mutations
resetConcentrationsignoredaddConcentrationnever namedMeasured
.bnglfiles, the scan flags 95. 94 of those load (one generates no network), and 41 warn. Inbenchmarks/models/bngl(103 files), 3 warn: bothegfr_net_redandwofsy_goldstein.test_loads_a_corpus_model_and_simulatesloadingegfr_net_red.bngl, a true one.pre-commit run --all-files(new files staged) andsphinx -Ware clean.