[Montana]

Fixes #1512.

Important

A .net species index that is not its line was skipped, and the species read by its line, so 2 B() 5 above 1 A() 1 ran the file's reaction on species 1 against B(): Aobs(1) = 1.839397 for the file's 0.367879, with no error. BioNetGen's two readers disagree on such a file, so it is now refused rather than read one way.

The problem

On main (0b2055a9), the issue's file: k = 1, one reaction 1 1 0 k degrading species 1, and the group Aobs on species 1.

species lines	run_network 3.0	BNG2.pl 2.9.3 readFile	bngsim on main	this PR
2 B() 5, 1 A() 1	0.367879	1.839397	1.839397	refused
1 A() 1, 3 B() 5 (a gap)	SIGSEGV	0.367879	0.367879	refused
1 A() 1, 1 B() 5 (a duplicate)	1.839397	0.367879	0.367879	refused
0 A() 1, 1 B() 5 (0-based)	SIGSEGV	0.367879	0.367879	refused
2 A() 1, 3 B() 5, reaction and group on 2	refuses ("No reactions")	1.839397	1.839397	refused
B() 5, 1 A() 1	0.367879	1.839397	1.839397	refused
1 A() 1, 3 C() 2, 2 B() 5, reaction and group on 2	1.839397	0.735759	0.735759	refused

The closed form for species 1 = A() is e^-1 = 0.367879. Nothing was raised, warned or logged.

What the referees do

• run_network places each species at the index it declares.

• BNG2.pl's readFile places it at its line: SpeciesList.pm strips the index and numbers by position, and writeNetwork renumbers the file.

• On 18 shapes I ran through both (the issue's 6 and 12 more), they agree exactly where the index is its line:

  • indices in order;
  • an unindexed block;
  • an unindexed line followed by 2 …;
  • 1 … followed by an unindexed line;
  • leading zeros (01, 02, and 001, 2);
  • a tab after the index.

  Both read each of those to 0.367879.

Important

So the one rule both referees agree on is index = line, and that is the rule the loader now holds a species line to. Remapping to the declared index would contradict readFile, and keeping the line would contradict run_network.

The fix

src/net_file_loader.cpp, parse_species:

• $\color{#8250df}{\textsf{Hold an index to its line.}}$ A species line with an index is refused unless the index is its 1-based line. The message is "species line '2 B() 5' has index 2 on the species block's line 1: reactions and groups name a species by its index, and BioNetGen's two readers place a species differently when its index is not its line (run_network at its index, BNG2.pl's readFile at its line). Number the species 1, 2, 3, ... in the order they are listed, or leave the index off".
• $\color{#8250df}{\textsf{Compare as a number.}}$ The index is compared as a number, so 01 on the first line loads, as both referees read it.
• What doesn't change:
  • an unindexed line keeps its place (The .net species-block parser is narrower than run_network's: an unindexed line is silently dropped, and an expression initial concentration is refused #600);
  • a seed species block is held to the same rule, since BNG2.pl and the loader read it as the species block.
• One refusal is stricter than both referees: an index past 2^32. With 4294967297 A() 1 on line 1, run_network's integer wraps the index to 1 and both referees read the file alike. Here it is refused, as an index that is not its line.
• The fix is in the one place every route reads. Model.from_net (codegen on and off), Model.load, parse_net_file and net_to_sbml all read through parse_species. net_to_sbml used to return a report that called the misread model fine, because its check compares bngsim with itself; it now raises.

Docs: docs/user-guide/loading-models.md's parse_net_file section gets a paragraph on the species line's index.

How this was checked

1. Reproduced on main with the issue's script, and on every row of the table.
2. The referees, on 18 shapes (above):
   • The 7 the two agree on load here, to 0.367879.
   • The rest are refused here. Each is a shape the two read differently, or one run_network crashes on or refuses, or the 2^32 index (stated above).
   • +1 and 1.0 as an index: BNG2.pl refuses them ("Invalid Molecule name"). bngsim refused them before this change and still does, as an unparsable initial concentration.
3. Every route refuses the issue's file: Model.from_net, with and without codegen, Model.load, parse_net_file (ValueError) and net_to_sbml. All five loaded it on main.
4. Nothing that loaded before moves: I loaded every unique .net I have, 179 files (the repository's plus local copies), on main and here. 179 of 179 give the same model and the same parse_net_file dictionary, and all 179 load on both.
5. New tests fail on main for the right reason: test_net_species_index_is_its_line.py, 18 cases.
   • 10 fail on main: the issue's file; every route; the 6 refused shapes; the offset block; the seed species block.
   • 8 controls pass on main: the 7 shapes the referees agree on, each to e^-1 within 1e-7, and a write_net round trip of an unindexed block.
6. Mutation tests: 4 mutants, all killed, on the final code:
   • the check removed (10 failing);
   • the index compared as text, not a number (2, the leading-zero controls);
   • the line off by one (12);
   • only an index below its line refused (5).
7. C++ tests: ctest, 8 of 8 pass.
8. Special legs over the 358 test files that load a .net, on the final tree:
   • BNGSIM_SKIP_AUDIT=strict: 13,760 passed, 84 skipped;
   • forced cc (BNGSIM_CODEGEN_THRESHOLD=1): 13,746 passed, 98 skipped;
   • forced MIR: 13,717 passed, 127 skipped, the same skips as main on these files.
9. Full Python suite, macOS arm64, no corpus (as CI runs it), on 0b2055a9, on the final tree: 19,381 passed, 140 skipped, 1 xfailed, 0 failed, so 19,522 collected, main's 19,504 plus these 18.
   • Collected floor: 71 above PYTEST_COLLECTED_FLOOR (19,451), inside the 500 slack.
   • Skips: 140, under PYTEST_SKIPPED_CEILING (252). The new tests have no skip.
   • These ran before a two-line rewording of one test comment; the file, after it, passes 18 of 18 under each leg.
10. Docker, Linux x86-64 (python:3.12, --platform linux/amd64, gcc 14, built from the final tree with the [antimony,hdf5] extras and scipy):

• The new file: 18 passed.
• Full suite: 18,817 passed, 537 skipped, 1 xfailed, 4 failed. The 4 are this container's standing failures (test_codegen_compile_reaping, test_codegen_toolchain_verdict, test_run_tests_sh, test_vendor_script_git_cwd), which fail the same way on main in it and on simulator: leave assignment-rule species at the value a run reports (#1510) #1637's and net: refuse a .net file from which no block is read (#1511) #1638's runs; none touches the loader.

11. pre-commit --all-files passes (13 hooks, clang-format, ruff and mypy among them). ci/changelog.py check passes, and local sphinx-build -W exits 0.

Which CI legs this PR runs

The MIR and Windows legs run only on the files in their paths: filters. This PR changes src/net_file_loader.cpp, a test file, a changelog fragment and a docs page.

leg	runs here?	what covers it
python-tests.yml (Ubuntu, macOS)	yes	the whole suite
native-tests.yml	yes (src/**)	ctest, 8 of 8 locally
windows-tail.yml (MSVC)	no: no Windows filter on main lists src/net_file_loader.cpp	#1629's 60f6528 adds it to windows-tail.yml; until that merges, this change reaches MSVC only through wheels.yml on a push to main
mir.yml	no	forced MIR locally, on the 358 files that load a .net (step 8)
windows-klu.yml, windows-nfsim.yml	no	not run: their tests target the KLU and NFsim paths

Checked against past review findings

• More referee cases than the issue names (net: read block markers as BNG2.pl reads them, and refuse what it refuses (#1498) #1539): 18 shapes, including two tested on species 2 as well as 1, through both referees. The rule is stated only as far as they go.
• Sibling sites:
  • Every route that reads a .net goes through parse_species, and each is pinned.
  • Parameters, reactions, groups and functions are referenced by name, so a permuted index there does no harm, as the issue says.
  • The two test-harness readers the issue names (parity_checks/bng_parity/_bng_common.py:145, benchmarks/suites/forward_sens/run.py:456) assume index = line. That is now guaranteed, so they are left alone.
• Exception types:
  • Model.from_net and Model.load raise ModelError, and parse_net_file raises ValueError, as for any refused file.
  • net_to_sbml now raises ModelError on such a file where it returned a report.
• A stated limitation gets a docs note: the user guide names the rule, and the changelog says what still loads.
• Strict xfails and quarantines citing .net species indices are ignored: with '2 B() 5' above '1 A() 1', reaction '1 1 0 k' degrades B and Aobs reads 1.839 where run_network gives 0.368, with no error #1512: none.
• In-flight PRs: net: read a Hill law with 0 < h < 1 as 0 at zero substrate, as run_network does (#1497) #1629 and net: refuse a .net file from which no block is read (#1511) #1638 also change src/net_file_loader.cpp and docs/user-guide/loading-models.md, in other places. Trial merges with each open PR, and on top of net: read a Hill law with 0 < h < 1 as 0 at zero substrate, as run_network does (#1497) #1629 and net: refuse a .net file from which no block is read (#1511) #1638 merged together, are clean.
• Count bounds, both directions: step 9.
• Tolerances have their reasons (result: write the species a Result reports to .cdat, in the core's layout (#1505) #1635): the closed-form check's CLOSED_FORM = 1e-7 is named, with its measurement beside it: 3.0e-09 on every control and the round trip, where a species read on the wrong line is 400% off.
• Claims are measured: each number here comes from a run above.

The diff (abridged)

@@ src/net_file_loader.cpp — parse_species
         if (field >= tokens.size()) {
             throw std::runtime_error("species line '" + stripped +
                                      "' has an index but no species pattern");
         }
+        if (field == 1) {
+            const std::string position = std::to_string(species.size() + 1);
+            const auto first = tokens[0].find_first_not_of('0');
+            const std::string index =
+                first == std::string::npos ? std::string("0") : tokens[0].substr(first);
+            if (index != position)
+                throw std::runtime_error(
+                    "species line '" + stripped + "' has index " + tokens[0] + " on the " + block +
+                    " block's line " + position +
+                    ": reactions and groups name a species by its index, and BioNetGen's "
+                    "two readers place a species differently when its index is not its "
+                    "line (run_network at its index, BNG2.pl's readFile at its line). ...");
+        }

field == 1 means the line has an index; an unindexed line skips the check. species.size() + 1 is the line's 1-based number in the block, since every species line is appended. The leading zeros are stripped, so the comparison is of numbers, and a line of all zeros reads as 0.

@@ docs/user-guide/loading-models.md — parse_net_file
+A species line is `[<index>] <species> [<concentration>]`. The index may be
+left off, and when it is there it must be the line's own number, 1, 2, 3, ...
+in the order the species are listed (issue #1512). Reactions and groups name a
+species by that number, and BioNetGen's two readers disagree on a file whose
+index is not its line: run_network places the species at its index, BNG2.pl's
+`readFile` at its line. `parse_net_file` and `Model.from_net` refuse such a
+file rather than pick one reading.

Tests

• python/tests/test_net_species_index_is_its_line.py (new, 18 cases):
  • Refused:
    • the issue's file, with the message checked;
    • every route (parse_net_file, Model.load, net_to_sbml);
    • a gap, a duplicate, 0-based, an unindexed line then 1, three species permuted, and an index past 2^32;
    • an offset block;
    • a seed species block.
  • Read as before: in order, unindexed, an unindexed line then 2, 1 then an unindexed line, leading zeros (two ways), and a tab after the index, each to e^-1; and a write_net round trip.

[elyfmiller]

Montana, thanks. The fix reproduces here. On the PR head (98eced0, arm64 with MIR), the new file's 18 cases pass; on its base (0b2055a), 10 fail and the 8 controls pass, as the body says, and v0.17.1 fails the same 10. The branch merges cleanly with today's main (10c117b), which now holds #1638's changes to the same loader, and onto it with #1642, which also touches the loading guide.

What else we checked:

• A comment or a blank line between species, with run_network 3.0 and BNG2.pl 2.9.3's readFile:
  • 1 A() 1, # a note, blank, 2 B() 5 reads to 0.367879 under both and here: the index counts species, not the block's text lines.
  • 1 A() 1, # a note, 3 B() 5 is a gap past a comment. run_network crashes (SIGSEGV) and readFile gives 0.367879, and it's refused here, as your gap is.
• An expression initial concentration (1 A() 2*k-1, then 2 B() 5): the lifted line counts as species 1, and the file reads to 0.367879 under run_network and here.
• Every .net file in the repository (185), today's main against main with our commit merged: each loads to the same species, reaction count, parameters, frozen parameters and initial state, and parse_net_file gives the same dictionary.
• The full Python suite on today's main with our commit merged: 19,122 passed, 402 skipped, 1 xfailed and 1 failed. The one, test_run_tests_sh, wants optional extras this machine lacks (jax, diffrax, roadrunner, vivarium-core), and fails the same on v0.17.1. Our commit changes only the test file, and your CI on 98eced0, native-tests included, is green.

We've pushed one commit on top of yours, bb6f7cd, for item 1:

1. No case had a line between two species. Every block in the file holds species lines only, so nothing pinned what the index is counted against. A loader counting every non-empty line of the block, comments included, passes all 18 cases.
   • The new cases: the two files above. The first is a control, which loaded on the base too, and the second is refused. That mutant fails both.
   • Also: the seed-species test had a second replace that matched nothing, and it's gone.

Not changed:

2. "on the species block's line N" counts species, not the block's text lines. With a comment between, 3 B() 5 is refused as being "on the species block's line 2" although it is the block's third line. It reads right if "line" means species line, so it's a note.

3. The body's CI table is out of date. It says no Windows filter lists src/net_file_loader.cpp until net: read a Hill law with 0 < h < 1 as 0 at zero substrate, as run_network does (#1497) #1629 merges, but net: read a Hill law with 0 < h < 1 as 0 at zero substrate, as run_network does (#1497) #1629 merged on 11-11, and windows-tail ran on this head and passed.