Test reliability, mypy, CI modernization, and automated releases - #834
Open
schneiderfelipe wants to merge 35 commits into
Open
Test reliability, mypy, CI modernization, and automated releases#834schneiderfelipe wants to merge 35 commits into
schneiderfelipe wants to merge 35 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).
main moved forward with a Dependabot bump (#833) to github/codeql-action while this branch was open, which conflicted with this branch's own codeql-analysis.yml changes (build-mode: none, dropped Autobuild, concurrency, paths-ignore) on the same lines. Applying the version bump here by hand instead of merging main in, to keep this branch's history linear and each reconciled file its own reviewable commit.
main moved forward with several Dependabot PRs while this branch was open (rich <15->16, ipython <9->10, pdoc <15->17, pytest-cov <6->8, ruff <0.16.3->0.16.5, #827-#831), all touching the same dev/cli dependency lists this branch's own changes (drop perflint, add pre-commit, ruff ignore-list edits, [tool.mypy]) live in. Applied the version bumps by hand instead of merging main in, to keep this branch's history linear and each reconciled file its own reviewable commit -- verified the result now differs from origin/main's pyproject.toml by exactly this branch's own intentional changes, nothing left over. uv.lock regeneration is its own following commit.
Reflects the previous commit's dependency-bound merge: targeted --upgrade-package for exactly the four packages main's Dependabot PRs actually bumped since this branch diverged (nbconvert, notebook, pytest, urllib3) rather than a blanket --upgrade, to keep the diff limited to what's actually needed for reconciliation. Resolved to slightly newer patch versions than main has for notebook/pytest (7.5.7/9.1.1 vs 7.5.6/9.0.3) since more have been released since main's PRs merged -- still within pyproject.toml's constraints either way. pytest 8.4.2 -> 9.1.1 is a major bump; verified it doesn't break anything this project relies on (--doctest-modules, --doctest-glob, the custom NUMBER doctest flag): `uv run pytest` still passes (683 passed, same count), ruff/mypy/pre-commit all still clean.
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). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fx96UthXND8qo7LuZ9PCS3
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
1 participant
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.