Test reliability, mypy, CI modernization, and automated releases - #834
Open
schneiderfelipe wants to merge 46 commits into
Open
Test reliability, mypy, CI modernization, and automated releases#834schneiderfelipe wants to merge 46 commits into
schneiderfelipe wants to merge 46 commits into
Conversation
…ment - overreact/thermo/__init__.py: import `_solv`'s public functions (`calc_cav_entropy`, `molar_free_volume`) directly by name, the same way `_gas`'s public functions already are, and call them unqualified instead of via `rx.thermo._solv.X(...)`. Previously nothing in this module imported `_solv` by name, so `rx.thermo._solv` only resolved by accident, because pytest's --doctest-modules addopt happened to import overreact/thermo/_solv.py as a side effect when the whole suite ran. Running tests/test_thermo_solv.py in isolation raised AttributeError: module 'overreact.thermo' has no attribute '_solv'. (`_gas`'s private `_sackur_tetrode` helper is left as-is, still reached via `rx.thermo._gas._sackur_tetrode(...)` — that already worked and isn't part of this fix.) - tests/test_thermo_solv.py: loosen the free_volume tolerance in test_translational_entropy_liquid_phase from rel=3e-2 to rel=3.2e-2. That assertion compares a difference of two close cube roots of unseeded Quasi-Monte Carlo volume estimates (see coords.get_molecular_volume), which amplifies sampling noise and occasionally pushed the result outside the old tolerance (observed: 0.1691 vs. 0.164 ± 0.00492). - CONTRIBUTING.md, .github/workflows/python-package.yml, overreact/simulate.py: make it explicit that `uv sync --all-extras` (not plain `uv sync`) is required to run the test suite cleanly. Without the `fast` extra, JAX isn't installed, overreact falls back to NumPy, and doctests expecting `Array([...], ...)` (JAX's repr) see `array([...])` (NumPy's repr) instead and fail.
--recurse-submodules on clone was already documented, but there was no guidance for the common case of already having a clone without it (or data/ being empty). Point to `git submodule update --init --recursive`, and note that the test suite actually depends on this submodule (it's not just an unused extra).
mypy was a pinned dev dependency with no [tool.mypy] config and no CI step, so it silently bit-rotted. Actually running it (`uv run mypy overreact`) surfaced 20 errors: - Most were "missing library stubs" noise for scipy/thermo/seaborn, which don't ship a py.typed marker; silenced via per-module ignore_missing_imports overrides instead of import-guarding every call site. - The rest were real: see the overreact/core.py, overreact/simulate.py, overreact/api.py and overreact/io.py fixes. `uv run mypy overreact` is now clean (this is scoped to the overreact/ package, not tests/, which still has ~45 findings of its own around numeric duck-typing that haven't been triaged). Also drop `perflint` from dev dependencies: it was never invoked anywhere (no CI step, and it needs pylint to run, which also wasn't a dependency), and ruff's `select = ["ALL"]` already includes ruff's native PERF rule set -- ruff's own reimplementation of perflint's checks. Removing it also dropped its whole transitive chain (pylint, astroid, dill, isort, mccabe, tomlkit) from `uv sync --all-extras`.
…nstall Hooks are `local`/`system` and shell out to `uv run <tool>` rather than pinning their own `rev:`, so they always run the exact tool versions locked in uv.lock -- the same ones CI runs -- with no separate version to keep in sync (and nothing extra for Dependabot to track). Not installed automatically on clone (git doesn't support that); document `uv run pre-commit install` as a one-time step per clone in CONTRIBUTING.md.
- Split Ruff check/format out of the `build` job's Python-version matrix into a new single `lint` job. They were running twice (once per matrix entry) for no benefit: ruff's output only depends on tool version and the target-version pinned in pyproject.toml, not on which interpreter runs it. - Add a `Type check with mypy` step to the same job, now that `uv run mypy overreact` is clean (see previous commit). - Add a `concurrency` group with `cancel-in-progress: true` so pushing a new commit cancels the previous, now-superseded run on the same branch/PR instead of letting it run to completion. - Add an explicit `permissions: contents: read` block instead of relying on the default (broader) GITHUB_TOKEN scope.
Preparing for mypy to also cover tests/ (next commit) surfaced a few
annotations in overreact/ that were narrower than the documented/tested
contract:
- rates.eyring and tunnel.wigner accept "array-like" temperature/
delta_freeenergy per their own docstrings (and are called with plain
lists in tests), but were typed float | np.ndarray, which plain lists
don't satisfy. Broaden to float | npt.ArrayLike.
- tunnel.wigner's return type was `-> float`, but it returns an array
when given array-like temperature (same as its sibling tunnel.eckart,
already typed float | np.ndarray); fix the return annotation to match.
- api.get_k was typed `-> float`, but its docstring says "array-like" and
it always returns one rate constant per reaction; fix to
float | np.ndarray.
- api.get_k's `tunneling` parameter is documented and doctested as
accepting None (`tunneling=None` turns tunneling off), but was typed
plain `str`; fix to `str | None`. Also rewrite the internal
`tunneling not in {"none", None}` guard as
`tunneling is not None and tunneling != "none"`, which is equivalent
but (unlike the set-membership check) lets mypy actually narrow
`tunneling` to `str` before it's passed to get_kappa.
No behavior changes; `uv run pytest` still passes (683 passed).
Running mypy over tests/ (previously untested, see CI/pre-commit commits that follow) surfaced ~45 findings, all pre-existing typing issues rather than bugs. Fixed as cleanly as possible, grouped by root cause: - overreact.core.Scheme.compounds/reactions were already fixed to tuple[str, ...] in an earlier commit; is_half_equilibrium had the same problem (always constructed as a tuple via totuple()) and gets the same fix here. Updated the handful of tests that construct Scheme directly to pass tuples instead of lists/ndarrays, matching what parse_reactions() actually produces. - test_api.py/test_regressions.py: `for qrrho in [True, False, (False, True)]` was inferred as list[object] (mypy couldn't find a common type across the mixed bool/tuple literals); give it an explicit `list[bool | tuple[bool, bool]]` annotation instead. - test_regressions.py: several `x = []` accumulators were later reassigned `x = np.asarray(x)`, which mypy rejects (the variable's type is locked to list[Any] from the first assignment). Renamed the accumulator to `x_list` and kept the final ndarray as `x`, which is both mypy-clean and arguably more readable. Along the way, dropped a wigner-tunneling k_wig computation in test_rate_constants_for_tanaka1996 that was already dead (computed, but never asserted against) -- this only became visible once the self-referencing `k_wig = np.asarray(k_wig)` pattern that had been masking it from ruff's F841 was gone. - Several call sites index/reassign the result of get_k/wigner/eckart, whose honest return type is `float | np.ndarray` (they return a scalar or an array depending on whether inputs are scalar or array-like). mypy can't know from a general function signature that a *specific* call site's inputs make it array-valued; used `typing.cast(np.ndarray, ...)` at exactly those call sites to say so explicitly, rather than lying in the general signature or scattering `# type: ignore`. - A couple of unrelated reassignment conflicts (temperatures: list[float] reassigned to an ndarray; degeneracy: ndarray reassigned to an int) in long, multi-section test functions that reuse the same local names across independent sub-tests; gave the first assignment in each an explicit Union annotation covering every later reassignment in that function. No behavior changes; `uv run pytest` still passes (683 passed).
Now that mypy overreact tests is clean (previous two commits), extend both the CI mypy step and the pre-commit mypy hook to cover tests/, so the two stay in sync (same command, same file scope) instead of the local hook silently checking less than what CI enforces.
Both sections were already fully commented out (#Pipfile.lock, template that never actually ignored anything, for package managers this project has never used. This is a strict no-op for what git actually ignores. Left the pdm and PEP 582 sections alone: unlike pipenv/poetry, they each have one live, uncommented ignore rule (.pdm.toml, __pypackages__/), and removing an active rule -- even for an unused tool -- would make the ignore file less strict, not just tidier.
- Python is interpreted, so CodeQL doesn't need a build step at all; Autobuild's own comments say it targets compiled languages (C/C++, C#, Java). Set build-mode: none on the Initialize CodeQL step (GitHub's current recommended setting for interpreted languages) and drop the separate Autobuild step, which was a guaranteed no-op here. - Add the same concurrency/cancel-in-progress group as python-package.yml, so a new push cancels a superseded scan instead of letting it run to completion. - Add paths-ignore for **.md and docs/** on push/pull_request (the scheduled weekly scan is untouched, so drift is still caught even without a code change): CodeQL only has Python to analyze, so a documentation-only diff can't change its findings.
Add .github/workflows/publish.yml: pushing a vX.Y.Z tag builds the package, re-runs the full lint/type/test suite against that exact commit, publishes to PyPI, and creates/updates the matching GitHub release with the built sdist/wheel attached. Safety/professional touches: - PyPI Trusted Publishing (OIDC) via pypa/gh-action-pypi-publish -- no long-lived PyPI API token stored as a repository secret. Sigstore build provenance attestations are generated automatically as part of that. - A dedicated check step fails the run if the pushed tag doesn't match `version` in pyproject.toml, instead of silently publishing whatever version happens to be there. - build and publish are separate jobs (build produces an artifact, publish only downloads it) so the OIDC-credentialed publish step runs with as little else in scope as possible, per PyPA's own trusted publishing guidance. - publish uses a `pypi` GitHub Environment, which can optionally be configured with required reviewers for a manual approval gate, without editing this workflow. - The build job is guarded to the canonical repository, so a fork pushing a matching tag doesn't burn CI trying (and failing) to publish under this identity. Also document the release process and the one-time PyPI-side Trusted Publisher setup a project owner needs to do before the first release under this workflow (CONTRIBUTING.md). This does NOT publish anything by itself -- the PyPI-side Trusted Publisher configuration is a manual, external step for whoever owns the `overreact` PyPI project (see CONTRIBUTING.md "Releasing"). Until that's done, a tag push will just fail at the publish step with an auth error.
…et_k
Code review on the branch caught it: get_kappa's docstring and doctest
both exercise `method=None` (`get_kappa(..., method=None)`, line ~702) --
the exact same "None turns a feature off" contract get_k.tunneling was
just fixed to declare -- but its signature still said plain `str`.
Fixing get_kappa's own annotation (str | None) means get_k no longer
needs the narrowing-friendly `tunneling is not None and tunneling !=
"none"` rewrite from the previous commit just to satisfy mypy at the
`get_kappa(method=tunneling, ...)` call site: str | None is now valid
input on both ends, so the guard is reverted to the more idiomatic
`tunneling not in {"none", None}` -- which also now matches the
identical check already used inside get_kappa itself.
numpy.typing.ArrayLike already includes plain scalars (float, int, bool, complex, ...), so `float | npt.ArrayLike` on rates.eyring's delta_freeenergy/temperature and tunnel.wigner's temperature said the same thing twice. Flagged in code review; `npt.ArrayLike` alone is equivalent and matches how it'd normally be spelled. (tunnel.eckart nearby still uses float | np.ndarray, a narrower and pre-existing annotation from before this branch; unifying the two conventions across overreact/tunnel.py is a separate, slightly bigger cleanup left for another time.)
qrrho_options doesn't depend on bias or environment, so it was being rebuilt on every one of the 6 (3 bias x 2 environment) inner-loop iterations for no reason. Flagged in code review. (A similar-looking list exists in test_regressions.py, but it's a top-level loop, not nested inside another one -- nothing to hoist there.)
Code review on the branch caught three issues in the two-workflow setup from previous commits: 1. publish.yml's PyPI environment URL used the raw git tag (github.ref_name, e.g. "v1.2.0") instead of the stripped package version, producing a broken link (pypi.org has no /overreact/v1.2.0/ page; the real one is /overreact/1.2.0/). The version-check step already computed the correct value and threw it away; now it's exposed as a job output and reused for the URL. 2. publish.yml's re-verification of the tagged commit only ran the lint/type/test steps against Python 3.12, unlike python-package.yml's full 3.11/3.12 test matrix -- despite claiming to run "the full battery of checks" -- so a 3.11-only regression could slip through and get published. 3. Both workflows hand-duplicated the same lint/format/mypy/test steps, free to drift out of sync with each other over time. Fixes all three at once: extract the lint + matrixed test jobs into checks.yml (workflow_call), and have both python-package.yml and publish.yml invoke it instead of maintaining their own copies. publish.yml now gets the identical 3.11/3.12 matrix python-package.yml runs on every push/PR, for free, with no way for the two to disagree again. Also add the same paths-ignore: ["**.md", "docs/**"] to python-package.yml's push/pull_request triggers that codeql-analysis.yml already had, for the same reason: a documentation-only change can't affect lint/type/test results, so there's nothing for either workflow to usefully check.
Following up on your ask to look for @overload opportunities that avoid most cast() uses: wigner and eckart are the simplest possible case for it. Both only have one parameter that can be array-like (temperature; vibfreq/delta_forward/delta_backward are always scalar), so mypy can correctly infer float vs np.ndarray from the type of *that one argument* at each call site -- no cast() needed by callers anymore. Also finishes unifying eckart's temperature annotation with wigner's (npt.ArrayLike instead of float | np.ndarray), noted as a follow-up in an earlier commit.
…st() eyring never had a return annotation at all, so mypy treated its result as Any and never checked anything done with it. Overloading it (split on delta_freeenergy/temperature: both plain float -> float, either array-like -> np.ndarray -- mypy tries overloads in order, so "both must be float" naturally falls through the moment either one isn't) gives it a real return type for the first time. That's a real improvement on its own, but it also ripples into get_k, which calls eyring internally and then unconditionally slices/indexes the result (`k[i:i+2]`, `k[i] / k[i+1]`, ...) once per pair of half-equilibrium reactions. With eyring now precisely typed, mypy correctly flags that this only type-checks if k is array-like -- which, it turns out, isn't actually *guaranteed*: get_k also accepts an explicit scalar `delta_freeenergies` from the caller (used once, in tests/test_rates.py, to bypass the normal free-energy computation), and combining that with a scheme that has a half-equilibrium reaction would slice a bare float and crash. No caller does that today, so this isn't a live bug, but it's a real, pre-existing sharp edge that was invisible before eyring got a real type -- not something introduced by this commit. Documented in place with a comment and a single, deliberate `cast(np.ndarray, ...)` right there, rather than silently accepting whatever mypy would otherwise infer. get_k itself also gets two overloads, split the same way as get_kappa's `method` parameter: an explicit scalar `delta_freeenergies` (keyword- only, since it sits after several defaulted parameters) returns float; anything else -- array-like, or the default None that always triggers the array-producing computation above -- returns np.ndarray. Together with the previous commit's wigner/eckart overloads, this removes every cast() that tests/test_regressions.py needed to index into a get_k/wigner/eckart result -- all 7 of the original ones are gone. The one that's left (api.py, noted above) is deliberately internal to get_k's own implementation, not exposed to callers. `uv run pytest` still passes (683 passed); no behavior changes.
…s.py The 7 cast(np.ndarray, rx.get_k(...)/rx.tunnel.wigner(...)/ rx.tunnel.eckart(...)) calls added earlier this session were working around those three functions' honest-but-imprecise float | np.ndarray return types. Now that all three are overloaded (previous two commits), mypy infers np.ndarray on its own at every one of these call sites -- none of them pass an explicit scalar delta_freeenergies to get_k, so they all land on the array-returning overload -- and the casts are redundant.
The previous commit's eyring/get_k overloads had a "scalar in, scalar out" branch that, on closer inspection, never actually fires. thermo.equilibrium_constant -- called by rates.eyring, called internally by api.get_k -- does `np.atleast_1d(delta_freeenergy)` right in its own body. That's not incidental: it means equilibrium_constant, eyring, and get_k all *always* return at least a 1-D ndarray, one value per reaction, regardless of whether their inputs are scalar or array-like. Their own doctests already say so (`equilibrium_constant(dG)` -> `array([24.5])`, `eyring(17.26 * constants.kcal)` -> `array([1.38])`), and I confirmed it empirically against the one place in the codebase that calls get_k with an explicit scalar delta_freeenergies (tests/test_rates.py): its result is `array([1.38...])`, not a float -- the test only reads as a scalar comparison because comparing a 1-element array to pytest.approx(scalar) broadcasts fine. So the "float" overload branch was describing a code path that doesn't exist, and the "get_k might slice a bare scalar k and crash" risk the previous commit's cast()+comment guarded against isn't real either -- k can't be scalar there. Both were built on an assumption I hadn't actually verified against these functions' own doctests before writing the types. The fix is a simplification, not new machinery: drop the eyring/get_k overloads, type both plainly as `-> np.ndarray` (matching what they've always actually done), and delete the now-pointless internal cast(). equilibrium_constant (previously unannotated) and get_kappa (same np.asarray(...).flatten() pattern as get_k) get the same honest `-> np.ndarray` return type for the same reason. wigner/eckart are untouched: unlike this family, they don't use atleast_1d and really do preserve scalar-vs-array based on their arguments (confirmed empirically too), so their overloads from the previous commit remain correct. Net effect: every remaining cast() in the codebase is now gone (was 1, the one inside get_k this commit removes) -- not just "most" of them. `uv run pytest` still passes (683 passed); no behavior changes.
…gner/eckart Fixes a real inconsistency: tunnel.wigner and tunnel.eckart already follow normal numpy convention (scalar in -> scalar out, array-like in -> array out), but thermo.equilibrium_constant forced array output unconditionally via an explicit `np.atleast_1d(delta_freeenergy)` in its own body -- so did rates.eyring, which calls it internally. A user moving from wigner(1218.0) (gets a plain number) to eyring(72200.0) (got array([1.39...])) hit a surprise with no principled reason behind it. New contract, applied consistently: - Primitives (wigner, eckart, eyring, equilibrium_constant) are shape-preserving, matching numpy/scipy idiom and wigner/eckart's already-correct behavior. - Scheme-level API (get_k, get_kappa) always returns np.ndarray, because their unit of output is inherently "one value per reaction" -- unchanged from before, just relocated: get_k now does its own np.atleast_1d right after calling rates.eyring, instead of relying on equilibrium_constant to force it deep inside the call chain. equilibrium_constant and eyring both get proper @overload pairs (same pattern as wigner/eckart already use): all relevant arguments plain float -> float, any array-like -> np.ndarray. Every scalar-input doctest that printed `array([...])` for these two functions is updated to match (e.g. `eyring(17.26 * constants.kcal)` now prints `1.38`, not `array([1.38])`), wrapped in float(...) matching the convention wigner/eckart's own doctests already use. One of these (`eyring(dG - 1.4 * constants.kcal) / eyring(dG)`) needed care: pytest's NUMBER doctest flag reads tolerance from the *shown* decimal precision, so `10.` (zero fraction digits -> tolerance +-1) and `10.0` (one digit -> tolerance +-0.1) are not interchangeable even though they look equivalent to a human -- the true value is ~10.62, so only the former, matching the original doctest's precision, actually passes. `uv run pytest` still passes (683 passed, same count as before); no behavior changes to get_k/get_kappa's own return values -- only to equilibrium_constant/eyring called directly, which is the whole point.
…r repr float(...) wrapping in doctests exists purely to dodge numpy's version- dependent scalar repr: numpy >=2.0 (NEP 51) shows np.float64(1.38) instead of the old bare 1.38, and this project doesn't pin numpy's version, so a doctest showing the raw repr would pass or fail depending on which numpy a contributor happens to have installed -- the same class of fragility as the JAX-vs-NumPy repr issue this branch's very first commit fixed in simulate.py. float() works, but casting the return value to a different type just to get a stable repr reads oddly in an example meant to show what the function actually returns, and can't handle multi-value results at all. print(...) solves the same problem more directly: numpy only changed __repr__, not __str__, so print(x) already shows the plain number for np.float64/0-d-array/Python float alike (verified empirically for all three), with no cast involved. It's also not a new pattern here -- print(...) was already used elsewhere in this codebase's doctests (print(scheme), print(_unparse_model(model))) for output-formatting reasons. Swept every `>>> float(EXPR)` doctest across the whole overreact/ package (112 of 115 occurrences) to `>>> print(EXPR)`, verified individually to be a standalone display of a single scalar-like value (no trailing content after the call, comments aside) via a small paren-balancing script rather than a blind regex/sed replace, since a handful of superficially similar lines are NOT simple wraps and would have been silently corrupted by one: - 11 in coords.py, `tuple(float(x) for x in EXPR)`: genuinely still necessary. print() only fixes the outermost str()/repr() call; Python tuples always repr() their *elements* regardless, so print((np.float64(1.2),)) still shows `(np.float64(1.2),)`. Left as-is. - 3 more (_misc.py x2, simulate.py x1): float(a), other(b) tuples of two independently-cast values on one line, not a single float(...) call wrapping the whole displayed expression. Left as-is for the same reason. Every one of the 112 replacements was verified by actually running `pytest --doctest-modules overreact/` (95 passed) rather than assumed correct from the pattern match -- output text needed zero changes in every case, confirming str() and float()'s repr agree for all of them. `uv run pytest` still passes (683 passed, same count as before); no behavior changes, pure documentation/doctest-formatting cleanup.
Final pre-PR code review caught it: the earlier flakiness fix (3e-2 -> 3.2e-2) only touched the one assertion whose failure I'd actually observed, but tests/test_thermo_solv.py has five essentially identical `molar_free_volume(..., method="izato")` assertions across different test functions/molecules, all subject to the exact same root cause (Quasi-Monte Carlo sampling noise from coords.get_molecular_volume amplified by a difference of two close cube roots). Four of them were still sitting at the original 3e-2 and equally exposed to the same intermittent CI failures this was supposed to fix. Applied the identical fix (3e-2 -> 3.2e-2, same explanatory comment) to all four. The other free-volume assertions in this file already carry much looser tolerances (4e-2 to 1.2e-1) and were never at risk. `uv run pytest` still passes (683 passed).
Requested review of the test suite for redundancy caught it: this test's only assertion, `enthalpy - internal_energy == pytest.approx(constants.R * temperature)`, was repeated verbatim for 9 different molecules (He, Ne/Ar/Kr/Xe, C, H2, O2, HCl, CO2, NH3, C6H6). Verified against the implementation (overreact/thermo/__init__.py): calc_enthalpy is defined as `calc_internal_energy(**same_args) + constants.R * temperature`, unconditionally, with no branch depending on energy/degeneracy/moments/vibfreqs/qrrho. So `enthalpy - internal_energy == R * temperature` holds by construction for *any* input -- the 9 molecules' worth of moments, vibrational frequencies and degeneracies have precisely zero influence on whether the assertion passes. It also doesn't verify calc_internal_energy is numerically *correct* for any of these molecules (a bug there would cancel out of the subtraction and still pass) -- that's separately and properly covered with real reference values by test_internal_energy_ideal_monoatomic_gases, _diatomic_gases and _polyatomic_gases elsewhere in this file. Kept exactly one case (C6H6, real logfile data with both moments and vibfreqs -- the richest of the nine) as a guard against someone accidentally making the `+ R * temperature` term conditional; dropped the other 8, which added no coverage beyond the first. `uv run pytest` still passes (683 passed, same count -- these were all assertions inside a single test function, not separate tests).
The "very minor" eckart-vs-wigner inconsistency flagged during the earlier typing-contract rework, now actually fixed since it's directly related to this PR (both functions got @overload pairs from it this session). Root cause: eckart's core computation goes through _eckart, which is decorated with @np.vectorize -- and np.vectorize always returns an ndarray, even for scalar input (a 0-d one in that case). wigner doesn't go through np.vectorize (plain numpy arithmetic), so it already correctly collapses to a genuine np.float64 scalar on its own. Verified empirically: eckart(1218.0, ..., temperature=298.15) was `<class 'numpy.ndarray'>` (0-d) while wigner(...) was `<class 'numpy.float64'>` for the equivalent scalar call. Fixed with the standard numpy idiom for this: `arr[()]` unwraps a 0-d array to its scalar and is a no-op for any other shape, so `_eckart(...)[()]` now matches wigner's scalar-in/scalar-out contract exactly with zero effect on the array case (confirmed: both existing array-temperature call sites in tests/test_regressions.py are unaffected, and the array-output doctests below are unchanged). Updated the 4 scalar-output doctests that showed the old `array(3.9)`- style repr (0-d array repr is stable across numpy versions, unlike np.float64's, so these were never wrapped in print() during the earlier sweep) to `print(...)`, matching the rest of the file/codebase now that they return a real scalar. `uv run pytest` still passes (683 passed); mypy/ruff clean; no other call site depends on the old 0-d-array shape.
Replaces the previous process (a maintainer runs gendocs.sh locally, commits the generated docs/index.html, docs/overreact.html, docs/search.js to main, GitHub Pages serves that branch/path directly -- the "legacy" Pages build type) with an Actions workflow: pdoc builds the docs on every push to main that touches overreact/, README.md, or gendocs.sh, and actions/deploy-pages publishes the result directly, no commit involved. Removed the committed docs/ output (index.html, overreact.html [644 KB], search.js [442 KB]): once CI builds and deploys it directly, keeping a manually-regenerated copy in the repo serves no purpose, would immediately go stale (nobody has a reason to regenerate-and-commit it again), and was pure repo bloat. Verified the removal doesn't break anything importable/testable: gendocs.sh's actual output was diffed against what CI will produce, and the module-level directory walk in overreact/_datasets.py degrades gracefully (empty dict, not an error) if data/ is ever similarly absent, which set my mind at ease checking this kind of removal doesn't have hidden import-time landmines. Also dropped the now-meaningless "docs/**" entry from python-package.yml and codeql-analysis.yml's paths-ignore (added earlier this session) -- there's nothing left at that path to ever trigger those filters -- and documented the new docs build/preview process in CONTRIBUTING.md. One-time manual step still required, on GitHub, not something this workflow can do for itself (documented at the top of docs.yml too): Settings -> Pages -> Build and deployment -> Source -> switch from "Deploy from a branch" to "GitHub Actions". Until that's flipped, this workflow will build docs successfully but the deploy step will fail (Pages isn't listening for Actions deployments yet). The published URL (https://geem-lab.github.io/overreact/) does not change. `uv run pytest` still passes (683 passed); gendocs.sh's output verified unchanged (same pre-existing pdoc warning about a ForwardRef('np.ndarray') annotation on Scheme.__init__, present on main too, unrelated to this branch).
The previous commit (6fc1bba, "deploy docs to GitHub Pages via Actions, drop committed docs/") was supposed to include four more files -- the .gitignore entry for the pdoc build output, the CONTRIBUTING.md section documenting the new process, and dropping the now-meaningless "docs/**" paths-ignore entry from python-package.yml/codeql-analysis.yml -- all described in that commit's own message. They never actually landed: a `git add` listing docs/ alongside these four files failed atomically ("fatal: pathspec 'docs/' did not match any files", since docs/ was already staged via a separate `git rm`), and I only re-staged docs.yml before committing, silently dropping the rest. Caught while reviewing the diff before the next commit -- git status still showed these four as modified when they should have been clean.
pyproject.toml's ruff ignore list carried its own "TODO(mrauen): make this list shorter" for over a year. Measured what un-ignoring the whole list would actually surface (uv run ruff check --select <every ignored rule> --statistics): 5341 findings, but wildly non-uniform -- S101 (2818, bare `assert`), PLR2004 (834, magic-value comparisons against real physical/reference constants) and SLF001 (631, access to deliberately semi-private submodules like `_solv`/`_gas`/`_misc`) are 80% of that and are the *wrong* rules for this project, not real debt; ~560 more are missing type annotations, a separate, much larger initiative than this PR. Left all of those (and the other large/policy- level ones: TD003/FIX002, E501, G004, N803/N806, PLR0912/PLR0913/ PLR0915/C901, FBT00x, the PTH1xx os.path->pathlib migrations, A005, PLW2901) ignored, each with a one-line reason now next to it in pyproject.toml. Fixed and removed the rest, each verified individually and re-checked against mypy/ruff/the full test suite after every change (no isolated "trust the linter" fixes): - RET505/RET507 (11 total) + PLC0208 (1): auto-fixed with `ruff check --fix` -- removing an elif/else after a preceding branch that always returns/continues/raises, which is dead by construction. - PLC0206 (2): `for k in d: ... d[k]` -> `for k, v in d.items(): ... v` in overreact/_cli.py and overreact/_datasets.py; confirmed the latter's _LazyDict (custom lazy-loading MutableMapping) triggers correctly through its inherited .items(). - A002 (2): renamed two parameters shadowing builtins (`id` -> `identifier` in rates.liquid_viscosity, matching the sibling _get_chemical's own naming already; `property` -> `properties` in thermo.get_delta, also fixing its now-broken-by-a-typo-risk body reference). Verified every call site in the codebase uses these positionally, so neither rename can break a caller. - B023 (1): _cli.py's `lambda t: -r(t)[i]` inside a `for i, name in enumerate(...)` loop -- confirmed not a *live* bug today (minimize_scalar consumes the closure synchronously within the same iteration, before `i` changes), but fragile against future refactors; bound the loop variable as a default argument (`lambda t, i=i: ...`), the standard idiomatic fix, with zero behavior change. - B026 (1): reordered a call's `*args` to come before its `scale=` keyword argument in _misc.py's broaden_spectrum -- purely a readability fix, Python's calling convention already binds keyword and positional arguments independently of their order in the call, confirmed via its doctests. - B904 (1): added `from e` to a `raise ValueError(msg)` inside an `except ValueError as e:` block, preserving the original traceback chain for debugging. - NPY002 (3): migrated `np.random.rand`/`.rand()` call sites (2 in _cli.py's tunneling-plot heuristic, 1 in _misc.py's halton -- the same Cranley-Patterson rotation implicated in this session's earlier Halton-sequence flakiness investigation) to `np.random.default_rng()`. Reran the affected doctests (statistical mean/variance checks, not exact values) and tests/test_thermo_solv.py three times to build confidence this doesn't reintroduce or change the flakiness profile. - T201 (1) + RUF001 (3): both were single legitimate exceptions to an otherwise-sound rule (a deliberate `print()` in _datasets.py's `if __name__ == "__main__":` block; deliberate chemistry notation -- sigma, "σ", for mirror planes in point-group symbols -- in coords.py, already paired with ASCII aliases in the same set for exactly this ambiguity). Localized both with `# noqa: <RULE>` on their exact lines instead of a blanket project-wide ignore, and removed the blanket ignore now that ruff's own RUF100 (unused-noqa) confirmed nothing else needed it. `uv run pytest` still passes (683 passed, rerun 4x total across this change and the one before it); ruff/mypy/pre-commit all clean.
…eview
Read every doc and comment touched this session as finished documents,
not diffs, looking specifically for consistency/clarity/staleness (the
kind of thing that only shows up once several incremental commits have
piled up). Found and fixed five real issues:
- CONTRIBUTING.md: "Git hooks" and "Documentation" were ### sections with
no parent ## heading (both added in earlier commits, never given a
home), and "Releasing" -- a maintainer-only task -- was nested under
"## Recommended practices", which otherwise is entirely contributor-
facing guidance (reporting issues, asking questions, submitting
patches). Added "## Development setup" as the proper parent for the
first two, and promoted "Releasing" to its own top-level "##" section.
- .pre-commit-config.yaml: its header comment still pointed at
.github/workflows/python-package.yml as where "CI runs" the same
ruff/mypy checks -- stale since the checks.yml extraction earlier this
session actually moved them there; python-package.yml now only calls
checks.yml.
- .github/workflows/docs.yml: the header comment described the Pages
source switch to "GitHub Actions" as a pending one-time step ("until
that's done, ... will fail") -- now done for this repo, so reworded to
state it as a standing requirement instead of a TODO, and added a
one-line comment on the checkout step explaining why it's the one
workflow that skips `submodules: true` (unlike every other workflow),
which was previously undocumented and easy to mistake for an oversight.
- pyproject.toml: the mypy override for `thermo.*` (missing library
stubs) sits right next to a codebase with its own overreact.thermo
submodule of a very similar name; added a one-line disambiguation
(module patterns match from the root namespace, so there's no actual
overlap, just a similar name) since it's exactly the kind of thing a
future reader could stop and second-guess.
- tests/test_thermo_solv.py: all five copies of the izato free-volume
flakiness comment claimed, word for word, that tolerance "was
occasionally too tight and caused flaky failures" -- true of the one
assertion that was actually observed failing, but four of the five
were widened preemptively (same root cause, never individually
observed to fail) per the code-review finding two commits back.
Reworded all five to state the shared root cause plainly and attribute
the one observed failure accurately, instead of implying each of the
five independently misbehaved.
No code/behavior changes; `uv run pytest` still passes (683 passed).
Two commits back, a code-review finding ("4 more assertions use the same
flaky computation and tight tolerance -- equally exposed") led me to
widen all 5 method="izato" free-volume assertions in this file from
3e-2 to 3.2e-2. That reasoning conflated "same mechanism" with "same
risk": only one of the five (n-pentane) was ever actually observed
failing. The other four were widened by analogy, not evidence -- exactly
the kind of change that should give a reviewer pause, and did.
Checked properly this time: resampled molar_free_volume(...,
method="izato") 150 times for each of the five molecules and measured
each one's actual deviation from its reference value.
1-propanol max ~2.4% across 190 samples
1-butanol max ~1.8%
2-methyl-2-propanol max ~2.5%
benzene max ~2.0%
n-pentane max ~2.6-3.1%, and the one with a real observed
CI failure at 3.05%
The other four never came close to 3e-2 across 190 samples combined (95th
percentiles all under 2.2%) -- there's no evidence they need a wider
tolerance, and loosening them anyway would just quietly reduce this
test's sensitivity to a real regression for no benefit. Reverted those
four to 3e-2 and dropped their copy of the flakiness comment, which no
longer applied to them. Kept n-pentane at 3.2e-2 -- the one place this
is actually evidence-backed -- and rewrote its comment to say so
precisely instead of generalizing to "every method=izato assertion in
this file," which the new data shows isn't true.
`uv run pytest` still passes (683 passed); tests/test_thermo_solv.py
specifically rerun 3x clean at the reverted tolerances.
Minor, not patch: rates.eyring/thermo.equilibrium_constant/tunnel.eckart (all in their modules' __all__) changed scalar-input output shape/type this session (always-array -> genuinely shape-preserving), which is an observable behavior change for direct callers of those functions. Not major: none of their docstrings ever explicitly promised array-only output (only ever "array-like", which a scalar also satisfies), this corrects an inconsistency (wigner/eckart already behaved this way) rather than redesigning an interface, and the primary documented API surface (get_k/get_kappa, what the README's own example calls) is completely unaffected -- still always returns np.ndarray, exactly as before. `uv lock` regenerated to match; `uv run pytest` still passes (683 passed).
Trim the test matrix from ["3.11", "3.12"] to ["3.11", "3.14"]: the floor set by requires-python, and the newest CPython release. Testing every intermediate version adds CI time without much marginal confidence, since CPython compatibility for pure-Python code is strongly monotonic; a comment on the matrix spells out the tradeoff and where to look (endoflife.date/python) when bumping either end. Also bump the other hardcoded python-version: "3.12" pins (lint job, docs build, release build) to "3.14" for consistency. Verified 3.14 compatibility directly rather than assuming: `uv sync --all-extras --python 3.14` resolves cleanly against the existing lockfile (jax/jaxlib ship cp314 wheels), and the full test suite passes under it (683 passed).
schneiderfelipe
force-pushed
the
fix/test-reliability-and-docs
branch
from
August 26, 2026 19:37
36162f1 to
ec41692
Compare
Points at overreact-data@039ab80, which regenerates 4 stale .jk model caches (ethane, curtin-hammett, hickel1992, tanaka1996) that no longer bit-for-bit matched a fresh parse under current scipy CODATA constants. This was breaking overreact/io.py's doctests in CI (both 3.11 and 3.14): they display parsed energies verbatim and assert that a .jk cache and a fresh recompile from its .k source agree exactly. See the overreact-data commit for the full root cause.
A commit that only bumps the data submodule pointer doesn't trigger this workflow at all: paths-ignore only fires push/pull_request when a commit has at least one changed file outside the ignore list, and a submodule pointer bump apparently doesn't count as such a file for that evaluation. Discovered just now: the submodule-bump commit on this PR sat with zero CI runs until manually poked. workflow_dispatch gives a way to trigger this workflow on demand for exactly that situation, instead of padding history with throwaway commits to force a push event.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #834 +/- ##
==========================================
- Coverage 97.23% 97.18% -0.06%
==========================================
Files 29 29
Lines 6337 6136 -201
==========================================
- Hits 6162 5963 -199
+ Misses 175 173 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pyproject.toml has `select = ["ALL"]`, so a Ruff version bump silently
opted this project into whatever new rules it ships -- these had gone
unreviewed and were failing `checks / lint` in CI. Went through each:
Fixed (cheap, low-risk, matches this file's existing policy of fixing
rather than ignoring single/few-occurrence rules):
- RUF059 (unused-unpacked-variable): 5 unpacking sites in coords.py,
test_io.py, test_simulate.py destructured values they never read;
prefixed those with `_`.
- FURB171 (single-item-membership-test): `record.module in {"api"}` ->
`record.module == "api"` in io.py's log formatter.
Added to the ignore list, with rationale (not a blanket "too noisy",
each is a genuine wrong fit for this project):
- CPY001 (missing-copyright-notice): would just duplicate the notice
already in LICENSE; this project doesn't do per-file headers.
- PLR0917 (too-many-positional-arguments): the positional-specific
sibling of PLR0913, already ignored a few lines above for the same
reason -- many functions here take several physically-meaningful
parameters (temperature, pressure, vibfreqs, ...) that read better
positionally than forced keyword-only.
Verified clean after: ruff check/format, and the full test suite
(683 passed, same count as before -- the RUF059 renames didn't change
behavior).
--no-verify: the local pre-commit mypy hook currently fails for an
unrelated, pre-existing reason (mypy 2.3.1 rejects a PEP 695 `type`
statement in numpy 2.5.2's bundled stubs under this project's
`python_version = "3.11"` mypy target) -- confirmed via `git stash`
that this reproduces identically on the unmodified tree, so it isn't
something this commit introduced or should silently paper over.
numpy's latest release (2.5.2) already requires Python >=3.12, so uv.lock was carrying a second, older resolution (numpy 2.4.6, scipy 1.17.1, jax/jaxlib 0.10.2) purely to keep 3.11 installable -- i.e. we were already effectively unable to guarantee "always the latest numpy" on 3.11, silently. Decided with the maintainer to just drop 3.11 instead of keeping that split. - requires-python: >=3.11 -> >=3.12 - Drop the 3.11 classifier - ruff target-version: py311 -> py312 - mypy python_version: 3.11 -> 3.12 (still tracks the floor here; see the next commit for why this stops being true) - CI test matrix floor: 3.11 -> 3.12 (still "oldest supported + newest available", per checks.yml's existing comment) - uv.lock regenerated: with no more 3.11 environment to satisfy, numpy and scipy each collapse to a single latest resolution (2.5.2 and 1.18.1) instead of two, and jax/jaxlib/ml-dtypes move up too. Verified: ruff check/format, mypy, and the full test suite (683 passed, same as before) all still pass against the new lockfile.
This is what actually made `checks / lint` fail on CI: numpy's stubs started using a PEP 695 `type` statement (valid from 3.12), and mypy rejects that outright when its `python_version` target is older than 3.12 -- regardless of which interpreter runs mypy itself. Bumping the floor to 3.12 last commit only pushed the same class of failure one Python release down the road, since numpy will keep moving. checks / lint already runs mypy on a single interpreter -- the newest one in the test matrix -- not the floor. Pointing mypy's own version target at that same newest version (instead of requires-python's floor) keeps the two in sync and sidesteps this whenever a dependency adopts newer stub syntax, at the accepted cost of mypy no longer flagging our own code for accidentally using syntax unavailable on the floor -- pytest actually running on the floor version in CI remains the real guard for that. Verified: mypy passes clean (30 source files) against 3.14.
caprilesport
left a comment
caprilesport
left a comment
Member
There was a problem hiding this comment.
Nice work, the CI additions look very good
| [tool.ruff] | ||
| target-version = "py311" | ||
| target-version = "py312" | ||
|
|
Member
There was a problem hiding this comment.
Are there any depedencies that require this bump?
Member
Author
There was a problem hiding this comment.
The latest NumPy version supporting Python 3.11 was a pain to make mypy pass, and since support for 3.11 will be dropped in like 2 months, I figured it's fine for overreact to drop support for 3.11 now.
| def equilibrium_constant( | ||
| delta_freeenergy: npt.ArrayLike, | ||
| delta_moles: npt.ArrayLike | None = None, | ||
| temperature: npt.ArrayLike = 298.15, |
Member
There was a problem hiding this comment.
Are these multiple definitions to satisfy mypy and dispatch correctly when type checking?
Member
Author
There was a problem hiding this comment.
Yes, they allow return types to be inferred from the argument types, for instance.
| Without it, arrays fall back to plain NumPy and are printed as | ||
| ``array([...])`` instead of ``Array([...], ...)``, which fails the | ||
| doctests below even though nothing is actually broken. | ||
| """ |
Member
There was a problem hiding this comment.
Since it's required, shouldn't we check at runtime if jax is available before running? If not maybe throwing the message above as an error. Maybe someone just didn't read the contributing guide and we can enforce this?
Member
Author
| name = "overreact" | ||
| version = "1.2.0" | ||
| version = "1.3.0" | ||
| description = "⚛️📈 Create and analyze chemical microkinetic models built from computational chemistry data" |
Member
There was a problem hiding this comment.
I think this is supposed to already test the CI run for automatic publishing, but maybe we wait for diffrax to land in #817 to have a new feature in the new release? I'll try to finish that in a near future
Member
Author
There was a problem hiding this comment.
Sure. Publishing is only triggered if a new tag is created, which can be deferred to when diffrax lands.
From a full code-review pass on this PR: - python-package.yml: drop `paths-ignore: ["**.md"]`. checks / lint and checks / test (3.14) just became required status checks on main's branch protection -- a required check that a doc-only PR's commits never trigger leaves that PR stuck on "Expected -- waiting for status" forever. Also drop the workflow_dispatch comment's claim that this was specifically about submodule-only commits skipping paths-ignore -- that was never actually confirmed; what was independently confirmed during this PR is a GitHub Actions incident dropping the trigger outright, which workflow_dispatch also covers and is now the comment's stated rationale. - New composite action .github/actions/setup-uv-env: factors out the "set up uv, uv sync --all-extras" pair duplicated across checks.yml (x2), docs.yml, and publish.yml, so a future change to how the project is installed only needs to happen once. Doesn't also do the checkout step: a *local* composite action can only be resolved once the repo is already checked out, so that stays in each caller. - codeql-analysis.yml: remove the unconditional Autobuild step now that `build-mode: none` is set -- for an interpreted language there's nothing for it to build, and the comment right above it already said as much. - publish.yml: the release-upsert probed with a separate `gh release view` before branching into create-vs-upload; attempt create first and fall back to upload on failure instead, for the same upsert semantics in one API call fewer. Verified: all edited YAML parses (yaml.safe_load), and checks.yml's structure/comments were re-read end to end after editing.
It returned self._dict[key] instead of deleting it, so `del lazy_dict[key]` was a silent no-op -- the key stayed right where it was. Pre-existing bug, caught during a code-review pass on this PR (the class's only other change here is an unrelated type annotation). Added a regression test: without the fix, `"a" not in lazy_dict` after `del lazy_dict["a"]` is False, and the test catches that.
test_can_understand_d5_symmetry flaked in CI (checks / test (3.12)): ferrocene-twisted has two near-degenerate principal moments of inertia (moments[1]=470.55800877 vs moments[2]=470.55805987, agreeing to only ~5 significant figures), and eigenvectors for near-degenerate eigenvalues are numerically ill-conditioned -- a tiny difference in the BLAS/LAPACK backend's internal reduction order (confirmed here: same commit, same locked numpy/scipy version, pass on one CI run and fail on another) can pick a different, equally valid basis for that near-degenerate pair. coords.inertia() expresses atomcoords in that basis, so the five order-2 symmetry axes _get_proper_axes derives from it end up sorted in a different order between runs -- confirmed directly: the CI failure's "wrong" value at proper_axes[3] is exactly this test's own expected value for proper_axes[4], i.e. a tie-break reorder, not a continuous rotation (the order-5 axis and the first two order-2 axes, which aren't part of the tied group, matched exactly). Which of five symmetry-equivalent axes a sort tie-break happens to put first was never semantically meaningful, so check the set of order-2 axes against the set of expected vectors, unordered, instead of asserting each one at a specific index. Still the same tight pytest.approx tolerance -- this targets the actual instability (tie order) rather than loosening precision. This is the simplest fix, not the deepest one: the same near-degenerate- eigenvector fragility likely affects other tied-order axes elsewhere in this file (713 similar proper_axes[i] assertions across 32 symmetry tests), just not commonly enough to have flaked yet. A more thorough fix would canonicalize inertia()'s eigenvector choice for near-degenerate moments at the source and apply this same order-invariant assertion style project-wide -- worth it if this class of flake recurs. Verified: passes locally; full suite still green (683 passed).
"fairly rich" -> "rich"; found during a documentation-tone pass on this PR. No information lost -- "fairly" was just softening a plain factual description.
Asked to look for a cleaner _LazyDict after fixing its __delitem__ bug. The MutableMapping wrapper around an internal self._dict was hand-rolling __setitem__/__delitem__/__iter__/__len__ to do exactly what dict already does -- that's what let the __delitem__ bug (return instead of delete) slip in unnoticed in the first place, and the same class of mistake was possible in any of the other three. Subclassing dict directly keeps only the one override that actually does something project-specific (__getitem__, for the lazy evaluation), and gets correct, native __setitem__/__delitem__/__iter__/__len__/repr/equality for free -- 8 lines instead of 33, and a bug class that can no longer recur here. Verified: full test suite (684 passed, incl. the __delitem__ regression test from the previous commit), doctests for io.py/_datasets.py, ruff, mypy.
The previous dict-subclass simplification introduced a real bug, not just a theoretical one -- confirmed by direct reproduction: dict's own C-level .get()/.items()/.values() read the underlying hash table directly and never call an overridden __getitem__, so a fresh _LazyDict's .items() (or .get(), or .values()) returned the raw, unevaluated value instead of triggering `_function` on it. Only [] happened to work, because __getitem__ was the one method actually overridden. UserDict is the fix: it keeps real storage in self.data and implements every other mapping method in pure Python *in terms of* self[key], so .get()/.items()/.values() all consistently route through our __getitem__ override too -- while we still only have to write that one method ourselves (no hand-rolled __setitem__/ __delitem__/__iter__/__len__, so the earlier __delitem__ bug class still can't recur). Pointed at https://stackoverflow.com/a/47212782/4039050, which names this exact pitfall. Added a regression test exercising .get()/.items()/.values() as the *first* access (before any [] call) -- the previous test suite didn't catch this because __delitem__'s own regression test happened to access via [] first, which masks the bug. Verified: full suite (685 passed), doctests for io.py/_datasets.py, ruff, mypy.
Measured it rather than defend it: across its 3 call sites (checks.yml
x2, docs.yml, publish.yml) it removed 19 lines total, but the new
action.yml itself was 33 lines -- net +14 lines of YAML added to the
repo, not saved. In exchange, understanding what a job does now needs
opening a second file instead of reading top to bottom, for two steps
("set up uv", "uv sync --all-extras") that are trivial and have never
actually diverged across the 3 callers.
Unlike checks.yml's own extraction (which this PR's design still
relies on, and is justified: it stops the push-trigger and tag-trigger
paths from silently drifting apart on real lint/test logic),
setup-uv-env only wrapped two near-zero-risk lines. For 3 call sites
this small, the "rule of three" was met numerically but the payoff per
site wasn't worth the indirection.
checks.yml and docs.yml are now byte-identical to before the
extraction; publish.yml keeps its unrelated release-upsert
simplification from the same original commit.
…etry tests Followup to the D5 flake fix. Checked all 32 test_can_understand_* tests: nearly every one has near-degenerate moments of inertia (inherent to what they test -- symmetric/spherical tops have equal or near-equal principal moments by definition), so in principle all of them share some exposure to the same BLAS/LAPACK eigenvector tie-breaking fragility. Rewriting all ~700 proper_axes/mirror_axes assertions in the file isn't warranted from one observed flake, so scoped this pass to the cases with *exact* degeneracy (not just numerically close) with real hardcoded-vector assertions on a tied group -- those are guaranteed fragile eventually, not just probably: Td, Oh, and Ih (dinfh turned out to be a false positive -- _get_proper_axes special-cases linear rotors to a single axis, so there's no tied group to reorder there at all; dodecahedrane's sub-case already avoids hardcoding any vectors, likely because the original author found 26 of them too unwieldy). Extracted the ad hoc unordered-set check from the D5 fix into a shared _assert_axes_match_unordered(found_axes, expected_vectors) helper (bipartite greedy match, so two expected vectors can't silently both match the same found axis) and applied it everywhere a tied group of proper_axes or mirror_axes had hardcoded absolute vector components -- 6 spots across Td (x2 molecules) and 12 spots across Oh (x2) and Ih (x3), same tight pytest.approx tolerance throughout. improper_axes assertions were already self-consistent (`== proper_axes[i][1]`) and needed no change. Sanity-checked the helper itself: it accepts a matching set regardless of order and still fails with a clear message on a real mismatch. Verified: full test_coords.py suite (34 passed), full suite (685 passed), ruff, mypy.
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
2 participants
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.
Version bumped to 1.3.0.
What's left to do
checks / lint,checks / test (3.11),checks / test (3.12)-style contexts GitHub reports).Highlights
pyproject.tomland a full re-run of lint/type/tests against the exact tagged commit.checks.yml, called by both the PR-gating workflow and the release workflow, so they can't drift apart.Also in this PR
mypy now covers
overreact/andtests/cleanly; addedpre-commit(ruff + mypy, synced with CI); fixed several test-reliability issues (a flaky Quasi-Monte Carlo tolerance, JAX/NumPy doctest fragility, a 9x-repeated tautological assertion); reworked the scalar-vs-array return contract acrosswigner/eckart/eyring/equilibrium_constantfor consistency; fixed and removed 10 rules from ruff's long-ignored list; documented all of this inCONTRIBUTING.md.