Skip to content

Latest commit

 

History

History
645 lines (524 loc) · 33.1 KB

CONTRIBUTING.md

File metadata and controls

645 lines (524 loc) · 33.1 KB

Contributing to bngsim

Thanks for your interest in improving bngsim. This file covers the essentials; the full development documentation has the project layout, extension guides, and CI details.

Certificate of origin

Contributions are accepted under the project's MIT license, the same terms bngsim is distributed under.

Please sign off on your commits, certifying that you wrote the contribution or otherwise have the right to submit it under that license — the Developer Certificate of Origin. Add -s when you commit:

git commit -s -m "your message"

which appends a Signed-off-by: line using the name and email from your git config.

Development setup

bngsim uses uv for environment and dependency management.

git clone https://github.com/lanl/bngsim.git
cd bngsim

# Create the project venv (.venv), build the C++ extension, and install bngsim
# editable with the test dependencies (pytest, scipy, antimony, jsonschema, …).
uv sync --extra test
# ...or `--extra dev` for the full toolchain (roadrunner, jax, pandas, ruff, …).

uv sync prunes — name every extra you want, every time. It makes the venv match exactly the extras you pass, uninstalling everything else. So running uv sync --extra test in a venv provisioned with --extra dev silently strips roadrunner, jax, vivarium and the rest. Pick the line you want and stay on it (--extra dev is a superset of --extra test). By the same rule, a package you hand-install with uv pip install disappears at the next sync unless it is declared in pyproject.toml — if you need it, declare it.

Requires CMake ≥ 3.20, a C++17 compiler, and Python ≥ 3.10 (uv can provision the interpreter). The build needs SuiteSparse/KLU: install it (brew install suite-sparse / apt-get install libsuitesparse-dev / conda install -c conda-forge suitesparse), or on macOS/Linux let the build compile a pinned KLU subset from source (needs bash, git and network access to GitHub).

The editable install does not auto-rebuild the C++ extension (editable.rebuild = false). After changing C++, refresh it:

uv sync --extra test --reinstall-package bngsim   # rebuild + reinstall the extension

The extras belong on that line — --reinstall-package does not exempt it from the prune rule above, so a bare uv sync --reinstall-package bngsim rebuilds the extension and strips every extra on the way (GH #229 is what that costs).

The faster path is an incremental cmake rebuild, which touches nothing else in the venv:

uv run --no-sync python scripts/rebuild_editable.py

It builds for whichever interpreter runs it, so run it with the venv's: a bare python may not exist, or may be a different Python. --no-sync keeps uv from syncing the venv, and possibly reinstalling bngsim, before the fast path starts.

That one needs pybind11 importable in the venv, which is why the dev extra declares it: pybind11 is a [build-system] requirement, so uv installs it into a transient isolated build env and never into .venv. On --extra test the script falls back to whatever pybind11 the system supplies, and tells you when it is doing that.

It also leaves a source-digest record beside the installed extension: the extension's SHA-256 and a digest of the C++ and CMake sources, written only when those sources did not change while it built. The stale-binary guard reads the record when a source file is newer than the extension, so switching branches, stashing, rebasing, or pulling a squash merge that rewrites the C++ with identical bytes no longer demands a rebuild; the import banner says fresh (digest) instead (issue #528). An extension built any other way leaves no record and is judged by timestamps alone.

One build directory per interpreter, not per configuration

build-dir = "build/{wheel_tag}" — the wheel tag is the Python version and the platform. It says nothing about the virtual environment being installed into, or about the options passed, so every build for one interpreter shares one CMake cache. A second install with different options rewrites the cache the first one is using (GH #459):

uv pip install --python /tmp/other/bin/python \
    --config-settings=cmake.define.BNGSIM_ENABLE_MIR=ON \
    --config-settings=cmake.define.BNGSIM_ENABLE_KLU=OFF .

Give a differently configured install its own tree and the two stay apart:

--config-settings=build-dir=/tmp/otherbuild/{wheel_tag}

rebuild_editable.py names every feature option on its configure line rather than inheriting any of them, and stops rather than building on a tree whose cache disagrees with what it asked for — the recovery it names is rm -rf on the tree plus a reinstall, because reconfiguring an already-built tree leaves link settings from the configuration it was built with. To change an option for one rebuild, set the same-named environment variable:

BNGSIM_ENABLE_MIR=1 uv run --no-sync python scripts/rebuild_editable.py

Re-locking after a pyproject.toml change

uv.lock records provides-extras, so any edit to this project's dependency metadata — adding an extra, moving a pin — invalidates the lock. CI's uv sync --extra dev then re-resolves instead of installing from it, and a re-resolve has to fetch metadata for every locked package, including the parity group's git-pinned PyBioNetGen. So the CI failure reads as a PyBioNetGen build error on a PR that never touched PyBioNetGen. Run uv lock and commit the result in the same PR.

PyBioNetGen is in no-build-isolation-package, which means it builds against your .venv rather than an isolated one — so that venv needs the two things its legacy setup.py assumes and does not declare:

uv pip install setuptools pip   # setup.py imports setuptools, then shells out
                                # to `pip install numpy`
uv lock                         # ~20 min: it downloads BNG2.pl at build time

Without them uv lock fails with ModuleNotFoundError: No module named 'setuptools', or with a CalledProcessError from pip install numpy — neither of which names the venv as the missing piece.

Do not run uv lock when you have not changed a dependency. Bumping the version for a release does not need it: edit the version = "..." line under [[package]] name = "bngsim" in uv.lock by hand, and confirm with uv lock --check, which exits 0. Running uv lock on an unchanged tree rewrites roughly 118 unrelated lines, dropping redundant python_full_version >= '3.11' markers from inside the amici package block. That churn is cosmetic — the amici gate itself survives, and both forms export an identical dependency set — but which form you get depends on your uv version, so the file ping-pongs between contributors for no reason. uv lock --check runs in CI and is tolerant of the difference, so it will not fail on either form.

Legacy BioNetGen (BNG2.pl) for parity_checks/

bngsim has no BNGL parser, so every BNGL job shells out to BNG2.pl for network generation before simulating in-process — the parity_checks/ engine tests, the cBNGL round-trip gate, and (since #162) Model.from_bngl itself. Without it those skip. Two ways to supply it — pick either:

uv sync --extra dev --group parity   # installs the pinned PyBioNetGen, which
                                     # bundles BNG2.pl + bin/run_network + bin/NFsim
export BNGPATH=/path/to/BioNetGen-2.9.3   # ...or point at an install you already have

Resolution order is $BNG2_PL → $BNGPATH → BNG2.pl on $PATH → PyBioNetGen's bundled copy, so an env var always overrides an installed package. $BNGPATH may be the BioNetGen folder or the BNG2.pl script itself. When nothing resolves, the skip message names every location that was tried — if a test says "no usable BNG2.pl", that message is the diagnosis, not a dead end. Its "Fix:" is the one for a wheel install, since Model.from_bngl shows users the same text; the test run's closing summary adds the checkout's fix, --group parity or $BNGPATH, because a pip install into this venv is gone at the next uv sync (issue #1692).

The resolver is bngsim._bngpath, in the shipped package since #162 because Model.from_bngl needs it; _core.bngpath re-exports it and adds the sys.exit-ing require_bng the sweep entrypoints use. Do not add a local BNG2.pl lookup anywhere — that module's header records the six near-duplicates that disagreed about precedence, and a seventh had appeared in bngsim.convert._bng2 before it was folded back in.

The parity group pins the same commit as parity_checks/requirements-pybionetgen.txt; test_pin_agreement.py fails if they drift. It is not the bngl extra: that extra wants the PyPI release for BNG2.pl, this group wants an exact commit for engine-routing provenance, and dev deliberately omits bngl so no environment has to reconcile a range with a git pin. For a fully isolated parity/benchmark environment (rather than adding PyBioNetGen to your dev venv), use parity_checks/bng_parity/bootstrap_parity_env.py instead.

AMICI for parity_checks/amici_parity/

AMICI is the second reference engine — the SBML/CVODES oracle amici_parity compares against, and the only independent check on forward sensitivities.

uv sync --extra dev --group amici    # builds the pinned AMICI (~1-4 min)

It is a group rather than an extra because the pin is a commit, and a direct reference in published metadata would make bngsim unpublishable — the same trade parity makes. The commit must match parity_checks/amici_parity/AMICI_PIN.json, which is the source of truth; test_amici_pin_agreement.py fails if they drift. Do not relax it to amici==1.0.1 off PyPI: the pin is 12 commits past that tag and they touch the engine, not just its tests.

Building it needs swig, a C++ toolchain, and a BLAS (macOS: Accelerate, nothing to install; Linux: the build fetches scipy-openblas64 itself).

If the clone fails with git-lfs: command not found, that is your machine, not AMICI. A git-lfs install that has since been removed leaves filter.lfs.* in ~/.gitconfig with required = true, and every clone of an LFS-using repo then aborts. Either reinstall git-lfs or drop those four keys (git config --global --remove-section filter.lfs).

Leaving AMICI undeclared is what used to break this: uv sync prunes anything not in the lock, so a hand-installed AMICI disappeared at the next sync and the whole suite went quiet. Worse for correctness than for coverage — with no second engine in the environment, a finite-difference oracle that happens to share a defect with the engine is the only oracle left, which is exactly how a 5x-wrong sensitivity column survived review (see CHANGELOG, issue #144 follow-up).

Running tests

uv run uses the project venv without needing source .venv/bin/activate:

uv run python -m pytest python/tests/ -q   # Python test suite

The C++ unit tests need a build configured with -DBNGSIM_BUILD_TESTS=ON; see the development docs.

Install the git hooks (ruff / clang-format / mypy on commit; the pytest suite on push, run via uv run) with:

uv run pre-commit install

The push hook runs the whole suite in parallel through pytest-xdist, with work stealing (ci/pre_push_pytest.py). That takes three to five minutes where a serial run takes about 45. It uses the main checkout's test cache from a worktree too, because a cold cache costs a worktree's first push about 15 minutes (see below). BNGSIM_PREPUSH_WORKERS=4 git push sets the worker count, and BNGSIM_PREPUSH_WORKERS=0 runs the suite serially in one process, for a failure that might depend on what ran before it. A venv synced before pytest-xdist joined the test extra stops the push with a message; uv sync --extra dev (plus any groups you use) brings it in.

A push skips the suite when every file it changes, deleted files included, is under changelog.d/ or is one of LICENSE, NOTICE, ACKNOWLEDGMENTS.md, CONTRIBUTORS.md, REPRODUCING.md, .readthedocs.yaml or .clang-format (SKIPPABLE in ci/pre_push_pytest.py), or when it changes no file at all (an empty commit to re-run CI). The suite never reads those files, and the changelog-fragments hook still checks the fragments. The hook then passes at once, without starting pytest. Anything else runs the suite, docs included, because it runs the examples in README.md and docs/. The script diffs the pushed commit against the one the remote already has, so a force-push that takes a change away counts that change, and python/tests/test_pre_push_pytest.py pins the list against every tracked file. It also runs the suite when the pushed commit is not the one checked out, since the checkout is what it tests, and when there is no range at all, as in pre-commit run pytest --hook-stage pre-push.

CI splits each Python leg into two jobs. conftest reads BNGSIM_TEST_SHARD=k/2 and keeps shard k's test files, so every collected test runs in exactly one job, and a third job per leg sums the two for the test-count guard. Files are balanced on ci/test_durations.json. When the two jobs of a leg drift apart, refresh it from a CI run's junit reports, which are in the python-tests-log-* artifacts: python3 ci/test_durations.py python-tests-*.xml. A stale table only unbalances the shards; it never drops a test.

The suite's artifact caches are its own

A pytest session redirects both of bngsim's content-addressed caches — compiled .so artifacts and BNG2.pl-generated networks — away from ~/.cache/bngsim and into a directory the suite owns (issue #372). It is .pytest_cache/d/bngsim/ by default, or a per-run temp directory when the cache provider is off (-p no:cacheprovider, which every CI leg passes). Both the module attribute and the env var are set, so subprocesses that import bngsim land there too.

The directory is persistent, so runs stay warm: a cold full suite measured 19m19s against roughly 14m warm. Two consequences worth knowing:

uv run python -m pytest python/tests/ --cache-clear   # force a cold-cache run
  • pytest --cache-clear wipes it, which is the supported way to test a cold compile path across the whole suite. It clears this checkout's own directory; the pre-push hook in a worktree uses the main checkout's (below);
  • BNGSIM_TEST_CACHE_DIR=/path relocates it — node-local scratch, a cache CI restores between runs, or a throwaway directory. It overrides BNGSIM_CODEGEN_CACHE_DIR / BNGSIM_BNGL_CACHE_DIR for the session rather than deferring to them: exporting those points bngsim's real cache somewhere, and the suite has no more business writing there than in ~/.cache.
  • On macOS a cold cache costs more than its compiles. The system checks each newly written .dylib the first time anything loads it, at 0.5-2.4 s a file and one file at a time across the machine, however many workers ask; a file it has checked once loads instantly from any process, and a copy at another path is checked again. A worktree's own .pytest_cache starts empty, so the pre-push hook sets BNGSIM_TEST_CACHE_DIR to the main checkout's .pytest_cache/d/bngsim unless you already have. A worktree's first push took 19m15s on a cold cache.

Being persistent, it still grows — every _CODEGEN_CACHE_KEY change orphans what came before it. The difference is that it grows somewhere disposable, under a directory git clean -xdf and --cache-clear both remove, and that bngsim-cache info -C .pytest_cache/d/bngsim/codegen will tell you how much of it your last emitter edit stranded.

A test that needs true isolation — a cold compile, or a cache whose entries it counts — still monkeypatches _codegen.CACHE_DIR (or _bngl_loader.CACHE_DIR) to a tmp_path, which overrides the session value and restores it afterwards. Do that in particular when the test invents a key: a fabricated _CODEGEN_CACHE_KEY or model hash produces an artifact that is orphaned the moment it is written, and it must not be written where anything else will see it.

Checking performance and parity

Three tools watch for regressions that unit tests cannot see.

benchmarks/perf_ab.py: did my change slow bngsim down? A wall-clock A/B of two builds on your machine. It interleaves the committed ODE networks in benchmarks/_dev/suite_ode.json (2 to 3,744 species) across the two sides and flags a model only when the slowdown is past 5 %, outside a 99 % bootstrap interval and at least 2 ms:

uv run python benchmarks/perf_ab.py                                # origin/main vs HEAD
uv run python benchmarks/perf_ab.py --candidate python:.venv/bin/python   # uncommitted work
uv run python benchmarks/perf_ab.py --quick                        # models up to 150 species

A git-ref side is built once into a wheel and cached under ~/.cache/bngsim/perf-ab. Leave the machine idle while it runs. It exits 1 if anything is flagged, and summary.md also shows whether the solver's work counters changed: a slowdown with identical counters is a constant-factor change. Run it for any change to src/, include/, the code generator, the loaders or Simulator, and paste the summary into the PR.

The nightly parity workflow (.github/workflows/nightly-parity.yml) runs bngsim against RoadRunner, BNG2.pl/run_network/NFsim, AMICI forward sensitivities, and the SBML and DSMTS test suites every night. It compares each model's verdict and bngsim's solver work with committed baselines and with the previous night, and opens or updates the issue Nightly parity: regressions detected when something regresses. It never blocks a PR. When a fix turns failing models good, re-baseline so that a later regression of the fix alerts: see parity_checks/nightly/README.md.

ci/sbml_suite_gate.py: does bngsim still give the SBML Test Suite's answers? This one does block a PR. Every leg of python-tests.yml runs the pinned suite's 1,823 semantic cases (about 5 s locally), whose expected values are the suite's own, and fails if any case's outcome differs from parity_checks/nightly/baselines/sbml_semantic.json. A case that regressed, a refusal that became a wrong answer, or a case the harness could not score (harness_error) is for the PR to fix. An improvement is for the PR to record, so that a later change that undoes it fails:

uv run --no-sync python harness/sbml_test_suite/fetch_semantic_suite.py --sparse --dest ~/Code/sbml-test-suite  # once
uv run --no-sync python ci/sbml_suite_gate.py                    # what CI checks
uv run --no-sync python ci/sbml_suite_gate.py --update-baseline  # record, then commit the file

Changing generated code

Compiled codegen artifacts are cached under ~/.cache/bngsim/codegen and keyed by model content plus _CODEGEN_CACHE_KEY — not by the generated C, because hashing the C would mean regenerating it on every cache probe.

_CODEGEN_CACHE_KEY is _CODEGEN_VERSION (a hand-maintained constant in python/bngsim/_codegen.py) plus a digest of the source of every module that determines the emitted C. _CODEGEN_SOURCE_MODULES, in that same file, is that list — read it there rather than from a copy here. This paragraph used to restate the names and went stale the first time one was added, so from #68 until issue #267 it told you to bump the version by hand for an edit the digest had covered all along. The digest means an edit to any module on that list invalidates stale artifacts on its own, so a change cannot go silently inert on a machine with a warm cache the way issues #41 and #43 did — there, the fixes appeared to do nothing at all, reporting dy/dp == 0 where a cold cache gave correct values.

Two cases the digest does not cover, where you must bump _CODEGEN_VERSION yourself and say why in the comment block above it:

  • a C++ change that alters codegen_data() and therefore the emitted source;
  • any time you want to deliberately invalidate a release's caches.

The key is also carried in the artifact's name — rhs_<key>_<hash><suffix>, built by _artifact_stem() (issue #363) — so bngsim-cache info can report how much of a cache your edit just orphaned and bngsim-cache prune --orphaned can sweep exactly that. Build every artifact path through that helper: a call site that spells the name itself is a silent cache miss, not an error.

When a value cannot be computed

Never substitute a number that could have been a real answer (issue #580). A zero rate constant, a zero concentration and a zero output column are all ordinary values, so a 0.0 standing in for "could not compute this" runs to completion and returns a plausible, wrong result that nothing flags. In order of preference:

  1. Raise. For a model that reads a value it never defines, that is UnderSpecifiedModelError, which names the symbol and the BNGSIM_ALLOW_UNSET_PARAMS=1 opt-out (SBML global and local parameters, internal#94, #323, #580). The .net loader refuses an expression it cannot compile and an initial concentration that names no parameter (#602, #571).
  2. Return something the caller cannot mistake for a value: None, std::optional, or a placeholder paired with a flag that the reader of the flag replaces, like the .net loader's is_expression.
  3. Warn, at minimum, and only when refusing would turn away models a reference engine runs with the same default. That is why a non-finite compartment size (#353), a species with no initial value and a compartment with no size (#580) warn instead of refusing: RoadRunner runs all three, and corpus models depend on it. Say what was substituted and for which names, in one message per load rather than one per symbol, and only where the substitute can reach a number.

The shapes to look for in review are except Exception: x = 0.0, d.get(name, 0.0) for a name that ought to be there, catch (...) { v = 0.0; }, and std::stod inside a catch-all, which also turns an overflowing 1e400 into 0.0 rather than inf. Test both sides of each check: that the refusal or the warning fires, and that a value which can be computed still loads, so the check cannot drift into refusing good models.

Fixes that need a capability key

Most fixes need no announcement beyond the changelog. A few do, and the test is narrow (issue #431): does a build without this fix return a wrong number instead of refusing? If so, add a key to capabilities()["features"].

The reason is that nothing else can carry the answer. The version string identifies a release cycle rather than a build — bngsim bumps __version__ at the start of one, so a from-source install made before your fix declares the number of the release that carries it — and a hasattr probe cannot see a change in what a build computes rather than in what it exposes. So a consumer deciding whether to trust the result has nothing to read, and its two ways of guessing wrong are not equally bad: guessing "absent" costs a slower path, guessing "present" costs a plausible wrong answer nobody looks at twice.

When you add one:

  • Give it a real probe where the fix can actually be missing — a binding the fix added, for a fix that is partly C++, since a source checkout builds the extension separately and it can lag the Python layer.
  • Publish it on every build, True or False. An absent key means "too old to have been asked", which a consumer must handle as a third case.
  • Give missing[name] a sentence that says what goes wrong and what to do.
  • Measure what the key claims in python/tests/test_behaviour_capability_keys.py and assert the key against the measurement, so the key cannot outlive the behaviour.
  • Names are permanent: capabilities() promises that existing keys are never renamed or removed.

Fixing bugs

These rules apply to every bug-fix PR, whether a person or an agent writes it (issue #960). They exist because, for a stretch, most new issues came from reviews of our own fixes, and each fix opened more issues than it closed.

Fix review findings in the PR; do not file them

A finding from a review of your PR is fixed in that PR, even outside the issue's original scope, when it is in code the PR touches or behaviour it changes, or is the same kind of bug in a neighbouring path. Filing it instead turns one unit of work into several, each with its own branch, review, CI run and merge, and loses the context the reviewer had.

  • Rerun the review until it comes back clean.
  • Turn each reviewer probe into a test.
  • List what was found and fixed under a "Review" heading in the PR body.
  • Reviewers report findings to the author, not as new issues.

File a new issue only for a finding that is a design question needing the maintainer's decision, large enough to be its own project (days, not hours), or unrelated to the PR (a different subsystem, found by accident). Add it to an existing tracking issue when one fits. A pre-existing bug that is merely nearby is not a reason to file. Fix it if it is small. Otherwise list it under a "Left alone" heading in the PR body, with its repro; the family's owner picks it up from there.

Forward sensitivities: compute inside the envelope, refuse outside it

Switches, events, windows, kinks, clocks and their interplay form an unbounded space of corner cases, and an adversarial reviewer will always find one more. So forward sensitivity through a discontinuity is computed only inside an envelope where it is known to be right, and refused loudly outside it. This is the rule of When a value cannot be computed applied to derivatives: a refusal is an acceptable outcome, a silently wrong derivative is not.

The exact envelope is for the sensitivity family's owner to write, and it is drawn deliberately narrowly. As a starting point, inside means:

  • Isolated transversal crossings. A switch or event crossing that is isolated in time, from other crossings and from events, and transversal: the trajectory crosses at nonzero speed. Anything within solver tolerance of another crossing or event is not isolated.
  • Single fires. One event fire per instant, or a same-instant batch whose fires read nothing an earlier fire wrote.
  • Plain thresholds. A threshold linear in the parameters, a clock that is time itself, and a window whose edges are each such a crossing.

Outside means:

  • coincident or near-coincident crossings;
  • sliding modes and chattering;
  • a crossing whose threshold moves at a power or another nonlinear rate, where that changes the sensitivity;
  • same-instant compositions that read earlier writes;
  • delays.

Each refusal names its reason and the issue that tracks it, and is raised at analysis time where the shape is static and at run time otherwise.

This changes how sensitivity issues and reviews are judged:

  • An issue whose case falls outside the envelope is closed by the PR that refuses it, not by a bespoke fix. One whose case falls inside is fixed.
  • Do not chase edge cases outside the envelope. A new one found in review gets the refusal, not a fix.
  • In review, a wrong derivative inside the envelope is a bug in that PR. So is a wrong derivative outside it that is not refused: the refusal is missing. A correct refusal is not a finding.
  • A refusal of a case inside the envelope is not wrong and is not a finding against the PR, but it is a refusal to lift once the silently wrong results are cleared. #961 tracks every refusal where a derivative exists, these among them, each with what is refused, why the code cannot tell it from the unsafe case, and, where known, what lifting it would take. Add a new one there.

Triage before fixing

Before starting on a family of issues, close the duplicates, each pointing at the issue it duplicates, and the ones already fixed, each pointing at what fixed it. Keep silently wrong results apart from performance, test-infrastructure and enhancement items, which do not block the correctness backlog.

Credit

Everyone whose code is merged is listed in CONTRIBUTORS.md, by name with GitHub handle — Ada Lovelace (@alovelace). You are added on the pull request that brings your first contribution, by you or by a maintainer, and we make a best effort to get the name right from what you have already made public: the name on your commits, your GitHub profile, how you signed the pull request.

Best effort is not certainty, and it is your name. If you would rather be listed differently — another form of your name, handle only, a different handle, or not at all — say so on your pull request or open an issue, and it will be changed.

The contributors listed check reminds every pull request of this. It reads the pull request's commits from GitHub, which has already matched each commit's email to an account, and fails if an author's handle is not in the file. Where GitHub matched the email to no account, the name on the commit must be a name or handle in the file instead. A handle-only entry is written - @alovelace. The fix for a red run is the missing line, on the same pull request. The check is not a required one: where a maintainer cannot push to a contributor's fork, they can merge and add the line straight after. GitHub returns at most 250 of a pull request's commits, so a longer one is checked only that far, with a warning. python3 ci/contributors.py check --all checks every commit on main (it needs gh), but not Co-authored-by trailers, which is where the other authors of a squash-merged pull request end up. Someone who asked not to be listed at all would turn the check red; teach ci/contributors.py about them rather than listing them.

Claiming an issue

Before you start on an issue, comment on it to say you are working on it and name your branch, so that two people do not fix the same thing. If you drop it, say so on the issue. A claim with no linked PR and no update for about two weeks lapses, and anyone may take the issue.

Before opening a PR

  • Keep new code consistent with the surrounding style (.clang-format for C++, ruff for Python).
  • Add or update tests for behavior changes.
  • For a change to src/, include/, the code generator, the loaders or Simulator, run benchmarks/perf_ab.py and put its summary in the PR (Checking performance and parity).
  • Stage a changelog fragment (below) and, where user-facing, update the relevant page under docs/.
  • Say how the PR was reviewed, under the "Review" heading the PR template gives you (Saying how a PR was reviewed).
  • Begin the PR's title with the part of bngsim it changes, then say what changes, and end with every issue it clears: simulator: read each batch row's default atol from its own start (#1660). Leave the numbers out only when no issue is involved.

See the development docs for building and testing, and for the guide to adding built-in functions. scripts/LOCAL_CI.md covers building the wheel matrix locally.

Saying how a PR was reviewed

Every PR says how it was reviewed, under a "Review" heading, so that whoever merges it knows what has been checked. A review is independent when the reviewer did not write the change and read the whole branch. Name the reviewer in one of three forms:

  • A person: their name and handle. A careful read by a maintainer is enough for a small change to documentation.
  • An AI model: the model, the effort level, how many rounds the review took, which round came back clean, and any commits made after the last round, with what they change.
  • Not reviewed: say so. A maintainer then runs /code-review before the PR merges.

For a reviewed PR, list what each round found and how it was fixed (see Fix review findings in the PR).

Changelog entries

Do not edit CHANGELOG.md. Write one file instead:

cat > changelog.d/659.fixed.md <<'EOF'
- **One sentence saying what was wrong and for whom (issue #659).** Then two or
  three saying what the fix changes and what a reader should do differently.
EOF
python3 ci/changelog.py check

The name is <issue>.<category>.md, with <category> one of added, changed, deprecated, removed, fixed, security. The file holds the bullet exactly as it will read in the changelog, leading - and two-space continuation indent included. Keep it under 1,200 characters — the evidence and the reasoning belong in the issue and the commit message, where no limit constrains them and where a reader who wants them will look.

The reason is that CHANGELOG.md has exactly one anchor a new entry can go at, so any two open branches edit the same region of it and git stops on a conflict (issue #668). A fragment has no shared anchor; the release commit assembles them. CI enforces both halves — a branch touching python/bngsim/, src/ or include/ must stage a fragment, and no branch may edit CHANGELOG.md. Label a pull request changelog exempt if its change is genuinely invisible to users.

changelog.d/README.md has the rest: multiple entries per issue, entries with no issue number, and what the release step does.