Repository navigation
nfsim: refuse a seed past 2**32 - 1, which NFsim ran as its low word (#1515) - #1640
Merged
Merged
Conversation
…anl#1515) NFsim seeds its generators from the seed's low 32 bits, so seed=2**32 + 12345 returned the seed=12345 trajectory bit for bit while Result.seed and session.seed reported 4294979641. NfsimSimulator::run and initialize now refuse such a seed by name, before the XML is read and before initialize drops a live session. A seed below 0 or past 2**64 - 1 now raises a ValueError naming the range on every method and session, where pybind11 raised TypeError.
windows-nfsim.yml runs every test_nfsim*.py, so test_nfsim_seed_width.py's 9 cases took the leg to 395 passed, 17 above the floor of 378 where 16 is the slack. The floor goes to 387, 8 below the count, as its note says.
This was referenced Nov 11, 2026
…sed re-initialize keeps the session's place (lanl#1515) The SSA, PSA and RuleMonkey controls checked only Result.seed, which is what lanl#1515 got wrong: NFsim reported 2**32 + 12345 and ran 12345. A backend that masked its seed to 32 bits passed them. Each now runs 12345 and 2**32 + 12345 and checks the trajectories differ (A_tot 95 against 87 at t = 1 on SSA and PSA, X_p_total 296 against 262 on RuleMonkey). A mask on any of the three fails its control. The refused re-initialize was checked at t = 0, so a session put back to its start with its own seed passed. It is now refused halfway, and the second half must match a never-refused seed-7 session's. A reset fails it.
Collaborator
|
Montana, thanks. The fix reproduces here. On the PR head (afd709c, arm64 with MIR), the two new files' 30 cases pass; on its base (0b2055a), 19 fail and the 11 controls pass, as the body says, and v0.17.1 fails the same 19. The issue's script gives the issue's table on the base, and on the head What else we checked:
We've pushed one commit on top of yours, 6fd19f3, for items 1 and 2:
Not changed:
|
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.
Fixes #1515.
Important
NFsim seeds its generators from the seed's low 32 bits, so
seed=2**32 + 12345returned theseed=12345trajectory bit for bit, whileResult.seedandsession.seedreported 4294979641. Two runs that read as independent were one, and no seed check could see it. Such a seed is now refused by name, and seeds up to 2**32 - 1 run as before.The problem
On main (
0b2055a9), with the issue's script (X(s~u) <-> X(s~p), 300 molecules),Xpat t = 1..5:seed=12345seed=2**32 + 12345nfResult.seed= 4294979641rmssaNfsimSessiondid the same. Itsinitialize(seed=2**32)also replaced a live session with seed 0's and reported 4294967296.The cause is in the vendored source. Both generators NFsim draws from mask the seed to its low word, as reference MT19937's
init_genranddoes:NfsimRNG::seed(third_party/nfsim/src/NFutil/nfsim_rng.h) doesstate_[0] = s & 0xFFFFFFFFUL;MTRand_int32::seed(NFutil/MTrand/mtrand.cpp) does the same.SSA, PSA and RuleMonkey seed
std::mt19937_64with all 64 bits.Separately, a seed below 0 or past 2**64 - 1 raised pybind11's
TypeError: run(): incompatible function argumentson every method and session, though the docs said "any integer".The fix
Important
Of the issue's three options I took the first: refuse an NFsim seed past 232 - 1**. Every seed up to 2**32 - 1 keeps its trajectory bit for bit. numpy's legacy MT19937, the same algorithm, refuses such a seed. NFsim's own command line reads
-seedas anint, so no BNG2.pl parity case needs a larger one. Widening the seed would need a change to vendored code and to the process-wideMTRandthat #1438 proposes retiring.src/nfsim_simulator.cpp:require_nfsim_seedraisesstd::invalid_argument(aValueErrorin Python), naming the seed it would have run as: "NFsim seeds its generators from 32 bits, so seed 4294979641 would run as seed 12345: an NFsim seed must be from 0 to 4294967295 (2**32 - 1)".NfsimSimulator::runandNfsimSimulator::initialize, so the XML is not read for a seed that will be refused, andinitializedoes not drop a live session for one. Every NFsim entry point goes through these two:Simulator.runwithnf,nfsimornf_reject;NfsimSession.initialize;_bngsim_core.NfsimSimulatorbindings.python/bngsim/_seed.py:_check_seedrefuses a seed outside 0 to 264 - 1 with "seed must be an integer from 0 to 264 - 1, got -1"._resolve_seedcalls it, which covers every stochastic method,run_batch,run_replicates,parameter_scanand both sessions. The ODE path, which reads an explicit seed for equal-priority event ties (GH fix(sens): make the default sensitivity column set independent coordinates (#203) #242), calls it too.Docs:
docs/quickstart.md's seed contract gives the range, and says NFsim takes 0 to 2**32 - 1.Result.seed,Simulator.run,NfsimSession.initializeandRuleMonkeySession.initializedocstrings say the same.Simulator.run's docstring said the seed is "Ignored formethod="ode"". An ODE run with events uses it for tie-breaking (opts.event_seed), so that line now says so._seed.pydocstring said NFsim narrows a seed only on Windows; it narrows it everywhere.How this was checked
nfrefuses2**32,2**32 + 12345,2**63and2**64 - 1, each naming its low word.2**32 - 1runs and is its own trajectory, not seed 0's.initializeleaves the session at seed 7, and its nextsimulateequals a fresh seed-7 session's.tests/data/nfsim/, 85 models, at seeds 0, 1, 12345, 231 and 232 - 1: 425 of 425 runs give the sameResult.seedand the same trajectory digest on main and here.simple_system.xml, and of a seed-7 session, are bit-identical too.ode,ssa,nf,rmand both sessions raisedTypeErrorfor-1and2**64; here each raises theValueErrorabove. PSA takes the same path, pinned in the tests.test_nfsim_seed_width.py, 9 cases: 7 fail on main (the four seeds, the message, the session, the live session); 2 controls pass (2**32 - 1 is its own run; a 32-bit seed reproduces).test_seed_range.py, 21 cases: 12 fail on main (ode,ssaandpsaruns,run_replicates, and both sessions, each at-1and2**64); 9 controls pass (SSA and PSA keep all 64 bits at 0, 232, 263 and 2**64 - 1, and RuleMonkey runs2**32 + 12345).run()(5 failing);initialize()(2);initialize()checking after it drops the session (1);ctest, 8 of 8 pass.BNGSIM_SKIP_AUDIT=strict: 8,683 passed, 41 skipped, 1 xfailed;BNGSIM_CODEGEN_THRESHOLD=1): 8,676 passed, 48 skipped, 1 xfailed;0b2055a9, on the final tree: 19,393 passed, 140 skipped, 1 xfailed, 0 failed, so 19,534 collected, main's 19,504 plus these 30.PYTEST_COLLECTED_FLOOR(19,451), inside the 500 slack.PYTEST_SKIPPED_CEILING(252). The new tests add no skip on this build, which has NFsim and RuleMonkey.python:3.12,--platform linux/amd64, gcc 14, built from the final tree with the[antimony,hdf5]extras and scipy):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, net: refuse a .net file from which no block is read (#1511) #1638's and net: refuse a species index that is not its line (#1512) #1639's runs; none touches a seed.--all-filespasses (13 hooks, clang-format, ruff and mypy among them).ci/changelog.py checkpasses, and localsphinx-build -Wexits 0.Which CI legs this PR runs
python-tests.yml(Ubuntu, macOS)native-tests.ymlsrc/**)ctest, 8 of 8 locallywindows-nfsim.yml(MSVC)src/nfsim_simulator.cppand_nfsim_session.pypython/tests/test_nfsim*.py, sotest_nfsim_seed_width.pyruns under MSVCwindows-tail.yml,windows-klu.yml(MSVC)_simulator.pytest_seed_semantics.pyamong windows-tail's;test_seed_range.pyis not on themmir.yml_simulator.pyChecked against past review findings
_resolve_seed, which every stochastic method, batch, scan and session uses, and in the ODE path.run_untilwithnfraisesNotImplementedError("Interactive simulation helpers are not supported for XML-backed network-free backends"), so it does not reach NFsim. The issue thought it reached NFsim throughrun; it doesn't.ValueErrorwhere it ran.ValueErrorwhere pybind11 raisedTypeError. No test or caller in the repository catches thatTypeError.method="ode"" line and the "32 bits on Windows" line are corrected, beside the ones the issue names.initializewas the one path that could have lost state. It is pinned against a fresh session.windows-nfsim.ymlruns everytest_nfsim*.py, so the 9 new cases take it from 386 passed to 395, past its floor's slack (378 + 16).ci/test_floors.envraisesNFSIM_PASSED_FLOORto 387, 8 below the count, as its note asks (afd709c).The diff (abridged)
std::invalid_argumentreaches Python asValueError. It is thrown beforeprepare_system, whose catch would have wrapped it as "NFsim setup failed".Tests
python/tests/test_nfsim_seed_width.py(new, 9 cases; runs under MSVC throughwindows-nfsim.yml):initializeleaving the live session;python/tests/test_seed_range.py(new, 21 cases):-1and2**64refused onode,ssaandpsaruns,run_replicates, and both sessions;2**32 + 12345.