From 65b6d6357bd99d919ffd3473d95ba5b5aa47fbc7 Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Fri, 28 Aug 2026 13:00:45 +0100 Subject: [PATCH 1/2] Close Stage 4: ten-criterion exit audit, three overstated verdicts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run under prompts/common/AUDITOR.md's stance against a green make ci (605 tests, 99%, 53 scenarios) and a green real CI run on main. Seven verdicts held; three did not, and the stage now closes at nine of ten rather than ten of ten. Criterion 4: the Advection conservation scenario passed for any flux array whatsoever -- its fixture zeroes every boundary face's normal velocity, and interior faces cancel structurally. Confirmed by mutation, not inferred. Adds the criterion's own first-named case (a fully periodic domain), mutation-verified in both directions; keeps and annotates the weak one rather than deleting it. Criterion 6: the shared-step-vocabulary half was never attempted, and the criterion had pre-declared its own failure signature. Left open deliberately -- it conflicts with a documented tests/unit/ convention, and choosing between them is a design decision, not an audit's call. Criterion 10: the sweep covered the files Stage 4 touched, not the files it invalidated. Seven documentation defects, the last three found by the new rule the first four produced. Why, in full: docs/planning/roadmap.md's Stage 4 status table and docs/CHANGELOG-DESIGN.md, 28-08-2026. Carried forward: docs/planning/backlog.md §13. Co-Authored-By: Claude Opus 5 --- adr/ADR-002-fvm-first.md | 27 ++- docs/CHANGELOG-DESIGN.md | 174 ++++++++++++++++++ docs/architecture/engine.md | 36 ++-- docs/architecture/overview.md | 26 ++- docs/architecture/rendering.md | 37 +++- docs/planning/backlog.md | 59 ++++++ docs/planning/roadmap.md | 59 +++++- docs/planning/status.md | 6 +- docs/practices.md | 47 +++++ src/pyflow/CLAUDE.md | 16 ++ src/pyflow/bootstrap.py | 18 +- src/pyflow/engine/CLAUDE.md | 30 ++- .../first_order_upwind_advection.feature | 29 +++ tests/unit/CLAUDE.md | 34 +++- .../unit/test_first_order_upwind_advection.py | 47 ++++- 15 files changed, 590 insertions(+), 55 deletions(-) diff --git a/adr/ADR-002-fvm-first.md b/adr/ADR-002-fvm-first.md index 32e6b8b..d44dc62 100644 --- a/adr/ADR-002-fvm-first.md +++ b/adr/ADR-002-fvm-first.md @@ -45,13 +45,26 @@ transported quantity's total is unchanged after many timesteps, to floating-point tolerance, on a domain whose boundary conditions all prescribe a zero gradient. -**What still does not exist:** a real Time Integration, Pressure-Velocity -Coupling, or Linear Solver implementation -- `docs/architecture/engine.md`'s -own entries for those three layers still name `assembly.py`'s trivial, -non-physical reference implementation, not a real scheme. Stage 4 -(`docs/planning/roadmap.md`, TASK-025..030) brings each in turn; -`first_order_upwind`/`central_difference` are the first two of the six -`adr/ADR-003` components to go real, not the last. +**All six now exist** (Stage 4 closed 2026-08-28): `RK4Integrator`, +`PISO` and `ConjugateGradientSolver` followed advection and diffusion +over TASK-025..027, and `DirichletBoundaryCondition`/ +`NeumannBoundaryCondition` over TASK-028/029, leaving `assembly.py` with +zero reference implementations. The paragraph this replaces read "**What +still does not exist:** a real Time Integration, Pressure-Velocity +Coupling, or Linear Solver implementation -- `docs/architecture/ +engine.md`'s own entries for those three layers still name +`assembly.py`'s trivial, non-physical reference implementation" -- true +when TASK-024 wrote it, falsified three days later by the very tasks its +own next sentence named, and found by the 2026-08-28 Stage 4 exit audit +rather than by any of them. Recorded rather than quietly overwritten: +**an ADR that tracks implementation status takes on that status's +maintenance burden**, and this is the one instance in the repository +where an ADR did. + +What genuinely does not exist yet is the *solver* those six compose +into -- Stage 5 (`docs/planning/roadmap.md`, TASK-031..034) is what +gives the engine a velocity field to correct and a pressure field to +solve for. --- diff --git a/docs/CHANGELOG-DESIGN.md b/docs/CHANGELOG-DESIGN.md index d101540..d504168 100644 --- a/docs/CHANGELOG-DESIGN.md +++ b/docs/CHANGELOG-DESIGN.md @@ -5593,3 +5593,177 @@ a released pytest-bdd containing #827 and expect a pass. `make check-claims` returning only its five known false positives. Documentation only -- no code changed, and TASK-018 deliberately not started. + +--- + +## 28-08-2026 + +### This log has a six-day gap, and it is not backfilled + +The entry above this one is dated 22-08-2026. Between then and now the +project built Stage 3 in full (TASK-018..022), Stage 4 in full +(TASK-040, TASK-023..030), two ADRs (`ADR-008`, `ADR-009`) and one +golden demo that computes real physics -- none of it recorded here. +`docs/practices.md`'s Session Workflow step 4 says to record new +decisions in this file; for six days that step was skipped, and every +decision went into `docs/planning/roadmap.md`'s task entries and the +`CLAUDE.md` tree instead. Those records are genuinely good -- this is a +duplication-of-venue problem, not a lost-knowledge one -- but the gap is +real, and a reader starting here would not know it exists. + +**Deliberately not backfilled.** Writing six days of entries from their +outcomes would mean narrating sessions nobody present witnessed, in a +document whose whole value is that it says what was actually decided +when. Recorded as an open item instead (`docs/planning/backlog.md` §13), +with the decision it needs stated: either this log is a live obligation +and the workflow needs something that fails when it is skipped, or the +roadmap is the decision record and step 4 should say so. + +### Stage 4 closed: ten-criterion exit audit, three overstated verdicts + +Run under `prompts/common/AUDITOR.md`'s stance against a green `make ci` +(605 tests, 99% coverage, 53 scenarios across 14 feature files) and a +green real CI run on `main` (33163793986). Seven of the ten verdicts +held on inspection; three did not. + +**Criterion 4 (physical correctness): the Advection conservation +scenario passed for reasons unrelated to what it claimed.** Its fixture +gives every boundary cell zero velocity, so every boundary face's flux +is `velocity_normal * phi = 0` whatever face value the scheme picks, +while interior faces cancel by construction inside +`accumulate_flux_to_cells`. The scenario therefore passes for *any* flux +array -- confirmed by mutation: forcing every advective face flux to +`0.0` leaves it green. That is precisely the qualifier the criterion had +reserved ("a bounded scheme can still fail to conserve if its flux +accounting is wrong"), reserved in a sentence nobody re-read while +writing the fixture. + +Fixed by building the criterion's own first-named case, which nobody +had: "Conservation on a fully periodic domain" -- all four edges +periodic, velocity `(1.7, -0.9)`, **no boundary condition configured at +all**. Every boundary face now carries a genuinely nonzero flux, and +global cancellation becomes a real property of the wrap accounting: +`accumulate_flux_to_cells` credits a periodic face to its owner alone +(the mesh reports no neighbour), so a periodic pair cancels only if both +faces resolve the same upstream cell. Mutation-verified in both +directions before being trusted -- clamping the wrapped neighbour to the +owner drifts the total from 52.0 to 54.87 and fails the new scenario, +while leaving the old one passing. The weak scenario is kept, with its +limitation written into the feature file rather than deleted: it still +checks that a zero-velocity boundary face contributes nothing +*additively*. + +Worth recording as the contrast that made the defect visible: +**Diffusion's** conservation scenario, written the same day in the same +shape, does have teeth -- mutating its boundary-gradient branch drifts +the total by 37.4. The difference is not care. It is that diffusion's +boundary flux has no velocity factor sitting in front of it to zero the +whole term out. A fixture can be degenerate in a way that stays +invisible unless someone asks which factor is zero. + +**Criterion 6 (executable Gherkin criteria): the gating half was met, +the shared-vocabulary half was never attempted** -- and the criterion had +pre-declared exactly what its own failure would look like ("a large crop +of task-specific step definitions by the time this Stage closes is +itself a finding against this criterion"). Counted: 9 shared steps, zero +physics-shaped additions, 109 task-specific step definitions across nine +modules, no shared `tests/unit/conftest.py`. + +The part worth carrying: **this was a documented decision, not an +oversight.** `tests/unit/CLAUDE.md` states "each binding test supplies +its own local steps" as a convention, written into TASK-024's entry and +restated for every task after. Two documents said opposite things for +six days and both were followed, because nothing ever put them on the +same page. Not resolved here -- which one is right is a design decision, +and an audit that picks one is deciding rather than auditing. Carried to +`docs/planning/backlog.md` §13, due before Stage 5's criteria are +drafted. + +**Criterion 10 (documentation matches the tree): the sweep's scope was +wrong, in the way Stage 3's audit had already found once.** It covered +the files Stage 4 touched, not the files Stage 4 invalidated. Four +defects, all in files no Stage 4 task opened: `engine.md`'s Flux entry +named no module for `accumulate_flux_to_cells`, which this criterion's +own text explicitly required; four `engine.md` entries carried a +decaying count of surviving `_Null*` classes -- "the five that still do", +"the four", "the three", "the two" -- of which there have been zero +since TASK-029, contradicting Criterion 7 two rows above in the same +table; `rendering.md` called simulation/frame scheduling "Stage 4+ work" +three commits after Stage 4 shipped it; and `bootstrap.py`'s module +docstring said "No simulation functionality" twenty lines above its own +`import ... simulation_step`. + +Then the rule those four produced (below) was run against the repository +before the verdict was written, and found three more: `docs/architecture/ +overview.md` still said "No concrete numerical *scheme* exists behind any +of [the six] yet -- that is Stage 4" -- the second time that file has +gone stale about a closed stage -- and, three lines further down, +called the `numerics.*` configuration section "the part still missing, +because the interfaces it would select among do not exist yet", which its +own bullet above contradicts; and `adr/ADR-002-fvm-first.md` still +carried "**What still does not exist:** a real Time Integration, +Pressure-Velocity Coupling, or Linear Solver implementation", written by +TASK-024 and falsified three days later by the very tasks its own next +sentence named. Seven in total, all fixed here. + +A rule that finds three more defects the moment it is written is a +better argument for itself than the four that prompted it. The narrower +lesson from the ADR-002 one is worth keeping separately: **an ADR that +tracks implementation status takes on that status's maintenance +burden.** ADR-002 is the only one in this repository that does, and it +is the only one that went stale. + +### The rule this produced, and why it is phrased as three greps + +`docs/practices.md` gains "A stage's documentation sweep is a grep, not +a diff review". The rule directly above it -- "Let a checked artifact +carry status, not a tense", written by Stage 3's own audit -- converted +`engine.md`'s `Implementation:` lines to checked module paths and +stopped there, and Stage 4 then reproduced the same class of defect in +three files it did not cover. A count is not a tense; a negation in a +`src/` docstring is not a documentation field; neither is reachable by +making one document's labels checkable. + +So the new rule is scoped to the class rather than the file: before +closing a stage, grep the whole repository -- `src/` docstrings included +-- for (1) the stage's own number and the next one, (2) every count of +anything the stage changed, (3) the negations (`does not exist`, +`no ... yet`, `nothing that`, `still`). Each of the four defects above +falls out of one of those three in seconds. **Fix the class, not the +file** is the actual lesson, and the reason this rule is not "also +re-read `rendering.md`". + +The `bootstrap.py` docstring deserves one more sentence, because it is +the sharpest instance: the identical stale self-description was found in +`__main__.py`'s CLI help text and fixed the same morning (commit +73ff113, with its own standing rule added to `src/pyflow/CLAUDE.md`). +That fix's blast-radius sweep stopped at the file the user had pointed +at. One `grep -rn "no simulation" src/` would have found the second +copy; the sweep was a re-read of the diff instead. + +### What the audit checked and did not find fault with + +Recorded because `docs/practices.md` says an audit finding nothing +should make you suspect the criteria -- the converse is that an audit +finding three things should say what the other seven were, or the +reader cannot tell thorough from lucky. + +Criterion 1's no-boundary-branching claim is real and checked by an +`inspect.getsource` assertion, not only behaviourally. Criterion 2's +seven MVP names each resolve to a real class under an `isinstance` +assertion, and the six component `Literal`s in `schema.py` were +confirmed untouched across every Stage 4 commit by `git log -p`, not by +reading the current file. Criterion 7 was confirmed by reading +`assembly.py`'s registration calls directly: seven real classes, zero +`_Null*`. Criterion 9's evidence is a real run, and `main` is green now +as well. TASK-030's own round-trip scenario -- the one this stage had +the most reason to be suspicious of, since it checks convergence rather +than the exact round-trip equality the criterion's headline asks for -- +was mutation-verified and holds: a clamped wrap gives 1.052 against a +0.831 bound and fails it. + +- *Verified by:* `make ci` clean on the audit branch (606 tests, 54 + scenarios across 14 feature files, 99% coverage, all twelve gates); + `make check-claims` returning only its two known false positives; each + mutation above run and reverted individually, with the numbers quoted + read off the actual failure output rather than predicted. diff --git a/docs/architecture/engine.md b/docs/architecture/engine.md index 34abf8e..dd7c9c4 100644 --- a/docs/architecture/engine.md +++ b/docs/architecture/engine.md @@ -145,7 +145,19 @@ Gradient and Divergence operator interfaces (TASK-018, Stage 3) jointly compute. Naming it as its own layer (per `docs/glossary.md` and `upgrade-paths.md`) matters because a face flux is what those operators' outputs mean physically, even though no single class named `Flux` need -exist in the implementation. **Gradient and Divergence gained their own +exist in the implementation. **What actually sums those face values back +onto a control volume is `src/pyflow/engine/simulation.py`'s +`accumulate_flux_to_cells` (TASK-040, Stage 4)** -- the discrete Gauss +theorem, `sum(value * area * outward_normal_sign) / volume` per cell, +generic over any `(mesh.num_faces,)` array regardless of which operator +produced it, and reused directly by `step`, `GreenGaussGradient` and +`GreenGaussDivergence` alike. "No module of its own" stays true (there +is no `Flux` class, and this function is not one) but it never meant "no +module computes this": this entry's own **Represents** sentence -- "what +actually gets summed over each control volume's faces" -- names a +concrete operation, and until the 2026-08-28 Stage 4 exit audit this +entry named no module for it while Stage 4 Completion Criterion 10 +explicitly required one. **Gradient and Divergence gained their own first real implementations in TASK-027** (Stage 4, `src/pyflow/engine/ numerics/gradient.py`'s `GreenGaussGradient`, `divergence.py`'s `GreenGaussDivergence`) -- built and owned by that task directly, not @@ -172,9 +184,9 @@ First-order Upwind Advection, Stage 4). The interface and its MVP scheme, `FirstOrderUpwindAdvection`, both live there; `engine/ numerics/assembly.py` registers it under this layer's configured name (`"first_order_upwind"`) -- the first of the six `adr/ADR-003` components -whose registered name resolves to a real implementation rather than -`assembly.py`'s own trivial, non-physical reference class (see that -module's own docstring for the five that still do, and why). +to go real (2026-08-27). All six do now, and `assembly.py` holds zero +`_Null*` reference classes: see that module's own docstring for the +per-task retirement history. **Upgrade path:** upwind → central difference → QUICK → TVD → WENO (`upgrade-paths.md` "Advection"). @@ -195,9 +207,8 @@ TASK-024 Central Difference Diffusion, Stage 4). The interface and its MVP scheme, `CentralDifferenceDiffusion`, both live there; `engine/ numerics/assembly.py` registers it under this layer's configured name (`"central_difference"`) -- the second of the six `adr/ADR-003` -components whose registered name resolves to a real implementation -rather than `assembly.py`'s own trivial, non-physical reference class -(see that module's own docstring for the four that still do, and why). +components to go real (2026-08-27); see Advection, above, for the state +of the other five. **Upgrade path:** simple central formulation → improved geometric/ non-orthogonal handling (`upgrade-paths.md` "Diffusion"). @@ -222,10 +233,8 @@ derivative at intermediate states a fixed value cannot supply. TASK-025 RK4 Time Integration, Stage 4). The interface and its MVP scheme, `RK4Integrator`, both live there; `engine/numerics/assembly.py` registers it under this layer's configured name (`"rk4"`) -- the third -of the six `adr/ADR-003` components whose registered name resolves to a -real implementation rather than `assembly.py`'s own trivial, non-physical -reference class (see that module's own docstring for the three that -still do, and why). +of the six `adr/ADR-003` components to go real (2026-08-27); see +Advection, above, for the state of the other five. **Upgrade path:** Euler → RK2 → RK4 → adaptive RK → implicit (`upgrade-paths.md` "Time Integration"). @@ -278,9 +287,8 @@ TASK-026 Conjugate Gradient Solver, Stage 4). The interface and its MVP scheme, `ConjugateGradientSolver`, both live there; `engine/ numerics/assembly.py` registers it under this layer's configured name (`"conjugate_gradient"`) -- the fourth of the six `adr/ADR-003` -components whose registered name resolves to a real implementation -rather than `assembly.py`'s own trivial, non-physical reference class -(see that module's own docstring for the two that still do, and why). +components to go real (2026-08-27); see Advection, above, for the state +of the other five. Handles the lid-driven cavity's positive-semi-definite pressure system (constant vector in the null space) via a gated projection, not unconditionally -- `adr/ADR-003`'s own "Updated" section and diff --git a/docs/architecture/overview.md b/docs/architecture/overview.md index 129f7ca..a316aa1 100644 --- a/docs/architecture/overview.md +++ b/docs/architecture/overview.md @@ -103,16 +103,32 @@ stopped being true when TASK-011 landed on 2026-08-20). Stage 3 (TASK-018..022, done 2026-08-23) gave all six an interface and a real `numerics.*` configuration section; that document's per-contract "Configuration control" lines name the keys and say - which stage implemented them. No concrete numerical *scheme* exists - behind any of them yet -- that is Stage 4. + which stage implemented them. Stage 4 (TASK-040, TASK-023..030, done + 2026-08-28) put a real concrete scheme behind every one of the six -- + `FirstOrderUpwindAdvection`, `CentralDifferenceDiffusion`, + `RK4Integrator`, `ConjugateGradientSolver`, `PISO`, and + `DirichletBoundaryCondition`/`NeumannBoundaryCondition` -- and + `assembly.py` holds zero reference implementations as a result. This + bullet read "No concrete numerical *scheme* exists behind any of them + yet -- that is Stage 4" until the 2026-08-28 Stage 4 exit audit, which + is the **second** time this file has gone stale about a stage that + had already closed (see the bullet above, and `docs/practices.md`, "A + stage's documentation sweep is a grep, not a diff review"). Configuration (`src/pyflow/configuration/`) sits in between and has grown with the engine rather than ahead of it: `PyFlowConfig`, YAML loading and validation are TASK-005's, `MeshConfig` arrived with TASK-012, `FieldDisplayConfig` with TASK-017, and a generator -(TASK-039) writes a valid file from the schema. The `numerics.*` -section `icds.md` proposes is the part still missing, because the -interfaces it would select among do not exist yet. +(TASK-039) writes a valid file from the schema, and the `numerics.*` +section arrived across Stage 3 alongside the interfaces it selects among +(`NumericsConfig`, TASK-018..022) -- with `simulation.*` +(`SimulationConfig`, TASK-030) the most recent addition, the first +section to drive a live stepping run rather than a single rendered +frame. This sentence read "the part still missing, because the +interfaces it would select among do not exist yet" until the 2026-08-28 +Stage 4 exit audit, while the bullet immediately above it already said +those six interfaces existed -- one file contradicting itself three +lines apart, which is what a status claim restated in prose costs. ## Why This Split diff --git a/docs/architecture/rendering.md b/docs/architecture/rendering.md index 17e34f1..334ca49 100644 --- a/docs/architecture/rendering.md +++ b/docs/architecture/rendering.md @@ -134,16 +134,33 @@ types into pygfx geometry is exactly their job. The dependency runs one way and should stay that way. `docs/architecture/overview.md`'s "Why This Split" section carries the full statement. -The relationship these two documents will eventually need to describe -- -how a simulation timestep and a render frame are scheduled relative to -each other (locked step, decoupled, capped at a target frame rate) -- -still does not exist, because nothing produces a timestep to schedule -against. Stages 1 and 2 put real content in the `Scene` (a mesh, then -fields) but nothing that advances in time. `on_frame` is the seam this -future scheduling will attach through; the scheduling policy itself is -**Stage 4+ work** (this read "Stage 1+" until 2026-08-22, which two -completed stages have since made meaningless), not specified here ahead -of having anything to schedule. +The relationship these two documents need to describe -- how a +simulation timestep and a render frame are scheduled relative to each +other (locked step, decoupled, capped at a target frame rate) -- **now +exists, and the policy is locked step: exactly one +`simulation.step()` per rendered frame.** `on_frame` is the seam it +attaches through, as predicted; `src/pyflow/bootstrap.py`'s +`_add_passive_scalar_transport` is what attaches to it, advancing the +field by `numerics.timestep` and re-rendering after every frame +(TASK-030, Stage 4, 2026-08-28 -- the Passive Scalar Transport golden +demo). + +Locked step is a real choice, not the absence of one, and its +consequence is worth naming: wall-clock speed is whatever the render +loop achieves, so `numerics.timestep` sets simulated time per *frame*, +not per second. Decoupling the two (a fixed simulated-time budget per +frame, sub-stepping to fill it) is the upgrade this seam leaves +available, not something the current policy forecloses -- but nothing +has needed it yet, so it is not built (P-016). + +**This paragraph read "still does not exist, because nothing produces a +timestep to schedule against ... the scheduling policy itself is Stage +4+ work" until the 2026-08-28 Stage 4 exit audit found it**, three +commits after the policy shipped. No Stage 4 task touched this file, so +nothing prompted a re-read of it -- the same failure mode Stage 3's own +audit found in `docs/architecture/overview.md`, recurring in the next +stage for the same reason (see `docs/practices.md`, "A stage's +documentation sweep is a grep, not a diff review"). ## Adding a Second Renderer diff --git a/docs/planning/backlog.md b/docs/planning/backlog.md index bcb0731..4a010f9 100644 --- a/docs/planning/backlog.md +++ b/docs/planning/backlog.md @@ -2854,3 +2854,62 @@ Original finding: "items fully complete but still showing `[ ]`" drift `docs/practices.md` records under "Closing a backlog item is a Blast Radius event". + +--- + +## 13. Carried forward from the Stage 4 exit audit (2026-08-28) + +Two findings the audit deliberately did **not** fix, because each needs a +design decision rather than a correction. Everything else it found is +fixed in its own change -- see `docs/planning/roadmap.md`'s Stage 4 +status table for the full per-criterion record. + +- [ ] **Reconcile Stage 4 Completion Criterion 6's shared-step-vocabulary + bullet against `tests/unit/CLAUDE.md`'s "each binding test supplies + its own local steps" convention.** The two say opposite things and + both were followed for the whole of Stage 4, which is how the + criterion came to be marked Met while its second half had never + been attempted. Measured at the stage boundary: + `tests/golden/conftest.py` holds 9 steps and gained zero + physics-shaped additions; the nine Stage 4 binding modules define + 109 step definitions between them; there is no shared + `conftest.py` under `tests/unit/` at all. "A small, non-square, + non-trivially-origined mesh" is implemented six times; + `_face_normal_velocity` four times, each copy's docstring pointing + at another copy. + + **Both resolutions are legitimate and the choice is real.** The + convention has a genuine argument behind it -- a step definition + shared across nine modules couples nine tasks' fixtures together, + and this repository has already been bitten by a shared fixture + that made two formulas agree. The criterion has the opposite one: + 109 hand-maintained steps is where a step vocabulary stops being + readable as criteria. Decide which, amend the loser, and say so + where the other one is stated. Do not simply delete the criterion's + bullet -- `docs/practices.md`'s "every qualifier becomes a bullet, + or is struck" makes striking a legitimate outcome, but only as a + recorded decision. + + Due before Stage 5's own criteria are drafted, since Stage 5's + tasks are all simulation work and will inherit whichever answer + this gets. + +- [ ] **`docs/CHANGELOG-DESIGN.md` has no entry after 2026-08-22.** + Stages 3 and 4 -- TASK-018 through TASK-030 plus TASK-040, two + ADRs (`ADR-008`, `ADR-009`), and every design decision recorded in + them -- are logged in `docs/planning/roadmap.md`'s task entries and + in `CLAUDE.md` files, but not there. `docs/practices.md`'s Session + Workflow step 4 ("Record any new decisions in + `docs/CHANGELOG-DESIGN.md`") was not followed for six days, and + nothing in `make ci` can notice, since the file is append-only + prose with no checked property. + + **Not backfilled by this audit, deliberately:** reconstructing six + days of decision history from their outcomes would produce a + plausible narrative of sessions nobody present witnessed, which is + the one thing this repository's Integrity section rules out. The + audit appended an entry for its own work and recorded the gap + there instead. Decide whether that log is still a live obligation + -- if it is, the workflow needs something that fails when it is + skipped; if it is not, retire step 4 and say the roadmap is the + decision record. diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index bf7a8ad..0ee304b 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -189,7 +189,7 @@ This paragraph previously said `make install` and `make test` were still expected to fail, pending `uv.lock` and a test suite (B2/C1) -- stale since 2026-08-16 and corrected 2026-08-19. Both now succeed: `uv.lock` is committed (B2) and `make test` runs the suite with coverage -(C1a/C1b): **605 tests at 99% as of 2026-08-28**, having been 64 when +(C1a/C1b): **606 tests at 99% as of 2026-08-28**, having been 64 when this paragraph was rewritten on 2026-08-19, 202 earlier the same day, 212 after TASK-014, 226 after TASK-015, 250 after TASK-016, 287 after TASK-017, 297 after TASK-039, 315 after the Stage 2 exit audit and 337 @@ -383,8 +383,14 @@ stage exit unedited), with two new tests holding it there -- (`tests/unit/test_main.py`) and its subprocess-boundary mirror in `tests/integration/test_cli.py` -- and a dated rule in `src/pyflow/CLAUDE.md` requiring this text be revisited whenever a -subcommand, flag, or golden demo changes. **53 of those 605 are Gherkin -scenarios rather than pytest functions** +subcommand, flag, or golden demo changes. 606 the same day, after the +Stage 4 exit audit: one new Gherkin scenario in +`first_order_upwind_advection.feature` ("Conservation on a fully +periodic domain"), added because the existing closed-domain conservation +scenario turned out to pass for any flux array whatsoever -- see this +Stage's own Completion Criterion 4 row, below, for the mutation evidence +and why the weak scenario was annotated rather than deleted. **54 of +those 606 are Gherkin scenarios rather than pytest functions** (`adr/ADR-007-executable-acceptance-criteria.md`; up from fourteen with `field_display.feature` gaining scenarios and `numerics_assembly.feature` joining, TASK-021; to 24 with TASK-040's own @@ -425,7 +431,10 @@ TASK-030's own golden demo, `passive_scalar_transport.feature` -- the required CLI-subprocess scenario every demo carries, and a quantitative claim that the transported field's own mass-weighted centroid moves downstream at approximately the prescribed velocity over real elapsed -time, not only that rendered pixels changed). +time, not only that rendered pixels changed; and to 54 with the Stage 4 +exit audit's own addition to `first_order_upwind_advection.feature`, +above -- the only scenario in this list added by an audit rather than by +the task that owned the criterion). **All** `make ci` targets pass, verified via the Makefile itself, not only via `uv tool run` in isolation -- that is `lint`, `typecheck`, `test`, `check-docs`, @@ -3852,20 +3861,54 @@ were first sketched. Numbered out of sequence rather than renumbered into the 023-030 run, and built first regardless, per the Build order note under the Discharge map above. -### Status as of 2026-08-28: Stage 4 complete, ten of ten criteria met +### Status as of 2026-08-28: Stage 4 complete, nine of ten criteria met + +**This line read "ten of ten" until the Stage 4 exit audit the same +day, which found three of those verdicts overstated -- one of them not +recoverable within the audit itself.** The table below is the corrected one; +the three amended rows say what was claimed, what was actually true, and +what was done about it, rather than being silently rewritten (root +`CLAUDE.md`'s Integrity section, and the precedent Stage 3's own +Criterion 8 row set). **Stage 4 still closes, at nine of ten.** +Criterion 6's shared-step-vocabulary half is the one left open: it needs +a design decision (`docs/planning/backlog.md` §13), and an audit that +picked one is deciding rather than auditing. Everything else the audit +found is fixed in its own change. Nothing here is a reason to reopen a +task -- a stage closing with a criterion honestly marked unmet and +carried forward is a better record than one closing at ten of ten +because nobody read the second half of a sentence. The audit was run under `prompts/common/AUDITOR.md`'s +stance against a green `make ci` (605 tests, 99% coverage, 53 scenarios +across 14 feature files) and a green real CI run on `main` +(33163793986). That makes four stage audits in a row -- Stages 1, 2, 3 +and 4 -- to find real defects behind a green build. Stage 0's own audit +is the single exception, and it ran before there was a real CI runner +for anything to be green on. + +The three that were wrong, and the one thing they have in common: +Criterion 4's advection-conservation scenario passed for reasons +unrelated to what it claimed to check (found by mutation, not by +reading); Criterion 6's shared-step-vocabulary half was never attempted +and the criterion had pre-declared exactly what its own failure would +look like; Criterion 10's documentation sweep checked the files Stage 4 +*touched* rather than the files Stage 4 *invalidated*. All three were +legible only to a reader asking "what would make this false?" -- and the +third is Stage 3's own audit finding recurring unchanged one stage +later, which is why this audit's new rule (`docs/practices.md`, "A +stage's documentation sweep is a grep, not a diff review") is about that +one specifically. | Criterion | Verdict | |-----------|---------| | 1. Simulation-stepping mechanism exists, face-flux accumulation uniform across every face | **Met** (TASK-040). `engine/simulation.py`'s `step()`/`accumulate_flux_to_cells` are real, unit-tested (`tests/unit/test_simulation.py`, `simulation_orchestrator.feature`), and never branch on `Mesh.is_boundary_face` -- a concrete Advection/Diffusion scheme handles a boundary face itself. | | 2. Real implementation replaces reference, under the existing MVP name | **Met.** All seven names TASK-023..029 own resolve to a real scheme (`FirstOrderUpwindAdvection`, `CentralDifferenceDiffusion`, `RK4Integrator`, `ConjugateGradientSolver`, `PISO`, `DirichletBoundaryCondition`, `NeumannBoundaryCondition`), each checked by `isinstance` in `tests/unit/numerics/test_assembly.py`, not just by the name still validating. | | 3. Contract suite still holds, shown insufficient alone | **Met.** Every real scheme joined its own interface's contract suite (`test_advection_contract.py` etc.) with no edit to any existing test body except where a real interface widening required one (`TimeIntegrator.advance`, `PressureCoupling.correct`, each its own recorded ADR); each scheme's own `.feature` file is what actually proves physical correctness, per this criterion's own "necessary and explicitly not sufficient". | -| 4. Physical correctness, per task | **Met.** Each of TASK-023..030's own Intent lines is discharged by that task's own `.feature` file -- TASK-030's own (the round-trip invariant) is checked as convergence under mesh refinement rather than exact equality at one resolution, a genuine numerical finding recorded in that task's own Design decisions, not a weaker check chosen for convenience. | +| 4. Physical correctness, per task | **Met as amended 2026-08-28; one bullet overstated as first written.** Each of TASK-023..030's own Intent lines is discharged by that task's own `.feature` file, and TASK-030's own round-trip invariant is checked as convergence under mesh refinement rather than exact equality at one resolution -- a genuine numerical finding, and mutation-verified by this audit to have real teeth (clamping the wrapped neighbour to the owner fails it: 1.052 against a 0.831 bound). **The Advection conservation bullet was not.** Its scenario ("Conservation on a closed domain") makes every boundary face's face-normal velocity zero, so every boundary flux is zero whatever face value the scheme picks, while interior faces cancel by construction inside `accumulate_flux_to_cells` -- meaning it passes for *any* flux array. Verified, not inferred: forcing every advective face flux to `0.0` leaves it passing. That is exactly the qualifier the criterion reserved ("a bounded scheme can still fail to conserve if its flux accounting is wrong, so this is not implied by the bullet above it"), and the criterion's own first-named fixture -- "a **periodic** or fully-closed domain" -- is the one that carries it. **Fixed in the audit's own change:** `first_order_upwind_advection.feature` gains "Conservation on a fully periodic domain" (all four edges periodic, velocity `(1.7, -0.9)`, no boundary condition configured at all), where every boundary face carries a genuinely nonzero flux and global cancellation is a real property of the wrap accounting. Mutation-verified in both directions: the clamped-wrap mutation fails the new scenario (total drifts 52.0 → 54.87) and leaves the old one passing. The weak scenario is kept, with its own limitation stated in the feature file rather than deleted -- it still checks that a zero-velocity boundary face contributes nothing *additively*. Diffusion's own conservation scenario was checked the same way and does have teeth (mutating its boundary-gradient branch drifts the total by 37.4). | | 5. Real implementations' own rejection paths tested | **Met.** Every `UnconfiguredBoundaryFaceError`/`IncompatibleVelocityFieldError`/`IncompatibleVectorFieldError`/`NotABoundaryFaceError` (TASK-030's own, on `wrapped_neighbour_cell`) is exercised directly against real bad input, not only inherited-untested from a shared helper -- `docs/practices.md`'s "rejection criteria stop at the constructor" checked task by task. | -| 6. Executable Gherkin criteria, `make check-scenarios` gates | **Met.** `make check-scenarios`: "All 53 scenario(s) across 14 feature file(s) are bound and run," verified directly, not assumed from the file count. | +| 6. Executable Gherkin criteria, `make check-scenarios` gates | **First half met; second half NOT met, and carried forward as a named divergence rather than claimed.** The gating half is real: `make check-scenarios` reports "All 54 scenario(s) across 14 feature file(s) are bound and run" (53 before this audit added one), verified directly. **The shared-step-vocabulary half was never attempted.** The criterion reads: "The shared step vocabulary in `tests/golden/conftest.py` gains physics-shaped additions from whichever task first needs them and is reused, not re-derived, by every task after -- a large crop of task-specific step definitions by the time this Stage closes is itself a finding against this criterion." Counted directly: `tests/golden/conftest.py` still holds 9 steps and gained **zero** physics-shaped additions; the nine Stage 4 binding modules define **109** step definitions of their own; there is no shared `conftest.py` under `tests/unit/` at all. Concretely re-derived rather than reused: the Gherkin step "a small, non-square, non-trivially-origined mesh" appears in six feature files and is implemented six separate times; `_face_normal_velocity` is duplicated verbatim in four modules, each docstring pointing at another copy ("same reasoning as X's own identically-named helper"); the Dirichlet/Neumann test doubles are declared again in seven. **This was a documented decision, not an oversight, which is the part worth recording:** `tests/unit/CLAUDE.md` states "each binding test supplies its own local steps" as a convention (written into TASK-024's entry, restated for every task after) -- and nobody reconciled it against this criterion, in either direction, at any point in the stage. Two documents disagreed for six days and both were followed. Not fixed here: reconciling them is a nine-module refactor or a criterion amendment, and which one is right is a design decision, not an audit's call. `docs/planning/backlog.md` carries it. | | 7. No `_Null*` registration survives under an implemented name | **Met, closed at TASK-029, unaffected by TASK-030** (which retires one more genuinely-dead helper, `_resolve_with_argument`, but no `_Null*` class -- there were none left). `assembly.py`'s own registration calls at the bottom of the file name only real classes. | | 8. Demonstration: Passive Scalar Transport | **Met** (TASK-030). `examples/golden-demos/passive_scalar_transport.yaml`, run via the real CLI; `tests/golden/test_passive_scalar_transport.py`'s own quantitative scenario (mass-weighted centroid displacement, tolerance measured from a real run); verified visually beyond the regression test -- rendered offscreen at increasing frame counts, the blob is seen translating and, by one full domain width of travel, wrapping around the periodic boundary. | | 9. `make ci` green on a real runner | **Met.** PR #38 (`feat/task-030-periodic-boundary`), run 33159480722: `ci (ubuntu-latest)` green in 2m57s, `ci (windows-latest)` green in 5m30s -- checked against the actual run via `gh pr checks --watch`, not inferred from the PR merging. | -| 10. Documentation matches the tree | **Met.** `make check-references`/`check-manifest`/`check-inventory`/`check-dependency-tree`/`check-docs`/`check-docs-index`/`check-graph` all pass against the tree as this task leaves it; every stale forward-reference this sweep found (two in `src/pyflow/engine/CLAUDE.md` describing periodic as still raising `UnconfiguredBoundaryFaceError`, one in `docs/planning/backlog.md` saying "no periodic boundary exists yet") was corrected in this same change, not left for a future exit audit to find. | +| 10. Documentation matches the tree | **Met as amended 2026-08-28; overstated as first written.** The mechanical half held then and holds now: `make check-references`/`check-manifest`/`check-inventory`/`check-dependency-tree`/`check-docs`/`check-docs-index`/`check-graph` all pass, and the stale forward-references TASK-030's own sweep found (two in `src/pyflow/engine/CLAUDE.md`, one in `docs/planning/backlog.md`) were genuinely fixed in that change. **The sweep's scope was wrong**, and in exactly the way Stage 3's own audit had already found once (this row's Stage 3 counterpart, `docs/architecture/overview.md`): it covered the files Stage 4 *touched*, not the files Stage 4 *invalidated*. Seven defects, all in files no Stage 4 task opened, all found by this audit and all fixed in its own change: **(a)** this criterion's own second clause -- "`engine.md`'s ... Flux entry name[s] the concrete module (TASK-040's orchestrator, for Flux)" -- was simply not done; the Flux entry named `gradient.py`/`divergence.py` and never `simulation.py`'s `accumulate_flux_to_cells`, the function that performs the "summed over each control volume's faces" its own **Represents** sentence describes. **(b)** Four entries in `engine.md` (Advection, Diffusion, Time Integration, Linear Solvers) each ended "see that module's own docstring for the {five, four, three, two} that still do" -- a decaying count of surviving `_Null*` reference classes, of which there have been **zero** since TASK-029, directly contradicting Criterion 7's own verdict two rows above. Each was written by the task that made it true and left by the four tasks that made it false. **(c)** `docs/architecture/rendering.md` still said simulation/render-frame scheduling "still does not exist, because nothing produces a timestep to schedule against ... the scheduling policy itself is **Stage 4+ work**" -- three commits after TASK-030 shipped that policy (locked step, one `simulation.step()` per rendered frame, `bootstrap.py`'s `_add_passive_scalar_transport` through `RenderWindow.run(on_frame=...)`). **(d)** `src/pyflow/bootstrap.py`'s own module docstring opened "No simulation functionality -- Stage 0's job..." twenty lines above its own `import ... simulation_step` -- the identical stale self-description `__main__.py`'s help text carried, found and fixed on the same day (commit 73ff113) by a blast-radius sweep that stopped at that one file. **The last three were found by the new rule the first four produced, run against the repository before this row was written** -- which is the only reason to trust it: **(e)** `docs/architecture/overview.md` still said "No concrete numerical *scheme* exists behind any of [the six] yet -- that is Stage 4", the second time that same file has gone stale about a stage that had already closed; **(f)** the same file, three lines further down, called the `numerics.*` configuration section "the part still missing, because the interfaces it would select among do not exist yet" -- contradicting its own bullet above it, since Stage 3 built both; **(g)** `adr/ADR-002-fvm-first.md` still carried "**What still does not exist:** a real Time Integration, Pressure-Velocity Coupling, or Linear Solver implementation", written by TASK-024 and falsified three days later by TASK-025/026/027, the tasks its own next sentence named. The rule that finds all seven is new in `docs/practices.md` ("A stage's documentation sweep is a grep, not a diff review"); (g) additionally records the narrower lesson that an ADR tracking implementation status inherits that status's maintenance burden, which is why no other ADR here does. | **Criterion 9 could not be marked Met from a local checkout alone when this table was first drafted** -- a local `make ci` pass is not the same diff --git a/docs/planning/status.md b/docs/planning/status.md index e706d12..0be73e4 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -40,8 +40,8 @@ pie showData ## Live repository facts - **45** `CLAUDE.md` files -- **605** tests collected -- **53** Gherkin scenarios (`tests/features/*.feature`) +- **606** tests collected +- **54** Gherkin scenarios (`tests/features/*.feature`) ## Stages @@ -99,7 +99,7 @@ pie showData ### Stage 4 -- First Numerical Methods -**complete, as of 2026-08-28** -- `██████░░░░` 5/9 tasks; 10/10 criteria met +**complete, as of 2026-08-28** -- `██████░░░░` 5/9 tasks; 9/10 criteria met | Task | Status | Date | Artifact | |------|--------|------|----------| diff --git a/docs/practices.md b/docs/practices.md index 1d13f96..156d6a7 100644 --- a/docs/practices.md +++ b/docs/practices.md @@ -1301,6 +1301,53 @@ and is what prevents the sweep from being needed at all -- a dedicated backlog review pass finding nothing is the goal this rule aims at, not a review pass finding a list of bugs to fix. +## A stage's documentation sweep is a grep, not a diff review + +**Standing rule, 2026-08-28, from the Stage 4 exit audit.** The rule +above converted `engine.md`'s `Implementation:` lines to checked paths +and stopped there. It was not enough, because a document says more about +status than its checked fields do -- and Stage 4 closed with four +documentation defects, every one of them in a file no Stage 4 task +opened, every one of them outside a "check every touched file" sweep: + +- `engine.md`'s Flux entry named no module for `accumulate_flux_to_cells` + -- which Stage 4 Completion Criterion 10's own text explicitly + required, and which the criterion was nonetheless marked Met against. +- Four `engine.md` entries carried "see that module's own docstring for + the {five, four, three, two} that still do", a **decaying count** of + `_Null*` reference classes. Each was correct when written and falsified + by a later task in the same stage. A count is not a tense, so the rule + above does not reach it. +- `docs/architecture/rendering.md` called simulation/frame scheduling + "Stage 4+ work" three commits after Stage 4 shipped it. +- `src/pyflow/bootstrap.py`'s module docstring said "No simulation + functionality" above its own `import ... simulation_step`. + +**The sweep at a stage boundary is over the files the stage +*invalidated*, not the files it *touched*, and the only way to find +those is to grep for the claim rather than to re-read the diff.** Before +closing a stage, grep the whole repository -- `src/` docstrings included, +not only `docs/` -- for: + +1. **the stage's own number and the next one** (`Stage 4`, `Stage 5+`, + `Stage 4 work`) -- anything scheduling work into the stage that just + closed is now either done or deferred, and must say which; +2. **every count of anything the stage changed** (`the five that still + do`, "N remaining", "both of the two") -- a hand-written count next to + a thing the stage added to or removed from is the highest-risk + sentence in the repository; +3. **the negations** (`does not exist`, `no ... yet`, `nothing that`, + `not yet built`, `still`) -- these are the sentences a stage makes + false without ever appearing in its diff. + +Each of the four defects above is caught by one of those three greps, in +seconds. None was caught by a stage's own task-by-task review, twice +running: Stage 3's audit found this exact class in +`docs/architecture/overview.md` and the fix applied then was +file-specific, so Stage 4 reproduced it in three new files. **Fix the +class, then, not the file** -- which is what this rule is, and why it is +phrased as three greps rather than as a list of documents to re-read. + --- # Design Rules diff --git a/src/pyflow/CLAUDE.md b/src/pyflow/CLAUDE.md index e1d9506..d27fab8 100644 --- a/src/pyflow/CLAUDE.md +++ b/src/pyflow/CLAUDE.md @@ -88,6 +88,22 @@ structural markers (`usage:`, `-h, --help`) the original C1a test already checked -- so a forgotten or reverted update fails `make test` instead of depending on a reviewer noticing. +**The same rule covers `bootstrap.py`'s own module docstring, added +2026-08-28 after the Stage 4 exit audit found it carrying the identical +defect the rule was written for.** That docstring still opened "No +simulation functionality -- Stage 0's job..." while the module twenty +lines below imported `simulation_step` and wired a live per-frame +stepping loop -- the same stale self-description, in the same package, +missed the same morning, because that fix's blast-radius sweep re-read +the diff instead of grepping for the claim. **A module's own docstring +is a self-description exactly as the CLI's help text is**, and every +module at this package root describes what PyFlow *does* rather than +what one task did; re-read them on the same trigger. Unlike the help +text, no test asserts this one's content -- there is no equivalent of +"the user sees this string" to assert against -- so it is covered by +`docs/practices.md`'s "A stage's documentation sweep is a grep, not a +diff review" (its third grep, the negations) instead of by a test. + **`__main__.py`'s second subcommand, `pyflow generate-config [--output PATH]` (TASK-039, added 2026-08-21)**, does not orchestrate multiple subpackages the way `bootstrap()` does -- it is a thin argparse wrapper diff --git a/src/pyflow/bootstrap.py b/src/pyflow/bootstrap.py index efd42c9..3d8bd17 100644 --- a/src/pyflow/bootstrap.py +++ b/src/pyflow/bootstrap.py @@ -1,9 +1,21 @@ """Application bootstrap (TASK-010): load configuration, initialise logging, open the rendering window, run the loop, exit cleanly. -No simulation functionality -- Stage 0's job is to prove every -engineering-infrastructure piece (D1-D3) integrates into one coherent -run, not to simulate anything. +TASK-010's own job was integration only -- prove every +engineering-infrastructure piece (D1-D3) composes into one coherent run, +not simulate anything. **That stopped being the whole of this module in +TASK-030 (Stage 4, 2026-08-28):** `_add_passive_scalar_transport` wires +a real `simulation.step()` into the render loop, one timestep per +rendered frame, whenever `config.simulation.scalar_pattern` is set. +Every other configuration still renders without stepping anything. + +This docstring read "No simulation functionality -- Stage 0's job..." +until the 2026-08-28 Stage 4 exit audit, in a module that by then +imported `simulation_step` twenty lines below -- the same stale +self-description `__main__.py`'s own help text carried, found the same +day but swept only as far as that file (see `src/pyflow/CLAUDE.md`'s +own rule on keeping the CLI's self-description current, which this +module is now covered by too). Lives at the `pyflow` package root, not inside `engine/`, deliberately: it composes `configuration`, `engine` (for logging) and `rendering` diff --git a/src/pyflow/engine/CLAUDE.md b/src/pyflow/engine/CLAUDE.md index 76a0e6c..0236105 100644 --- a/src/pyflow/engine/CLAUDE.md +++ b/src/pyflow/engine/CLAUDE.md @@ -211,7 +211,8 @@ cell-count-shaped tensor allocation directly on `Field`, which would have silently committed the interface to collocated (cell-centred) storage despite its own stated promise not to assume any particular arrangement; caught before landing, not after (`docs/CHANGELOG-DESIGN.md`, -2026-08-21). `CollocatedField` (TASK-015, not yet built) is where actual +2026-08-21). `CollocatedField` (TASK-015, not yet built when TASK-014 +landed; built the same day) is where actual storage, generic callable-based initialisation, and value access will live, shared by `ScalarField` and `VectorField` alike. @@ -430,12 +431,37 @@ at all -- the upstream value is simply the owner's. existing parametrised suite (Stage 4 Completion Criterion 3) with no edit to any existing test body there; its own physical-correctness claims (boundedness, the CFL-limit stable/unstable pair, conservation on -a closed domain) are `tests/features/first_order_upwind_advection.feature`, +a closed domain **and, since the 2026-08-28 Stage 4 exit audit, on a +fully periodic one**) are `tests/features/first_order_upwind_advection.feature`, bound by `tests/unit/test_first_order_upwind_advection.py` -- not a golden demo, the same "lives in `tests/unit/`, not `tests/golden/`" shape `simulation_orchestrator.feature`/`test_simulation.py` (TASK-040) already established. +**Its two conservation scenarios are not equally strong, and the feature +file says which is which** (2026-08-28, Stage 4 exit audit). The +closed-domain one gives every boundary cell zero velocity, so every +boundary face's flux is `velocity_normal * phi = 0` regardless of the +face value the scheme picks, and interior faces cancel by construction +inside `accumulate_flux_to_cells` -- it passes for any flux array +whatsoever (verified by forcing every flux to `0.0`, not inferred). The +fully-periodic one, added by that audit, is the one with teeth: every +boundary face carries a real nonzero flux, and a periodic pair cancels +globally only because both of its faces resolve the same upstream cell, +which a wrapped-neighbour lookup does and a clamped or mirrored one does +not. **Keep both, and keep the annotation** -- deleting the weak one +would lose the "a zero-velocity boundary face contributes nothing +*additively*" check it does still make, and deleting the annotation +would leave a future reader trusting it for more than it proves. The +general shape, worth carrying to any conservation check written here: +**on a mesh with uniform volumes, an antisymmetric flux accumulator +makes interior conservation structural, so a conservation scenario tests +only what happens at the boundary** -- pick a fixture whose boundary +term is nonzero, or the scenario tests nothing at all. +`CentralDifferenceDiffusion`'s own conservation scenario passes this +bar for the opposite reason (no velocity factor in front of its boundary +flux to zero the term out) and was mutation-confirmed to have teeth. + **`CentralDifferenceDiffusion`** (TASK-024, done 2026-08-27, Stage 4's third task) is `diffusion.py`'s first real concrete scheme, the second of the six `adr/ADR-003` components to go real. Constructed with diff --git a/tests/features/first_order_upwind_advection.feature b/tests/features/first_order_upwind_advection.feature index 3b77b3b..77c9ca4 100644 --- a/tests/features/first_order_upwind_advection.feature +++ b/tests/features/first_order_upwind_advection.feature @@ -61,8 +61,37 @@ Feature: First-order Upwind Advection When the field is advanced for several timesteps at twice the CFL limit Then the field's magnitude grows far beyond its initial maximum + # Stage 4 Completion Criterion 4 asks for conservation "on a periodic + # or fully-closed domain". This is the fully-closed half, and it is + # deliberately weak: every boundary face here has zero face-normal + # velocity, so its flux is zero whatever face value the scheme picks, + # and interior faces cancel by construction inside + # `accumulate_flux_to_cells`. Verified by mutation at the 2026-08-28 + # Stage 4 exit audit: forcing every advective face flux to 0.0 leaves + # this scenario passing. What it does still check is that a + # zero-velocity boundary face contributes nothing *additively* -- a + # scheme that added a boundary value rather than multiplying by the + # face-normal velocity would fail it. The periodic half below is what + # carries the criterion's own "a bounded scheme can still fail to + # conserve if its flux accounting is wrong" qualifier. Scenario: Conservation on a closed domain Given a domain whose boundary cells all have zero velocity And interior cells with a nonzero velocity When the field is advanced for many timesteps Then the field's total summed over every cell is unchanged to floating-point tolerance + + # The periodic half, added by the 2026-08-28 Stage 4 exit audit. Here + # every boundary face carries a genuinely nonzero flux, and + # conservation is a real property of the wrap accounting rather than a + # structural guarantee: `accumulate_flux_to_cells` credits a periodic + # face's contribution to its owner only (the mesh reports no + # neighbour), so the two faces of a periodic pair cancel globally only + # if both resolve the same upstream cell -- which a wrapped-neighbour + # lookup does and a mirrored or clamped one does not. Verified by + # mutation, not assumed: clamping `neighbour` to the owner at a + # periodic face makes the total drift and fails this scenario, while + # leaving the closed-domain scenario above passing. + Scenario: Conservation on a fully periodic domain + Given a fully periodic domain and a velocity aligned with neither mesh axis + When the field is advanced for many timesteps + Then the field's total summed over every cell is unchanged to floating-point tolerance diff --git a/tests/unit/CLAUDE.md b/tests/unit/CLAUDE.md index 9dd4025..86ffab2 100644 --- a/tests/unit/CLAUDE.md +++ b/tests/unit/CLAUDE.md @@ -54,7 +54,12 @@ is the second**, binding `tests/features/first_order_upwind_advection.feature` claims (bounded, conservative on a closed domain, not the same as stable). Same shape as `test_simulation.py`: its own `_Context` dataclass, its own local test-only `BoundaryCondition` doubles, no -golden-demo config file or CLI run. +golden-demo config file or CLI run. **A second conservation scenario, on +a fully periodic domain, was added 2026-08-28 by the Stage 4 exit audit +because the closed-domain one turned out to pass for any flux array +whatsoever** -- see `src/pyflow/engine/CLAUDE.md`'s +`FirstOrderUpwindAdvection` entry for why, and for the general rule it +produced about what a conservation scenario can and cannot test. **`test_central_difference_diffusion.py` (TASK-024, added 2026-08-27) is the third**, binding `tests/features/central_difference_diffusion.feature` @@ -204,3 +209,30 @@ to check this) drops by only roughly 16% and stays several times larger throughout -- the scenario's own two-thirds bound was chosen to separate those two measured outcomes, not guessed, and confirmed to actually fail under that same mutation before being trusted. + +**The "each binding test supplies its own local steps" convention this +file has restated for every module above is in unresolved conflict with +Stage 4 Completion Criterion 6, and neither document knew it** (found +2026-08-28, Stage 4 exit audit). That criterion says the shared step +vocabulary in `tests/golden/conftest.py` "gains physics-shaped additions +from whichever task first needs them and is reused, not re-derived, by +every task after", and adds that "a large crop of task-specific step +definitions by the time this Stage closes is itself a finding against +this criterion". Measured at the stage boundary: `tests/golden/ +conftest.py` gained zero physics-shaped steps and still holds 9; the +nine modules listed above define 109 between them; "a small, non-square, +non-trivially-origined mesh" is implemented six separate times, and +`_face_normal_velocity` four, each copy's docstring pointing at another +copy. + +Both positions have a real argument. The convention's: a step definition +shared across nine modules couples nine tasks' fixtures together, and +this repository has already shipped a bug that a shared fixture's +coincidence hid (`docs/practices.md`, "Verify a conversion where its +factors are distinct"). The criterion's: 109 hand-maintained steps is +the point at which a step vocabulary stops reading as acceptance +criteria. **Until it is resolved (`docs/planning/backlog.md` §13, due +before Stage 5's criteria are drafted), keep following the convention +here** -- do not start half-sharing, which would give the coupling +without the readability. Whoever resolves it amends whichever of the two +documents loses, in the same change, and says so in the other. diff --git a/tests/unit/test_first_order_upwind_advection.py b/tests/unit/test_first_order_upwind_advection.py index cba30f8..19aa52e 100644 --- a/tests/unit/test_first_order_upwind_advection.py +++ b/tests/unit/test_first_order_upwind_advection.py @@ -13,6 +13,7 @@ import math from dataclasses import dataclass +from dataclasses import field as dataclass_field from typing import Literal import torch @@ -78,6 +79,7 @@ class _Context: scalar: ScalarField velocity: VectorField boundary_conditions: dict[str, BoundaryCondition] + periodic_pairs: dict[str, str] = dataclass_field(default_factory=dict) flux: torch.Tensor | None = None error: Exception | None = None target_face: int | None = None @@ -98,7 +100,7 @@ def _zero_gradient_everywhere() -> dict[str, BoundaryCondition]: def _run_flux(ctx: _Context) -> None: - scheme = FirstOrderUpwindAdvection(ctx.boundary_conditions, {}) + scheme = FirstOrderUpwindAdvection(ctx.boundary_conditions, ctx.periodic_pairs) try: ctx.flux = scheme.flux(ctx.scalar, ctx.velocity) except UnconfiguredBoundaryFaceError as exc: @@ -236,6 +238,47 @@ def _given_interior_velocity(ctx: _Context) -> None: ctx.velocity.set_value_at(ctx.mesh.cell_id(i, j), (1.7, -0.9)) +@given( + "a fully periodic domain and a velocity aligned with neither mesh axis", + target_fixture="ctx", +) +def _given_fully_periodic_domain() -> _Context: + """Every edge periodic, so every boundary face carries a genuinely + nonzero flux -- unlike the closed-domain fixture above, whose + zero-velocity boundary cells make every boundary flux zero whatever + face value the scheme picks. + + Conservation here is a real property of the wrap accounting: + `accumulate_flux_to_cells` credits a periodic face's contribution to + its owner alone (the mesh reports no neighbour), so the two faces of + a periodic pair cancel globally only if both resolve the same + upstream cell. A wrapped-neighbour lookup does; a mirrored or + clamped one does not, and the total then drifts. No boundary + condition is configured at all -- a periodic face must never consult + one, so an empty mapping is the fixture that proves it doesn't. + """ + mesh = StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(4, 3)) + # Neither component zero and the two unequal, so the wrap is + # exercised on both axes at once and no coincidence of dx == dy or + # u == v can make a wrong accounting agree with a right one + # (`docs/practices.md`, "Verify a conversion where its factors are + # distinct"). + velocity = VectorField(mesh, "velocity", num_components=2, initial_value=(1.7, -0.9)) + scalar = ScalarField(mesh, "temperature") + initial_values = [3.0, 1.0, 4.0, 1.0, 5.0, 9.0, 2.0, 6.0, 5.0, 3.0, 5.0, 8.0] + for cell, value in enumerate(initial_values): + scalar.set_value_at(cell, value) + ctx = _Context( + mesh=mesh, + scalar=scalar, + velocity=velocity, + boundary_conditions={}, + periodic_pairs={"north": "south", "south": "north", "east": "west", "west": "east"}, + ) + ctx.history = [scalar.values.clone()] + return ctx + + # -- When ------------------------------------------------------------------ @@ -269,7 +312,7 @@ def _when_advanced_many(ctx: _Context) -> None: def _advance(ctx: _Context, steps: int) -> None: assert ctx.dt is not None assert ctx.history is not None - scheme = FirstOrderUpwindAdvection(ctx.boundary_conditions, {}) + scheme = FirstOrderUpwindAdvection(ctx.boundary_conditions, ctx.periodic_pairs) scalar = ctx.scalar for _ in range(steps): flux = scheme.flux(scalar, ctx.velocity) From 864ea0c00f9c45c6c668f7f4a466741128db5bfa Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Fri, 28 Aug 2026 15:22:41 +0100 Subject: [PATCH 2/2] Resolve Criterion 6: the criterion named an unreachable venue Stage 4 Completion Criterion 6 asked for a shared step vocabulary in tests/golden/conftest.py, reused rather than re-derived. A conftest.py applies only to its own subtree, and all nine of Stage 4's binding modules live in tests/unit/ -- so the criterion was unbuildable as written. Verified by running it, not by reading pytest's docs: a tests/unit/ scenario using a step defined in tests/golden/conftest.py fails with StepDefinitionNotFoundError. So this was never criterion-versus-convention. The criterion named a venue that could not serve its consumers; tests/unit/CLAUDE.md's "each binding test supplies its own local steps" grew into the vacuum and hardened into a principle while the duplication accumulated. Fix the venue, share the building blocks, not the step definitions. tests/unit/_numerics.py is the tests/golden/_demo.py counterpart; all nine modules import from it. Condition doubles 9 to 0, local _face_normal_velocity 4 to 0, _west_face 4 to 0, modules carrying the mesh constants 8 to 0. Each module keeps its own _Context and step bodies, so the coupling the convention was right about is not introduced -- a shared tests/unit/conftest.py was considered and rejected for exactly that reason. Both documents amended. Stage 4 now closes at ten of ten, on a different ten than it started with. Why, in full: docs/CHANGELOG-DESIGN.md, 28-08-2026, and the Stage 4 Criterion 6 row in docs/planning/roadmap.md. Co-Authored-By: Claude Opus 5 --- docs/CHANGELOG-DESIGN.md | 68 ++++++- docs/planning/backlog.md | 68 ++++--- docs/planning/roadmap.md | 60 ++++-- docs/planning/status.md | 2 +- docs/repository-inventory.md | 3 +- docs/repository-manifest.md | 10 +- tests/unit/CLAUDE.md | 66 ++++--- tests/unit/_numerics.py | 186 ++++++++++++++++++ .../unit/test_central_difference_diffusion.py | 81 ++------ tests/unit/test_conjugate_gradient_solver.py | 43 ++-- tests/unit/test_dirichlet_boundary.py | 70 ++----- .../unit/test_first_order_upwind_advection.py | 112 +++-------- tests/unit/test_neumann_boundary.py | 70 ++----- tests/unit/test_periodic_boundary.py | 66 ++----- tests/unit/test_piso_pressure_coupling.py | 12 +- tests/unit/test_rk4_time_integration.py | 12 +- tests/unit/test_simulation.py | 34 +--- tools/validators/check_references.py | 9 + 18 files changed, 518 insertions(+), 454 deletions(-) create mode 100644 tests/unit/_numerics.py diff --git a/docs/CHANGELOG-DESIGN.md b/docs/CHANGELOG-DESIGN.md index d504168..7d820db 100644 --- a/docs/CHANGELOG-DESIGN.md +++ b/docs/CHANGELOG-DESIGN.md @@ -5762,8 +5762,74 @@ than the exact round-trip equality the criterion's headline asks for -- was mutation-verified and holds: a clamped wrap gives 1.052 against a 0.831 bound and fails it. +### Criterion 6, resolved the same day: the criterion was unbuildable + +The audit left this one open on the grounds that choosing between the +criterion and `tests/unit/CLAUDE.md`'s convention was a design decision +rather than an audit's call. Investigating it before putting the choice +to the maintainer produced a third answer that dissolved most of it. + +**Stage 4 Completion Criterion 6's shared-vocabulary bullet named +`tests/golden/conftest.py`, which cannot serve the modules that needed +it.** A `conftest.py` applies only to its own directory subtree, and all +nine of Stage 4's binding modules live in `tests/unit/`. Verified by +running it rather than by citing pytest's documentation: a +`tests/unit/` scenario using a step defined in +`tests/golden/conftest.py` fails with `StepDefinitionNotFoundError`. +The criterion was drafted 2026-08-25 assuming Stage 4's features would +be golden-demo-shaped; TASK-040 then established -- correctly, and +recorded in `tests/unit/CLAUDE.md` -- that most are unit-level, and +nobody returned to the criterion. So the two documents were never +really in disagreement: one named an unreachable venue, and the other +grew into the vacuum and hardened into a principle. + +**The convention's own argument was also weaker than it read.** It +warned that a shared fixture couples nine tasks together, which is a +real risk this repository has been bitten by. But the fixtures were +already coupled in substance and merely uncoupled in maintenance: +`origin=(0.5, -1.0), spacing=(0.2, 0.3)` byte-identical across eight +modules with only `extent` varying, `_FixedGradientCondition` across +four, `_face_normal_velocity` across four (three byte-identical), +`_west_face` across four. Eight copies of one fixture is one fixture +with eight places to fix if it is ever found degenerate -- the opposite +of what per-module copies were meant to buy. + +**Resolved, maintainer's decision: fix the venue, share the building +blocks, not the step definitions.** `tests/unit/_numerics.py` is the +`tests/golden/_demo.py` counterpart -- an in-repo precedent rather than +a new pattern -- and holds `default_mesh`, `FixedValueCondition`, +`FixedGradientCondition`, `zero_gradient_everywhere`, `west_face` and +the `face_normal_velocity` pair. All nine binding modules import from +it; each keeps its own `_Context`, its own step bodies, and any double +only it needs. Measured after the change: local condition-double +declarations 9 to 0, `_face_normal_velocity` definitions 4 to 0, +`_west_face` definitions 4 to 0, modules carrying the mesh constants 8 +to 0. The step-definition count barely moved (109 to 110, the audit's +own new scenario), which is the point -- the criterion's target was +re-derivation, not step count, and a step whose body is one call into a +shared builder is not re-derived. + +A shared `tests/unit/conftest.py` of `@given`/`@when`/`@then` +definitions was the closest reading of the criterion's literal text once +the venue was corrected, and was rejected in the same decision: a shared +step must populate a shared context, so it would force one `_Context` +type across nine modules. That is precisely the coupling the convention +was right about. **Sharing what a step is built from, rather than the +step itself, is what lets both documents be right** -- which is why the +amendment to each of them is small. + +One deliberate non-simplification, recorded because it looks like an +oversight: `face_normal_velocity_toward` takes `neighbour` explicitly +and `face_normal_velocity` derives it. Three modules want the mesh's own +neighbour; `test_periodic_boundary.py` wants the *wrapped* one, because +a periodic face has no mesh-reported neighbour at all. Collapsing the +two behind a default argument would hide the one difference that is the +whole point of the scenario making the call. + - *Verified by:* `make ci` clean on the audit branch (606 tests, 54 scenarios across 14 feature files, 99% coverage, all twelve gates); `make check-claims` returning only its two known false positives; each mutation above run and reverted individually, with the numbers quoted - read off the actual failure output rather than predicted. + read off the actual failure output rather than predicted; the + conftest-scoping claim confirmed by a throwaway probe module that was + run and then deleted, not by reading documentation. diff --git a/docs/planning/backlog.md b/docs/planning/backlog.md index 4a010f9..1f27562 100644 --- a/docs/planning/backlog.md +++ b/docs/planning/backlog.md @@ -2864,35 +2864,47 @@ design decision rather than a correction. Everything else it found is fixed in its own change -- see `docs/planning/roadmap.md`'s Stage 4 status table for the full per-criterion record. -- [ ] **Reconcile Stage 4 Completion Criterion 6's shared-step-vocabulary +- [x] **Reconcile Stage 4 Completion Criterion 6's shared-vocabulary bullet against `tests/unit/CLAUDE.md`'s "each binding test supplies - its own local steps" convention.** The two say opposite things and - both were followed for the whole of Stage 4, which is how the - criterion came to be marked Met while its second half had never - been attempted. Measured at the stage boundary: - `tests/golden/conftest.py` holds 9 steps and gained zero - physics-shaped additions; the nine Stage 4 binding modules define - 109 step definitions between them; there is no shared - `conftest.py` under `tests/unit/` at all. "A small, non-square, - non-trivially-origined mesh" is implemented six times; - `_face_normal_velocity` four times, each copy's docstring pointing - at another copy. - - **Both resolutions are legitimate and the choice is real.** The - convention has a genuine argument behind it -- a step definition - shared across nine modules couples nine tasks' fixtures together, - and this repository has already been bitten by a shared fixture - that made two formulas agree. The criterion has the opposite one: - 109 hand-maintained steps is where a step vocabulary stops being - readable as criteria. Decide which, amend the loser, and say so - where the other one is stated. Do not simply delete the criterion's - bullet -- `docs/practices.md`'s "every qualifier becomes a bullet, - or is struck" makes striking a legitimate outcome, but only as a - recorded decision. - - Due before Stage 5's own criteria are drafted, since Stage 5's - tasks are all simulation work and will inherit whichever answer - this gets. + its own local steps" convention** (raised and **closed the same + day, 2026-08-28**). Raised as a design decision between two + defensible positions; investigating it produced a third answer + that made the choice much easier. + + **The criterion was unbuildable as written.** It named + `tests/golden/conftest.py` as the shared venue, and a `conftest.py` + applies only to its own directory subtree -- while all nine of + Stage 4's binding modules live in `tests/unit/`. Verified by + running it, not by reading pytest's documentation: a `tests/unit/` + scenario using a step defined in `tests/golden/conftest.py` fails + with `StepDefinitionNotFoundError`. So the convention had not + *disagreed* with the criterion so much as filled a vacuum the + criterion left, and then hardened into a principle. + + **The convention's own argument also turned out weaker than it + looked.** It warned that sharing couples nine tasks' fixtures -- + but the fixtures were already coupled in substance: `origin=(0.5, + -1.0), spacing=(0.2, 0.3)` appeared byte-identical in eight + modules with only `extent` varying, `_FixedGradientCondition` in + four, `_face_normal_velocity` in four, `_west_face` in four. That + is not eight independent fixtures; it is one fixture with eight + places to fix if it is ever found degenerate. + + **Resolved (maintainer's decision):** fix the venue and share the + building blocks, not the step definitions. + `tests/unit/_numerics.py` -- the counterpart to + `tests/golden/_demo.py`, an in-repo precedent rather than a new + pattern -- now holds `default_mesh`, the two boundary-condition + doubles, `zero_gradient_everywhere`, `west_face` and the + `face_normal_velocity` pair; eight modules import them. Each module + keeps its own `_Context`, its own step bodies, and any double only + it needs, so the coupling the convention warned about is not + introduced. Criterion 6's bullet is amended to name the reachable + venue; `tests/unit/CLAUDE.md`'s convention is amended to "local by + default, shared where genuinely identical". A shared + `tests/unit/conftest.py` of step definitions was considered and + rejected in the same decision: it would require one `_Context` type + across nine modules. - [ ] **`docs/CHANGELOG-DESIGN.md` has no entry after 2026-08-22.** Stages 3 and 4 -- TASK-018 through TASK-030 plus TASK-040, two diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index 0ee304b..5cdda78 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -3684,12 +3684,31 @@ own identifier when it closes." only as the drafting instruction it also is (TASK-040 included -- its own step/state contract is user-observable behaviour, not architecture, so Stage 3's exemption does not carry over to it). The - shared step vocabulary in `tests/golden/conftest.py` gains - physics-shaped additions from whichever task first needs them and is - reused, not re-derived, by every task after -- a large crop of - task-specific step definitions by the time this Stage closes is - itself a finding against this criterion, the same shape of warning - Stage 6's own criteria already state for a different claim. + shared vocabulary a task's steps are built from gains physics-shaped + additions from whichever task first needs them and is reused, not + re-derived, by every task after -- a large crop of duplicated + fixtures and helpers by the time this Stage closes is itself a + finding against this criterion, the same shape of warning Stage 6's + own criteria already state for a different claim. + **Amended 2026-08-28, Stage 4 exit audit: this bullet originally + named `tests/golden/conftest.py` as the venue, which is not reachable + from where Stage 4's features actually live.** A `conftest.py` + applies only to its own directory subtree, and all nine of this + Stage's binding modules are under `tests/unit/` -- verified directly, + not reasoned about: a `tests/unit/` scenario using a step defined in + `tests/golden/conftest.py` fails with + `pytest_bdd.exceptions.StepDefinitionNotFoundError`. The criterion + was drafted on 2026-08-25 assuming this Stage's features would be + golden-demo-shaped; TASK-040 then established (correctly) that most + are unit-level, and nobody came back to this sentence. The venue is + now `tests/unit/_numerics.py`, the counterpart to + `tests/golden/_demo.py` -- and what is shared is deliberately the + *building blocks* a step is written from (fixture constants, + test-only doubles, independently-derived geometry helpers), not the + step definitions themselves, since sharing those would mean sharing + the `_Context` each module populates and coupling nine tasks' fixture + objects into one type. See `tests/unit/CLAUDE.md`, amended in the + same change, and that module's own docstring. - **Every scenario's fixture avoids a degenerate case that could make a wrong implementation agree with a right one by coincidence** -- non-square mesh, non-trivial origin, spacing that isn't 1, a @@ -3861,22 +3880,25 @@ were first sketched. Numbered out of sequence rather than renumbered into the 023-030 run, and built first regardless, per the Build order note under the Discharge map above. -### Status as of 2026-08-28: Stage 4 complete, nine of ten criteria met +### Status as of 2026-08-28: Stage 4 complete, ten of ten criteria met -**This line read "ten of ten" until the Stage 4 exit audit the same -day, which found three of those verdicts overstated -- one of them not -recoverable within the audit itself.** The table below is the corrected one; +**Three of these ten verdicts were overstated when first written, and +the Stage 4 exit audit the same day found all three.** This line briefly +read "nine of ten" between the audit's first pass and its resolution of +Criterion 6, which is the one that needed a decision rather than a +correction. The table below is the corrected one; the three amended rows say what was claimed, what was actually true, and what was done about it, rather than being silently rewritten (root `CLAUDE.md`'s Integrity section, and the precedent Stage 3's own -Criterion 8 row set). **Stage 4 still closes, at nine of ten.** -Criterion 6's shared-step-vocabulary half is the one left open: it needs -a design decision (`docs/planning/backlog.md` §13), and an audit that -picked one is deciding rather than auditing. Everything else the audit -found is fixed in its own change. Nothing here is a reason to reopen a -task -- a stage closing with a criterion honestly marked unmet and -carried forward is a better record than one closing at ten of ten -because nobody read the second half of a sentence. The audit was run under `prompts/common/AUDITOR.md`'s +Criterion 8 row set). **Stage 4 closes at ten of ten, but not the ten it started with.** +Criterion 4 gained the scenario its own qualifier had always required; +Criterion 6's bullet was found unbuildable as written and amended to a +venue that exists, with the duplication it was aimed at actually +removed; Criterion 10's seven documentation defects are fixed. Nothing +here reopened a task. The distinction worth keeping is that "ten of ten" +was true on the second pass for reasons it was not true on the first -- +the first table's ten was arrived at by not reading the second half of +three sentences. The audit was run under `prompts/common/AUDITOR.md`'s stance against a green `make ci` (605 tests, 99% coverage, 53 scenarios across 14 feature files) and a green real CI run on `main` (33163793986). That makes four stage audits in a row -- Stages 1, 2, 3 @@ -3904,7 +3926,7 @@ one specifically. | 3. Contract suite still holds, shown insufficient alone | **Met.** Every real scheme joined its own interface's contract suite (`test_advection_contract.py` etc.) with no edit to any existing test body except where a real interface widening required one (`TimeIntegrator.advance`, `PressureCoupling.correct`, each its own recorded ADR); each scheme's own `.feature` file is what actually proves physical correctness, per this criterion's own "necessary and explicitly not sufficient". | | 4. Physical correctness, per task | **Met as amended 2026-08-28; one bullet overstated as first written.** Each of TASK-023..030's own Intent lines is discharged by that task's own `.feature` file, and TASK-030's own round-trip invariant is checked as convergence under mesh refinement rather than exact equality at one resolution -- a genuine numerical finding, and mutation-verified by this audit to have real teeth (clamping the wrapped neighbour to the owner fails it: 1.052 against a 0.831 bound). **The Advection conservation bullet was not.** Its scenario ("Conservation on a closed domain") makes every boundary face's face-normal velocity zero, so every boundary flux is zero whatever face value the scheme picks, while interior faces cancel by construction inside `accumulate_flux_to_cells` -- meaning it passes for *any* flux array. Verified, not inferred: forcing every advective face flux to `0.0` leaves it passing. That is exactly the qualifier the criterion reserved ("a bounded scheme can still fail to conserve if its flux accounting is wrong, so this is not implied by the bullet above it"), and the criterion's own first-named fixture -- "a **periodic** or fully-closed domain" -- is the one that carries it. **Fixed in the audit's own change:** `first_order_upwind_advection.feature` gains "Conservation on a fully periodic domain" (all four edges periodic, velocity `(1.7, -0.9)`, no boundary condition configured at all), where every boundary face carries a genuinely nonzero flux and global cancellation is a real property of the wrap accounting. Mutation-verified in both directions: the clamped-wrap mutation fails the new scenario (total drifts 52.0 → 54.87) and leaves the old one passing. The weak scenario is kept, with its own limitation stated in the feature file rather than deleted -- it still checks that a zero-velocity boundary face contributes nothing *additively*. Diffusion's own conservation scenario was checked the same way and does have teeth (mutating its boundary-gradient branch drifts the total by 37.4). | | 5. Real implementations' own rejection paths tested | **Met.** Every `UnconfiguredBoundaryFaceError`/`IncompatibleVelocityFieldError`/`IncompatibleVectorFieldError`/`NotABoundaryFaceError` (TASK-030's own, on `wrapped_neighbour_cell`) is exercised directly against real bad input, not only inherited-untested from a shared helper -- `docs/practices.md`'s "rejection criteria stop at the constructor" checked task by task. | -| 6. Executable Gherkin criteria, `make check-scenarios` gates | **First half met; second half NOT met, and carried forward as a named divergence rather than claimed.** The gating half is real: `make check-scenarios` reports "All 54 scenario(s) across 14 feature file(s) are bound and run" (53 before this audit added one), verified directly. **The shared-step-vocabulary half was never attempted.** The criterion reads: "The shared step vocabulary in `tests/golden/conftest.py` gains physics-shaped additions from whichever task first needs them and is reused, not re-derived, by every task after -- a large crop of task-specific step definitions by the time this Stage closes is itself a finding against this criterion." Counted directly: `tests/golden/conftest.py` still holds 9 steps and gained **zero** physics-shaped additions; the nine Stage 4 binding modules define **109** step definitions of their own; there is no shared `conftest.py` under `tests/unit/` at all. Concretely re-derived rather than reused: the Gherkin step "a small, non-square, non-trivially-origined mesh" appears in six feature files and is implemented six separate times; `_face_normal_velocity` is duplicated verbatim in four modules, each docstring pointing at another copy ("same reasoning as X's own identically-named helper"); the Dirichlet/Neumann test doubles are declared again in seven. **This was a documented decision, not an oversight, which is the part worth recording:** `tests/unit/CLAUDE.md` states "each binding test supplies its own local steps" as a convention (written into TASK-024's entry, restated for every task after) -- and nobody reconciled it against this criterion, in either direction, at any point in the stage. Two documents disagreed for six days and both were followed. Not fixed here: reconciling them is a nine-module refactor or a criterion amendment, and which one is right is a design decision, not an audit's call. `docs/planning/backlog.md` carries it. | +| 6. Executable Gherkin criteria, `make check-scenarios` gates | **Met as amended 2026-08-28; the second half was unmet, and the criterion turned out to be unbuildable as written.** The gating half was always real: `make check-scenarios` reports "All 54 scenario(s) across 14 feature file(s) are bound and run" (53 before this audit added one), verified directly. **The shared-vocabulary half was never attempted, and could not have been.** It named `tests/golden/conftest.py` as the venue; a `conftest.py` applies only to its own directory subtree, and all nine of this Stage's binding modules live in `tests/unit/`. Proven, not argued: a `tests/unit/` scenario using a step defined in `tests/golden/conftest.py` fails with `StepDefinitionNotFoundError`. So this was never "criterion versus convention" -- the criterion named a venue that could not serve its own consumers, and `tests/unit/CLAUDE.md`'s "each binding test supplies its own local steps" grew into the vacuum and hardened into a principle nobody re-examined. **What the duplication actually was**, measured at the stage boundary: the mesh constants `origin=(0.5, -1.0), spacing=(0.2, 0.3)` byte-identical in eight modules with only `extent` varying; `_FixedValueCondition` in three and `_FixedGradientCondition` in four; `_face_normal_velocity` in four, three byte-identical and the fourth genuinely wider (a periodic face has no mesh-reported neighbour, so the test must pass the wrapped one in); `_west_face` in four. Eight copies of one fixture is not eight independent fixtures -- it is one fixture with eight places to fix, which is the opposite of what per-module copies were meant to buy. **Resolved in the audit's own change, maintainer's decision:** `tests/unit/_numerics.py` now holds the shared building blocks (the counterpart to `tests/golden/_demo.py`, an in-repo precedent rather than a new pattern), and all nine binding modules import from it. Measured after: local condition-double declarations 9 to **0**, local `_face_normal_velocity` definitions 4 to **0**, local `_west_face` definitions 4 to **0**, modules carrying the mesh constants 8 to **0** (one comment still quotes them, describing a hand-derived fixture). The step-definition count is essentially unchanged (109 to 110, the audit's own new scenario) and that is the point: the criterion's target was re-derivation, not step count, and a step whose body is one call into a shared builder is not re-derived. Deliberately *not* a shared `tests/unit/conftest.py` of step definitions: that would require one `_Context` type across nine modules, which is the coupling the convention was right to warn about. Each module keeps its own `_Context`, its own step bodies, and any double only it needs. The criterion bullet above is amended to name the reachable venue; `tests/unit/CLAUDE.md`'s convention is amended to "local by default, shared where genuinely identical". | | 7. No `_Null*` registration survives under an implemented name | **Met, closed at TASK-029, unaffected by TASK-030** (which retires one more genuinely-dead helper, `_resolve_with_argument`, but no `_Null*` class -- there were none left). `assembly.py`'s own registration calls at the bottom of the file name only real classes. | | 8. Demonstration: Passive Scalar Transport | **Met** (TASK-030). `examples/golden-demos/passive_scalar_transport.yaml`, run via the real CLI; `tests/golden/test_passive_scalar_transport.py`'s own quantitative scenario (mass-weighted centroid displacement, tolerance measured from a real run); verified visually beyond the regression test -- rendered offscreen at increasing frame counts, the blob is seen translating and, by one full domain width of travel, wrapping around the periodic boundary. | | 9. `make ci` green on a real runner | **Met.** PR #38 (`feat/task-030-periodic-boundary`), run 33159480722: `ci (ubuntu-latest)` green in 2m57s, `ci (windows-latest)` green in 5m30s -- checked against the actual run via `gh pr checks --watch`, not inferred from the PR merging. | diff --git a/docs/planning/status.md b/docs/planning/status.md index 0be73e4..d3e3f1e 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -99,7 +99,7 @@ pie showData ### Stage 4 -- First Numerical Methods -**complete, as of 2026-08-28** -- `██████░░░░` 5/9 tasks; 9/10 criteria met +**complete, as of 2026-08-28** -- `██████░░░░` 5/9 tasks; 10/10 criteria met | Task | Status | Date | Artifact | |------|--------|------|----------| diff --git a/docs/repository-inventory.md b/docs/repository-inventory.md index 743d8fb..45c7ee2 100644 --- a/docs/repository-inventory.md +++ b/docs/repository-inventory.md @@ -16,7 +16,7 @@ reading job and lives in the manifest. Test counts and coverage are not here either -- those come from running the suite, not from listing files. -**266 tracked files** across 45 directories; +**267 tracked files** across 45 directories; 4 are empty. ## (root) @@ -346,6 +346,7 @@ listing files. - `CLAUDE.md` - `__init__.py` +- `_numerics.py` - `test_bootstrap.py` - `test_central_difference_diffusion.py` - `test_check_claims.py` diff --git a/docs/repository-manifest.md b/docs/repository-manifest.md index afbd8dc..9c6fa51 100644 --- a/docs/repository-manifest.md +++ b/docs/repository-manifest.md @@ -864,7 +864,15 @@ above. `golden/` holds `test_field_display.py` (TASK-017), and `test_numerics_assembly.py` (TASK-021) -- since 2026-08-22 these bind the feature files rather than carrying their own assertions, with the demo-independent step vocabulary -in `golden/conftest.py` and its machinery in `golden/_demo.py`. All +in `golden/conftest.py` and its machinery in `golden/_demo.py`. +`unit/_numerics.py` (added 2026-08-28, Stage 4 exit audit) is the +`tests/unit/` counterpart to `golden/_demo.py`: the shared fixture +constants, test-only `BoundaryCondition` doubles and geometry helpers +that Stage 4's nine numerical-scheme binding modules were each carrying +their own copy of. Deliberately not a `unit/conftest.py` of shared +`pytest-bdd` steps -- see that module's own docstring and +`tests/unit/CLAUDE.md` for why the venue Stage 4 Completion Criterion 6 +originally named could not work at all. All still run their demo via the real public CLI as their primary scenario, per `docs/implementation/golden-demos.md`'s public-API rule -- `test_field_display.py` also checks exact per-cell pixel positions for a diff --git a/tests/unit/CLAUDE.md b/tests/unit/CLAUDE.md index 86ffab2..40c64f0 100644 --- a/tests/unit/CLAUDE.md +++ b/tests/unit/CLAUDE.md @@ -210,29 +210,43 @@ throughout -- the scenario's own two-thirds bound was chosen to separate those two measured outcomes, not guessed, and confirmed to actually fail under that same mutation before being trusted. -**The "each binding test supplies its own local steps" convention this -file has restated for every module above is in unresolved conflict with -Stage 4 Completion Criterion 6, and neither document knew it** (found -2026-08-28, Stage 4 exit audit). That criterion says the shared step -vocabulary in `tests/golden/conftest.py` "gains physics-shaped additions -from whichever task first needs them and is reused, not re-derived, by -every task after", and adds that "a large crop of task-specific step -definitions by the time this Stage closes is itself a finding against -this criterion". Measured at the stage boundary: `tests/golden/ -conftest.py` gained zero physics-shaped steps and still holds 9; the -nine modules listed above define 109 between them; "a small, non-square, -non-trivially-origined mesh" is implemented six separate times, and -`_face_normal_velocity` four, each copy's docstring pointing at another -copy. - -Both positions have a real argument. The convention's: a step definition -shared across nine modules couples nine tasks' fixtures together, and -this repository has already shipped a bug that a shared fixture's -coincidence hid (`docs/practices.md`, "Verify a conversion where its -factors are distinct"). The criterion's: 109 hand-maintained steps is -the point at which a step vocabulary stops reading as acceptance -criteria. **Until it is resolved (`docs/planning/backlog.md` §13, due -before Stage 5's criteria are drafted), keep following the convention -here** -- do not start half-sharing, which would give the coupling -without the readability. Whoever resolves it amends whichever of the two -documents loses, in the same change, and says so in the other. +**The convention is "local by default, shared where genuinely +identical" -- amended 2026-08-28 by the Stage 4 exit audit, which found +the older blanket form ("each binding test supplies its own local +steps") in unresolved conflict with Stage 4 Completion Criterion 6.** + +The conflict turned out not to be a real disagreement. That criterion +asked for a shared vocabulary in `tests/golden/conftest.py`, which +**cannot** serve this directory: a `conftest.py` applies only to its own +subtree, so a `tests/unit/` scenario using a step defined there fails +with `StepDefinitionNotFoundError` -- verified directly, not reasoned +about. The criterion named an unreachable venue; this convention grew +into the vacuum, and then hardened into a principle nobody re-examined +while the duplication it licensed accumulated: the mesh constants +byte-identical in eight modules, `_FixedGradientCondition` in four, +`_face_normal_velocity` in four, `_west_face` in four. Eight copies of +one fixture is one fixture with eight places to fix. + +**What this means concretely.** `tests/unit/_numerics.py` holds the +shared building blocks -- `default_mesh`, `FixedValueCondition`, +`FixedGradientCondition`, `zero_gradient_everywhere`, `west_face`, +`face_normal_velocity`/`face_normal_velocity_toward` -- the counterpart +to `tests/golden/_demo.py`, and the same underscore-prefixed +"machinery, not a test module" naming. Import from it rather than +copying. + +**Still local, and this is the load-bearing half:** each module keeps +its own `_Context` dataclass, its own `@given`/`@when`/`@then` bodies, +and any double only it needs (`test_piso_pressure_coupling.py`'s +`_ZeroNormalVelocity`, `test_periodic_boundary.py`'s +`_InertLinearSolver`/`_InertPressureCoupling`, +`test_simulation.py`'s `_EchoAdvection`). Sharing a *step definition* +would mean sharing the context it populates, coupling nine tasks' +fixture objects into one type -- which is exactly the risk the older +blanket convention was right about, and the reason the shared thing is a +helper module rather than a `tests/unit/conftest.py`. **When a fixture +detail genuinely differs, copy it rather than adding a parameter to the +shared one**: `test_periodic_boundary.py` needs +`face_normal_velocity_toward`'s explicit `neighbour` because a periodic +face has no mesh-reported neighbour, and that difference is visible at +the call site precisely because it was not hidden behind a default. diff --git a/tests/unit/_numerics.py b/tests/unit/_numerics.py new file mode 100644 index 0000000..f3b7bce --- /dev/null +++ b/tests/unit/_numerics.py @@ -0,0 +1,186 @@ +"""Shared building blocks for the Stage 4 numerical-scheme feature +bindings in this directory -- the `tests/unit/` counterpart to +`tests/golden/_demo.py`, and the same underscore-prefixed +"machinery, not a test module" naming. + +**Why this is a helper module and not a `conftest.py` of shared +`pytest-bdd` steps.** Stage 4 Completion Criterion 6 +(`docs/planning/roadmap.md`) asked for a shared step vocabulary in +`tests/golden/conftest.py`, reused rather than re-derived by every task +after. That is not reachable from here: a `conftest.py` applies only to +its own directory subtree, and every one of Stage 4's nine binding +modules lives in `tests/unit/`. Verified directly at the 2026-08-28 +Stage 4 exit audit rather than reasoned about -- a `tests/unit/` +scenario using a step defined in `tests/golden/conftest.py` fails with +`StepDefinitionNotFoundError`. The criterion named a venue that could +not serve its own consumers, which is why the convention in +`tests/unit/CLAUDE.md` grew up in its place. + +**What belongs here, and what deliberately does not.** This module holds +the *building blocks* a step definition is written from -- the fixture +constants, the test-only `BoundaryCondition` doubles, the independently +derived geometry helper -- never the `@given`/`@when`/`@then` definitions +themselves. Sharing a step definition would mean sharing the `_Context` +it populates, coupling nine tasks' fixture objects into one type; sharing +what a step is *built from* removes the duplication that can actually be +wrong without that coupling. Each module keeps its own `_Context`, its +own step bodies, and any double only it needs (`test_piso_pressure_ +coupling.py`'s `_ZeroNormalVelocity`, `test_periodic_boundary.py`'s +`_InertLinearSolver`). + +The duplication this replaced, measured at that same audit: the mesh +constants below appeared byte-identical in eight modules, with only +`extent` varying; `FixedValueCondition` in three and +`FixedGradientCondition` in four; `face_normal_velocity` in four, three +of them byte-identical. Eight copies of one fixture is not eight +independent fixtures -- it is one fixture with eight places to fix if it +is ever found degenerate, which is the opposite of what per-module +copies were meant to buy. +""" + +from __future__ import annotations + +from typing import Literal + +from pyflow.engine.field import Field +from pyflow.engine.mesh import StructuredCartesianMesh +from pyflow.engine.numerics.boundary_condition import BoundaryCondition +from pyflow.engine.vector_field import VectorField + +DEFAULT_ORIGIN = (0.5, -1.0) +"""Deliberately not `(0.0, 0.0)`: a trivial origin lets a scheme that +forgot to offset agree with one that didn't (`docs/practices.md`, +"Verify a conversion where its factors are distinct"). +""" + +DEFAULT_SPACING = (0.2, 0.3) +"""`dx != dy`, and neither is 1.0 -- so a scheme that confuses the two +axes, or that treats a face area as a cell volume, cannot pass by +coincidence. +""" + + +def default_mesh(extent: tuple[int, int] = (3, 2)) -> StructuredCartesianMesh: + """The non-square, non-trivially-origined mesh every numerical-scheme + feature in this directory is written against. + + `extent` varies per scenario (a convergence study needs more cells + than a hand-derived flux check); the origin and spacing do not, and + are shared precisely so that finding either of them degenerate is a + one-line fix rather than an eight-module sweep. + """ + return StructuredCartesianMesh(origin=DEFAULT_ORIGIN, spacing=DEFAULT_SPACING, extent=extent) + + +class FixedValueCondition(BoundaryCondition): + """The Dirichlet shape: supplies a fixed face value, ignoring the + field entirely -- the same shape `DirichletBoundaryCondition` itself + has, kept as a double so a scheme's own wiring can be exercised + without the real class under test standing in the way. + + A test whose *own* claim is that the real condition class is wired + correctly must use the real one instead (`test_dirichlet_boundary.py`, + `test_neumann_boundary.py` -- each uses a double only for the + boundary faces its scenarios do not exercise). + """ + + def __init__(self, value: float) -> None: + self._value = value + + @property + def kind(self) -> Literal["value", "gradient"]: + return "value" + + def evaluate(self, field: Field, face: int) -> float: + self._check_boundary_face(field, face) + return self._value + + +class FixedGradientCondition(BoundaryCondition): + """The Neumann shape: supplies a fixed face gradient. + + Defaults to `0.0` for the common "an insulated wall this scenario + does not care about" case -- typically the boundary faces a scenario + does *not* exercise, so that the one it does exercise is the only + thing that can move the result. Where a scenario's claim is about the + gradient's numeric value, pass a nonzero one: a zero-gradient result + is also what a boundary wired to nothing at all would silently + produce (Stage 4 Completion Criterion 4's own Neumann bullet). + + Passing a nonzero gradient also proves the opposite where that is the + claim -- `FirstOrderUpwindAdvection` never reads this number at all + for its own advective treatment (zero-order extrapolation reads only + `kind`), so an advection scenario using a deliberately odd gradient + shows the value is not what gets used. Same as + `FixedValueCondition`: a test whose own claim is that the *real* + condition class is wired correctly must use the real one instead. + """ + + def __init__(self, gradient: float = 0.0) -> None: + self._gradient = gradient + + @property + def kind(self) -> Literal["value", "gradient"]: + return "gradient" + + def evaluate(self, field: Field, face: int) -> float: + self._check_boundary_face(field, face) + return self._gradient + + +def zero_gradient_everywhere() -> dict[str, BoundaryCondition]: + """A zero-gradient condition on each of the four named edges -- the + "nothing crosses any wall" fixture several scenarios need as + background before saying something about one face in particular. + """ + condition = FixedGradientCondition() + return {"north": condition, "south": condition, "east": condition, "west": condition} + + +def west_face(mesh: StructuredCartesianMesh) -> int: + """The first face on the mesh's west edge -- the edge every + boundary-treatment scenario in this directory happens to pick, since + its canonical normal `(-1, 0)` makes "inflow" and "outflow" read + the same way in each of them. + """ + return next(f for f in range(mesh.num_faces) if mesh.boundary_face_name(f) == "west") + + +def face_normal_velocity_toward( + mesh: StructuredCartesianMesh, + velocity: VectorField, + face: int, + neighbour: int | None, +) -> float: + """The face-normal velocity at `face`, interpolating between its + owner and `neighbour` -- or using the owner's own velocity alone when + `neighbour` is `None`. + + Derived here independently rather than by calling into + `FirstOrderUpwindAdvection`, so a test's own notion of "which side is + upstream" is not circular with the implementation under test. + + `neighbour` is a parameter rather than something this function reads + off the mesh because a **periodic** face has no mesh-reported + neighbour at all -- `test_periodic_boundary.py` passes + `mesh.wrapped_neighbour_cell(face)` instead, and that difference is + the whole point of the scenario making the call. Use + `face_normal_velocity` below for every other case. + """ + owner, _reported_neighbour = mesh.face_neighbours(face) + normal_x, normal_y = mesh.face_normal(face) + owner_x, owner_y = velocity.value_at(owner) + if neighbour is None: + v_x, v_y = owner_x, owner_y + else: + neighbour_x, neighbour_y = velocity.value_at(neighbour) + v_x, v_y = (owner_x + neighbour_x) / 2, (owner_y + neighbour_y) / 2 + return v_x * normal_x + v_y * normal_y + + +def face_normal_velocity(mesh: StructuredCartesianMesh, velocity: VectorField, face: int) -> float: + """`face_normal_velocity_toward` against whichever neighbour the mesh + itself reports -- the ordinary case, interior or boundary. + """ + _owner, neighbour = mesh.face_neighbours(face) + return face_normal_velocity_toward(mesh, velocity, face, neighbour) diff --git a/tests/unit/test_central_difference_diffusion.py b/tests/unit/test_central_difference_diffusion.py index 28c4103..540a4f2 100644 --- a/tests/unit/test_central_difference_diffusion.py +++ b/tests/unit/test_central_difference_diffusion.py @@ -13,12 +13,10 @@ import math from dataclasses import dataclass, field -from typing import Literal import torch from pytest_bdd import given, scenarios, then, when -from pyflow.engine.field import Field from pyflow.engine.mesh import StructuredCartesianMesh from pyflow.engine.numerics.boundary_condition import BoundaryCondition from pyflow.engine.numerics.diffusion import ( @@ -28,6 +26,14 @@ from pyflow.engine.scalar_field import ScalarField from pyflow.engine.simulation import accumulate_flux_to_cells +from ._numerics import ( + FixedGradientCondition, + FixedValueCondition, + default_mesh, + west_face, + zero_gradient_everywhere, +) + scenarios("central_difference_diffusion.feature") _GAMMA = 2.0 @@ -40,36 +46,6 @@ # -- Test-only boundary conditions ------------------------------------ -class _FixedValueCondition(BoundaryCondition): - """The Dirichlet shape: supplies a fixed face value.""" - - def __init__(self, value: float) -> None: - self._value = value - - @property - def kind(self) -> Literal["value", "gradient"]: - return "value" - - def evaluate(self, field: Field, face: int) -> float: - self._check_boundary_face(field, face) - return self._value - - -class _FixedGradientCondition(BoundaryCondition): - """The Neumann shape: supplies a fixed face gradient.""" - - def __init__(self, gradient: float) -> None: - self._gradient = gradient - - @property - def kind(self) -> Literal["value", "gradient"]: - return "gradient" - - def evaluate(self, field: Field, face: int) -> float: - self._check_boundary_face(field, face) - return self._gradient - - # -- Fixture context ----------------------------------------------------- @@ -90,21 +66,6 @@ class _Context: spacings: list[float] = field(default_factory=list) -def _mesh() -> StructuredCartesianMesh: - # Non-"nice" origin/spacing and a non-square extent, matching every - # other contract suite's fixture in this repository. - return StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(3, 2)) - - -def _zero_gradient_everywhere() -> dict[str, BoundaryCondition]: - condition = _FixedGradientCondition(0.0) - return {"north": condition, "south": condition, "east": condition, "west": condition} - - -def _west_face(mesh: StructuredCartesianMesh) -> int: - return next(f for f in range(mesh.num_faces) if mesh.boundary_face_name(f) == "west") - - def _run_flux(ctx: _Context) -> None: scheme = CentralDifferenceDiffusion(ctx.boundary_conditions, {}, _GAMMA) try: @@ -141,9 +102,9 @@ def _exact_laplacian(x: float, y: float) -> float: @given("a small, non-square, non-trivially-origined mesh", target_fixture="ctx") def _given_default_mesh() -> _Context: - mesh = _mesh() + mesh = default_mesh() scalar = ScalarField(mesh, "temperature", initial_value=0.0) - return _Context(mesh=mesh, scalar=scalar, boundary_conditions=_zero_gradient_everywhere()) + return _Context(mesh=mesh, scalar=scalar, boundary_conditions=zero_gradient_everywhere()) @given("a non-uniform field with known values at every cell") @@ -159,10 +120,10 @@ def _given_dirichlet_boundary(ctx: _Context) -> None: for cell in range(ctx.mesh.num_cells): ctx.scalar.set_value_at(cell, 4.0) ctx.boundary_conditions = { - **_zero_gradient_everywhere(), - "west": _FixedValueCondition(ctx.prescribed_value), + **zero_gradient_everywhere(), + "west": FixedValueCondition(ctx.prescribed_value), } - ctx.target_face = _west_face(ctx.mesh) + ctx.target_face = west_face(ctx.mesh) @given("a boundary face whose condition prescribes a gradient") @@ -171,10 +132,10 @@ def _given_neumann_boundary(ctx: _Context) -> None: for cell in range(ctx.mesh.num_cells): ctx.scalar.set_value_at(cell, 4.0) ctx.boundary_conditions = { - **_zero_gradient_everywhere(), - "west": _FixedGradientCondition(ctx.prescribed_gradient), + **zero_gradient_everywhere(), + "west": FixedGradientCondition(ctx.prescribed_gradient), } - ctx.target_face = _west_face(ctx.mesh) + ctx.target_face = west_face(ctx.mesh) @given("a boundary face with no configured condition at all") @@ -187,21 +148,21 @@ def _given_no_condition(ctx: _Context) -> None: target_fixture="ctx", ) def _given_convergence_setup() -> _Context: - mesh = _mesh() + mesh = default_mesh() scalar = ScalarField(mesh, "temperature") return _Context( mesh=mesh, scalar=scalar, - boundary_conditions=_zero_gradient_everywhere(), + boundary_conditions=zero_gradient_everywhere(), resolutions=[8, 16, 32], ) @given("a domain whose boundary conditions all prescribe a zero gradient", target_fixture="ctx") def _given_zero_flux_boundaries() -> _Context: - mesh = StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(4, 3)) + mesh = default_mesh(extent=(4, 3)) scalar = ScalarField(mesh, "temperature") - ctx = _Context(mesh=mesh, scalar=scalar, boundary_conditions=_zero_gradient_everywhere()) + ctx = _Context(mesh=mesh, scalar=scalar, boundary_conditions=zero_gradient_everywhere()) return ctx @@ -230,7 +191,7 @@ def _when_measure_convergence(ctx: _Context) -> None: x, y = mesh.cell_centroid(cell) field_values.set_value_at(cell, _laplacian_eigenfunction(x, y)) - condition = _FixedValueCondition(0.0) + condition = FixedValueCondition(0.0) boundary_conditions = { "north": condition, "south": condition, diff --git a/tests/unit/test_conjugate_gradient_solver.py b/tests/unit/test_conjugate_gradient_solver.py index 86dc942..73063f0 100644 --- a/tests/unit/test_conjugate_gradient_solver.py +++ b/tests/unit/test_conjugate_gradient_solver.py @@ -25,42 +25,22 @@ from __future__ import annotations from dataclasses import dataclass -from typing import Literal import torch from pytest_bdd import given, scenarios, then, when -from pyflow.engine.field import Field from pyflow.engine.mesh import StructuredCartesianMesh -from pyflow.engine.numerics.boundary_condition import BoundaryCondition from pyflow.engine.numerics.diffusion import CentralDifferenceDiffusion from pyflow.engine.numerics.linear_solver import ConjugateGradientSolver, LinearSolverResult from pyflow.engine.scalar_field import ScalarField from pyflow.engine.simulation import accumulate_flux_to_cells -scenarios("conjugate_gradient_solver.feature") - - -class _ZeroGradientCondition(BoundaryCondition): - """The Neumann shape, fixed at zero -- "every boundary prescribes - velocity, none prescribes pressure", the configuration that leaves - the constant mode in the pressure-correction matrix's own null space - (`docs/architecture/icds.md`'s Linear Solver ICD). - """ - - @property - def kind(self) -> Literal["value", "gradient"]: - return "gradient" - - def evaluate(self, field: Field, face: int) -> float: - self._check_boundary_face(field, face) - return 0.0 - +from ._numerics import ( + FixedGradientCondition, + default_mesh, +) -def _mesh() -> StructuredCartesianMesh: - # Non-"nice" origin/spacing and a non-square extent, matching every - # other contract suite's fixture in this repository. - return StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(3, 2)) +scenarios("conjugate_gradient_solver.feature") def _build_semidefinite_matrix(mesh: StructuredCartesianMesh) -> torch.Tensor: @@ -69,8 +49,17 @@ def _build_semidefinite_matrix(mesh: StructuredCartesianMesh) -> torch.Tensor: scheme produces, negated (the raw operator is negative semi-definite; a pressure-correction system is posed as the positive semi-definite version of the same operator). + + **Zero gradient on every edge is the load-bearing choice, not a + convenient default**: it is "every boundary prescribes velocity, none + prescribes pressure" (`docs/architecture/icds.md`'s Linear Solver + ICD), the configuration that leaves the constant mode in the + pressure-correction matrix's own null space -- which is the whole + character this fixture exists to reproduce. Change it and the system + stops being semi-definite, and this task's own criterion stops being + checked. """ - condition = _ZeroGradientCondition() + condition = FixedGradientCondition() boundary_conditions = { "north": condition, "south": condition, @@ -110,7 +99,7 @@ class _Context: target_fixture="ctx", ) def _given_semidefinite_system() -> _Context: - mesh = _mesh() + mesh = default_mesh() matrix = _build_semidefinite_matrix(mesh) return _Context(matrix=matrix, rhs=torch.zeros(mesh.num_cells, dtype=torch.float64)) diff --git a/tests/unit/test_dirichlet_boundary.py b/tests/unit/test_dirichlet_boundary.py index f3b6691..8a6158e 100644 --- a/tests/unit/test_dirichlet_boundary.py +++ b/tests/unit/test_dirichlet_boundary.py @@ -16,12 +16,10 @@ import math from dataclasses import dataclass -from typing import Literal import torch from pytest_bdd import given, scenarios, then, when -from pyflow.engine.field import Field from pyflow.engine.mesh import StructuredCartesianMesh from pyflow.engine.numerics.advection import FirstOrderUpwindAdvection from pyflow.engine.numerics.boundary_condition import BoundaryCondition, DirichletBoundaryCondition @@ -29,6 +27,13 @@ from pyflow.engine.scalar_field import ScalarField from pyflow.engine.vector_field import VectorField +from ._numerics import ( + default_mesh, + face_normal_velocity, + west_face, + zero_gradient_everywhere, +) + scenarios("dirichlet_boundary.feature") _GAMMA = 2.0 @@ -39,23 +44,6 @@ """ -class _FixedGradientCondition(BoundaryCondition): - """The Neumann shape, for the three boundary faces neither scenario - in this file exercises -- not the class under test. - """ - - def __init__(self, gradient: float) -> None: - self._gradient = gradient - - @property - def kind(self) -> Literal["value", "gradient"]: - return "gradient" - - def evaluate(self, field: Field, face: int) -> float: - self._check_boundary_face(field, face) - return self._gradient - - @dataclass class _Context: mesh: StructuredCartesianMesh @@ -67,48 +55,16 @@ class _Context: prescribed_value: float | None = None -def _mesh() -> StructuredCartesianMesh: - # Non-"nice" origin/spacing and a non-square extent, matching every - # other contract suite's fixture in this repository. - return StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(3, 2)) - - -def _zero_gradient_everywhere() -> dict[str, BoundaryCondition]: - condition = _FixedGradientCondition(0.0) - return {"north": condition, "south": condition, "east": condition, "west": condition} - - -def _west_face(mesh: StructuredCartesianMesh) -> int: - return next(f for f in range(mesh.num_faces) if mesh.boundary_face_name(f) == "west") - - -def _face_normal_velocity(ctx: _Context, face: int) -> float: - """Independently derived, not calling into `FirstOrderUpwindAdvection` - itself -- the same reasoning `test_first_order_upwind_advection.py`'s - own identically-named helper states, so this test's own notion of - "upstream" isn't circular with the implementation under test. - """ - owner, neighbour = ctx.mesh.face_neighbours(face) - normal_x, normal_y = ctx.mesh.face_normal(face) - owner_x, owner_y = ctx.velocity.value_at(owner) - if neighbour is None: - v_x, v_y = owner_x, owner_y - else: - neighbour_x, neighbour_y = ctx.velocity.value_at(neighbour) - v_x, v_y = (owner_x + neighbour_x) / 2, (owner_y + neighbour_y) / 2 - return v_x * normal_x + v_y * normal_y - - # -- Given ----------------------------------------------------------------- @given("a small, non-square, non-trivially-origined mesh", target_fixture="ctx") def _given_default_mesh() -> _Context: - mesh = _mesh() + mesh = default_mesh() velocity = VectorField(mesh, "velocity", num_components=2, initial_value=(0.0, 0.0)) scalar = ScalarField(mesh, "temperature", initial_value=1.0) return _Context( - mesh=mesh, scalar=scalar, velocity=velocity, boundary_conditions=_zero_gradient_everywhere() + mesh=mesh, scalar=scalar, velocity=velocity, boundary_conditions=zero_gradient_everywhere() ) @@ -117,7 +73,7 @@ def _given_inflow_boundary(ctx: _Context) -> None: # west's canonical normal is (-1, 0); velocity (+1, 0) gives # velocity_normal = 1*(-1) = -1 -- inflow. ctx.velocity = VectorField(ctx.mesh, "velocity", num_components=2, initial_value=(1.0, 0.0)) - ctx.target_face = _west_face(ctx.mesh) + ctx.target_face = west_face(ctx.mesh) @given( @@ -131,7 +87,7 @@ def _given_real_dirichlet_for_advection(ctx: _Context) -> None: @given("a boundary face") def _given_a_boundary_face(ctx: _Context) -> None: - ctx.target_face = _west_face(ctx.mesh) + ctx.target_face = west_face(ctx.mesh) @given( @@ -141,7 +97,7 @@ def _given_a_boundary_face(ctx: _Context) -> None: def _given_real_dirichlet_for_diffusion(ctx: _Context) -> None: ctx.prescribed_value = 9.5 ctx.boundary_conditions = { - **_zero_gradient_everywhere(), + **zero_gradient_everywhere(), "west": DirichletBoundaryCondition(ctx.prescribed_value), } @@ -169,7 +125,7 @@ def _then_inflow_uses_prescribed_value(ctx: _Context) -> None: assert ctx.flux is not None assert ctx.target_face is not None assert ctx.prescribed_value is not None - velocity_normal = _face_normal_velocity(ctx, ctx.target_face) + velocity_normal = face_normal_velocity(ctx.mesh, ctx.velocity, ctx.target_face) implied = float(ctx.flux[ctx.target_face]) / velocity_normal assert implied == ctx.prescribed_value diff --git a/tests/unit/test_first_order_upwind_advection.py b/tests/unit/test_first_order_upwind_advection.py index 19aa52e..6f2d81a 100644 --- a/tests/unit/test_first_order_upwind_advection.py +++ b/tests/unit/test_first_order_upwind_advection.py @@ -1,8 +1,12 @@ """Binds `tests/features/first_order_upwind_advection.feature` (TASK-023) -- Stage 4's first real numerical scheme, and Stage 4 Completion -Criterion 4's own claim for it: bounded, and (a distinct claim) -conservative on a closed domain, with boundedness explicitly not the -same claim as stability. +Criterion 4's own claim for it: bounded, and (two distinct claims) +conservative on a closed domain and on a fully periodic one, with +boundedness explicitly not the same claim as stability. The periodic +conservation scenario was added by the 2026-08-28 Stage 4 exit audit, +which found the closed-domain one passing for any flux array whatsoever +-- see `src/pyflow/engine/CLAUDE.md`'s `FirstOrderUpwindAdvection` entry +for what a conservation scenario can and cannot test. Not a golden demo -- no config file under `examples/golden-demos/`, no CLI run. Lives here, not under `tests/golden/`, per this directory's own @@ -14,12 +18,10 @@ import math from dataclasses import dataclass from dataclasses import field as dataclass_field -from typing import Literal import torch from pytest_bdd import given, scenarios, then, when -from pyflow.engine.field import Field from pyflow.engine.mesh import StructuredCartesianMesh from pyflow.engine.numerics.advection import ( FirstOrderUpwindAdvection, @@ -30,44 +32,15 @@ from pyflow.engine.simulation import accumulate_flux_to_cells from pyflow.engine.vector_field import VectorField -scenarios("first_order_upwind_advection.feature") - - -# -- Test-only boundary conditions ------------------------------------ - - -class _FixedValueCondition(BoundaryCondition): - """The Dirichlet shape: supplies a fixed face value.""" - - def __init__(self, value: float) -> None: - self._value = value - - @property - def kind(self) -> Literal["value", "gradient"]: - return "value" - - def evaluate(self, field: Field, face: int) -> float: - self._check_boundary_face(field, face) - return self._value - - -class _FixedGradientCondition(BoundaryCondition): - """The Neumann shape: supplies a fixed face gradient -- the scheme - under test never actually reads the numeric value for its own - advective treatment (zero-order extrapolation only reads `kind`), - so any value here proves the gradient itself is not what gets used. - """ - - def __init__(self, gradient: float) -> None: - self._gradient = gradient - - @property - def kind(self) -> Literal["value", "gradient"]: - return "gradient" +from ._numerics import ( + FixedGradientCondition, + FixedValueCondition, + default_mesh, + face_normal_velocity, + zero_gradient_everywhere, +) - def evaluate(self, field: Field, face: int) -> float: - self._check_boundary_face(field, face) - return self._gradient +scenarios("first_order_upwind_advection.feature") # -- Fixture context ----------------------------------------------------- @@ -88,17 +61,6 @@ class _Context: dt: float | None = None -def _mesh() -> StructuredCartesianMesh: - # Non-"nice" origin/spacing and a non-square extent, matching every - # other contract suite's fixture in this repository. - return StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(3, 2)) - - -def _zero_gradient_everywhere() -> dict[str, BoundaryCondition]: - condition = _FixedGradientCondition(0.0) - return {"north": condition, "south": condition, "east": condition, "west": condition} - - def _run_flux(ctx: _Context) -> None: scheme = FirstOrderUpwindAdvection(ctx.boundary_conditions, ctx.periodic_pairs) try: @@ -107,32 +69,16 @@ def _run_flux(ctx: _Context) -> None: ctx.error = exc -def _face_normal_velocity(ctx: _Context, face: int) -> float: - """Independently derived (not calling into `FirstOrderUpwindAdvection` - itself) so the test's own notion of "upstream" isn't circular with - the implementation under test. - """ - owner, neighbour = ctx.mesh.face_neighbours(face) - normal_x, normal_y = ctx.mesh.face_normal(face) - owner_x, owner_y = ctx.velocity.value_at(owner) - if neighbour is None: - v_x, v_y = owner_x, owner_y - else: - neighbour_x, neighbour_y = ctx.velocity.value_at(neighbour) - v_x, v_y = (owner_x + neighbour_x) / 2, (owner_y + neighbour_y) / 2 - return v_x * normal_x + v_y * normal_y - - # -- Given ----------------------------------------------------------------- @given("a small, non-square, non-trivially-origined mesh", target_fixture="ctx") def _given_default_mesh() -> _Context: - mesh = _mesh() + mesh = default_mesh() velocity = VectorField(mesh, "velocity", num_components=2, initial_value=(0.0, 0.0)) scalar = ScalarField(mesh, "temperature", initial_value=0.0) return _Context( - mesh=mesh, scalar=scalar, velocity=velocity, boundary_conditions=_zero_gradient_everywhere() + mesh=mesh, scalar=scalar, velocity=velocity, boundary_conditions=zero_gradient_everywhere() ) @@ -159,7 +105,7 @@ def _given_outflow_boundary(ctx: _Context) -> None: @given("that boundary's own condition prescribes a value the interior cell does not have") def _given_outflow_condition(ctx: _Context) -> None: ctx.prescribed_value = 99.0 - condition = _FixedValueCondition(ctx.prescribed_value) + condition = FixedValueCondition(ctx.prescribed_value) ctx.boundary_conditions = { "north": condition, "south": condition, @@ -182,12 +128,12 @@ def _given_inflow_boundary(ctx: _Context) -> None: @given("that boundary's own condition prescribes a fixed value") def _given_inflow_value_condition(ctx: _Context) -> None: ctx.prescribed_value = 7.5 - ctx.boundary_conditions = {"west": _FixedValueCondition(ctx.prescribed_value)} + ctx.boundary_conditions = {"west": FixedValueCondition(ctx.prescribed_value)} @given("that boundary's own condition prescribes a gradient instead of a value") def _given_inflow_gradient_condition(ctx: _Context) -> None: - ctx.boundary_conditions = {"west": _FixedGradientCondition(-3.0)} + ctx.boundary_conditions = {"west": FixedGradientCondition(-3.0)} @given("that boundary has no configured condition at all") @@ -203,7 +149,7 @@ def _given_line_with_pulse() -> _Context: scalar = ScalarField(mesh, "temperature", initial_value=0.0) scalar.set_value_at(mesh.num_cells // 2, 10.0) ctx = _Context( - mesh=mesh, scalar=scalar, velocity=velocity, boundary_conditions=_zero_gradient_everywhere() + mesh=mesh, scalar=scalar, velocity=velocity, boundary_conditions=zero_gradient_everywhere() ) ctx.history = [scalar.values.clone()] return ctx @@ -211,19 +157,19 @@ def _given_line_with_pulse() -> _Context: @given("zero-gradient conditions at both ends") def _given_zero_gradient_ends(ctx: _Context) -> None: - ctx.boundary_conditions = _zero_gradient_everywhere() + ctx.boundary_conditions = zero_gradient_everywhere() @given("a domain whose boundary cells all have zero velocity", target_fixture="ctx") def _given_zero_boundary_velocity() -> _Context: - mesh = StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(4, 3)) + mesh = default_mesh(extent=(4, 3)) velocity = VectorField(mesh, "velocity", num_components=2, initial_value=(0.0, 0.0)) scalar = ScalarField(mesh, "temperature") initial_values = [3.0, 1.0, 4.0, 1.0, 5.0, 9.0, 2.0, 6.0, 5.0, 3.0, 5.0, 8.0] for cell, value in enumerate(initial_values): scalar.set_value_at(cell, value) ctx = _Context( - mesh=mesh, scalar=scalar, velocity=velocity, boundary_conditions=_zero_gradient_everywhere() + mesh=mesh, scalar=scalar, velocity=velocity, boundary_conditions=zero_gradient_everywhere() ) ctx.history = [scalar.values.clone()] return ctx @@ -257,7 +203,7 @@ def _given_fully_periodic_domain() -> _Context: condition is configured at all -- a periodic face must never consult one, so an empty mapping is the fixture that proves it doesn't. """ - mesh = StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(4, 3)) + mesh = default_mesh(extent=(4, 3)) # Neither component zero and the two unequal, so the wrap is # exercised on both axes at once and no coincidence of dx == dy or # u == v can make a wrong accounting agree with a right one @@ -338,7 +284,7 @@ def _then_bounded(ctx: _Context) -> None: owner, neighbour = ctx.mesh.face_neighbours(face) if neighbour is None: continue - velocity_normal = _face_normal_velocity(ctx, face) + velocity_normal = face_normal_velocity(ctx.mesh, ctx.velocity, face) implied = float(ctx.flux[face]) / velocity_normal owner_value = ctx.scalar.value_at(owner) neighbour_value = ctx.scalar.value_at(neighbour) @@ -361,7 +307,7 @@ def _then_outflow_uses_owner(ctx: _Context) -> None: assert ctx.flux is not None assert ctx.target_face is not None owner, _ = ctx.mesh.face_neighbours(ctx.target_face) - velocity_normal = _face_normal_velocity(ctx, ctx.target_face) + velocity_normal = face_normal_velocity(ctx.mesh, ctx.velocity, ctx.target_face) implied = float(ctx.flux[ctx.target_face]) / velocity_normal assert isinstance(ctx.scalar, ScalarField) assert implied == ctx.scalar.value_at(owner) @@ -374,7 +320,7 @@ def _then_inflow_uses_prescribed_value(ctx: _Context) -> None: assert ctx.flux is not None assert ctx.target_face is not None assert ctx.prescribed_value is not None - velocity_normal = _face_normal_velocity(ctx, ctx.target_face) + velocity_normal = face_normal_velocity(ctx.mesh, ctx.velocity, ctx.target_face) implied = float(ctx.flux[ctx.target_face]) / velocity_normal assert implied == ctx.prescribed_value @@ -384,7 +330,7 @@ def _then_inflow_extrapolates_owner(ctx: _Context) -> None: assert ctx.flux is not None assert ctx.target_face is not None owner, _ = ctx.mesh.face_neighbours(ctx.target_face) - velocity_normal = _face_normal_velocity(ctx, ctx.target_face) + velocity_normal = face_normal_velocity(ctx.mesh, ctx.velocity, ctx.target_face) implied = float(ctx.flux[ctx.target_face]) / velocity_normal assert isinstance(ctx.scalar, ScalarField) assert implied == ctx.scalar.value_at(owner) diff --git a/tests/unit/test_neumann_boundary.py b/tests/unit/test_neumann_boundary.py index 343ad2d..b8e914a 100644 --- a/tests/unit/test_neumann_boundary.py +++ b/tests/unit/test_neumann_boundary.py @@ -19,12 +19,10 @@ import math from dataclasses import dataclass -from typing import Literal import torch from pytest_bdd import given, scenarios, then, when -from pyflow.engine.field import Field from pyflow.engine.mesh import StructuredCartesianMesh from pyflow.engine.numerics.advection import FirstOrderUpwindAdvection from pyflow.engine.numerics.boundary_condition import BoundaryCondition, NeumannBoundaryCondition @@ -32,6 +30,13 @@ from pyflow.engine.scalar_field import ScalarField from pyflow.engine.vector_field import VectorField +from ._numerics import ( + default_mesh, + face_normal_velocity, + west_face, + zero_gradient_everywhere, +) + scenarios("neumann_boundary.feature") _GAMMA = 2.0 @@ -42,23 +47,6 @@ """ -class _FixedGradientCondition(BoundaryCondition): - """The Neumann shape, for the three boundary faces the diffusion - scenario in this file doesn't exercise -- not the class under test. - """ - - def __init__(self, gradient: float) -> None: - self._gradient = gradient - - @property - def kind(self) -> Literal["value", "gradient"]: - return "gradient" - - def evaluate(self, field: Field, face: int) -> float: - self._check_boundary_face(field, face) - return self._gradient - - @dataclass class _Context: mesh: StructuredCartesianMesh @@ -70,54 +58,22 @@ class _Context: prescribed_gradient: float | None = None -def _mesh() -> StructuredCartesianMesh: - # Non-"nice" origin/spacing and a non-square extent, matching every - # other contract suite's fixture in this repository. - return StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(3, 2)) - - -def _zero_gradient_everywhere() -> dict[str, BoundaryCondition]: - condition = _FixedGradientCondition(0.0) - return {"north": condition, "south": condition, "east": condition, "west": condition} - - -def _west_face(mesh: StructuredCartesianMesh) -> int: - return next(f for f in range(mesh.num_faces) if mesh.boundary_face_name(f) == "west") - - -def _face_normal_velocity(ctx: _Context, face: int) -> float: - """Independently derived, not calling into `FirstOrderUpwindAdvection` - itself -- the same reasoning `test_first_order_upwind_advection.py`'s - own identically-named helper states, so this test's own notion of - "upstream" isn't circular with the implementation under test. - """ - owner, neighbour = ctx.mesh.face_neighbours(face) - normal_x, normal_y = ctx.mesh.face_normal(face) - owner_x, owner_y = ctx.velocity.value_at(owner) - if neighbour is None: - v_x, v_y = owner_x, owner_y - else: - neighbour_x, neighbour_y = ctx.velocity.value_at(neighbour) - v_x, v_y = (owner_x + neighbour_x) / 2, (owner_y + neighbour_y) / 2 - return v_x * normal_x + v_y * normal_y - - # -- Given ----------------------------------------------------------------- @given("a small, non-square, non-trivially-origined mesh", target_fixture="ctx") def _given_default_mesh() -> _Context: - mesh = _mesh() + mesh = default_mesh() velocity = VectorField(mesh, "velocity", num_components=2, initial_value=(0.0, 0.0)) scalar = ScalarField(mesh, "temperature", initial_value=1.0) return _Context( - mesh=mesh, scalar=scalar, velocity=velocity, boundary_conditions=_zero_gradient_everywhere() + mesh=mesh, scalar=scalar, velocity=velocity, boundary_conditions=zero_gradient_everywhere() ) @given("a boundary face") def _given_a_boundary_face(ctx: _Context) -> None: - ctx.target_face = _west_face(ctx.mesh) + ctx.target_face = west_face(ctx.mesh) @given("a real Neumann boundary condition prescribing a nonzero gradient") @@ -128,7 +84,7 @@ def _given_real_neumann_condition(ctx: _Context) -> None: # correct and harmless for both. ctx.prescribed_gradient = -3.5 ctx.boundary_conditions = { - **_zero_gradient_everywhere(), + **zero_gradient_everywhere(), "west": NeumannBoundaryCondition(ctx.prescribed_gradient), } @@ -138,7 +94,7 @@ def _given_inflow_boundary(ctx: _Context) -> None: # west's canonical normal is (-1, 0); velocity (+1, 0) gives # velocity_normal = 1*(-1) = -1 -- inflow. ctx.velocity = VectorField(ctx.mesh, "velocity", num_components=2, initial_value=(1.0, 0.0)) - ctx.target_face = _west_face(ctx.mesh) + ctx.target_face = west_face(ctx.mesh) # -- When ------------------------------------------------------------------ @@ -179,6 +135,6 @@ def _then_advection_extrapolates_owner(ctx: _Context) -> None: assert ctx.flux is not None assert ctx.target_face is not None owner, _ = ctx.mesh.face_neighbours(ctx.target_face) - velocity_normal = _face_normal_velocity(ctx, ctx.target_face) + velocity_normal = face_normal_velocity(ctx.mesh, ctx.velocity, ctx.target_face) implied = float(ctx.flux[ctx.target_face]) / velocity_normal assert implied == ctx.scalar.value_at(owner) diff --git a/tests/unit/test_periodic_boundary.py b/tests/unit/test_periodic_boundary.py index 1d1d3a2..c7df02e 100644 --- a/tests/unit/test_periodic_boundary.py +++ b/tests/unit/test_periodic_boundary.py @@ -18,7 +18,6 @@ import math from dataclasses import dataclass -from typing import Literal import torch from pytest_bdd import given, scenarios, then, when @@ -36,6 +35,13 @@ from pyflow.engine.simulation import step as simulation_step from pyflow.engine.vector_field import VectorField +from ._numerics import ( + FixedGradientCondition, + default_mesh, + face_normal_velocity_toward, + west_face, +) + scenarios("periodic_boundary.feature") _GAMMA = 2.0 @@ -44,24 +50,6 @@ """ -class _FixedGradientCondition(BoundaryCondition): - """Zero-gradient, for the non-periodic boundary faces the round-trip - scenario's own mesh still needs *something* configured for (diffusion - has no inflow/outflow carve-out) -- never the mechanism under test. - """ - - def __init__(self, gradient: float = 0.0) -> None: - self._gradient = gradient - - @property - def kind(self) -> Literal["value", "gradient"]: - return "gradient" - - def evaluate(self, field: Field, face: int) -> float: - self._check_boundary_face(field, face) - return self._gradient - - class _InertLinearSolver(LinearSolver): """`AssembledNumerics` requires one to construct at all; `step` never calls it -- same precedent as `test_simulation.py`'s own double. @@ -95,12 +83,6 @@ class _Context: round_trip_errors: tuple[float, float] | None = None -def _mesh() -> StructuredCartesianMesh: - # Non-"nice" origin/spacing and a non-square extent, matching every - # other contract suite's fixture in this repository. - return StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(4, 2)) - - def _distinct_scalar(mesh: StructuredCartesianMesh) -> ScalarField: # Every cell gets a genuinely different value -- a wrong wrapped-cell # id (or a mirrored/clamped-to-owner fallback) would read a @@ -109,26 +91,6 @@ def _distinct_scalar(mesh: StructuredCartesianMesh) -> ScalarField: return ScalarField(mesh, "tracer", initial_value=lambda x, y: 10 * x + 100 * y) -def _west_face(mesh: StructuredCartesianMesh) -> int: - return next(f for f in range(mesh.num_faces) if mesh.boundary_face_name(f) == "west") - - -def _face_normal_velocity(ctx: _Context, face: int, neighbour: int | None) -> float: - """Independently derived, not calling into either scheme under test -- - same reasoning `test_neumann_boundary.py`'s own identically-named - helper states. - """ - owner, _ = ctx.mesh.face_neighbours(face) - normal_x, normal_y = ctx.mesh.face_normal(face) - owner_x, owner_y = ctx.velocity.value_at(owner) - if neighbour is None: - v_x, v_y = owner_x, owner_y - else: - neighbour_x, neighbour_y = ctx.velocity.value_at(neighbour) - v_x, v_y = (owner_x + neighbour_x) / 2, (owner_y + neighbour_y) / 2 - return v_x * normal_x + v_y * normal_y - - # -- Given ------------------------------------------------------------- @@ -137,7 +99,7 @@ def _face_normal_velocity(ctx: _Context, face: int, neighbour: int | None) -> fl target_fixture="ctx", ) def _given_default_mesh() -> _Context: - mesh = _mesh() + mesh = default_mesh(extent=(4, 2)) velocity = VectorField(mesh, "velocity", num_components=2, initial_value=(0.0, 0.0)) scalar = _distinct_scalar(mesh) return _Context( @@ -145,9 +107,9 @@ def _given_default_mesh() -> _Context: scalar=scalar, velocity=velocity, boundary_conditions={ - "north": _FixedGradientCondition(), - "south": _FixedGradientCondition(), - "east": _FixedGradientCondition(), + "north": FixedGradientCondition(), + "south": FixedGradientCondition(), + "east": FixedGradientCondition(), }, periodic_pairs={}, ) @@ -155,7 +117,7 @@ def _given_default_mesh() -> _Context: @given("a west boundary face configured periodic with its east partner") def _given_periodic_west(ctx: _Context) -> None: - ctx.target_face = _west_face(ctx.mesh) + ctx.target_face = west_face(ctx.mesh) ctx.periodic_pairs = {"west": "east", "east": "west"} ctx.wrapped_neighbour = ctx.mesh.wrapped_neighbour_cell(ctx.target_face) @@ -269,7 +231,9 @@ def _then_advection_reads_wrapped_neighbour(ctx: _Context) -> None: assert ctx.target_face is not None assert ctx.wrapped_neighbour is not None owner, _ = ctx.mesh.face_neighbours(ctx.target_face) - velocity_normal = _face_normal_velocity(ctx, ctx.target_face, ctx.wrapped_neighbour) + velocity_normal = face_normal_velocity_toward( + ctx.mesh, ctx.velocity, ctx.target_face, ctx.wrapped_neighbour + ) implied = float(ctx.flux[ctx.target_face]) / velocity_normal assert implied == ctx.scalar.value_at(ctx.wrapped_neighbour) assert implied != ctx.scalar.value_at(owner) diff --git a/tests/unit/test_piso_pressure_coupling.py b/tests/unit/test_piso_pressure_coupling.py index d7981a4..e5fd3c6 100644 --- a/tests/unit/test_piso_pressure_coupling.py +++ b/tests/unit/test_piso_pressure_coupling.py @@ -38,6 +38,10 @@ from pyflow.engine.numerics.pressure_coupling import PISO, PressureSolveDidNotConvergeError from pyflow.engine.vector_field import VectorField +from ._numerics import ( + default_mesh, +) + scenarios("piso_pressure_coupling.feature") @@ -67,12 +71,6 @@ def solve(self, matrix: torch.Tensor, rhs: torch.Tensor) -> LinearSolverResult: return LinearSolverResult(solution=torch.zeros_like(rhs), converged=False, iterations=1000) -def _mesh() -> StructuredCartesianMesh: - # Non-"nice" origin/spacing and a non-square extent, matching every - # other contract suite's fixture in this repository. - return StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(5, 4)) - - def _boundary_conditions() -> dict[str, BoundaryCondition]: condition = _ZeroNormalVelocity() return {"north": condition, "south": condition, "east": condition, "west": condition} @@ -113,7 +111,7 @@ class _Context: target_fixture="ctx", ) def _given_closed_box_mesh() -> _Context: - mesh = _mesh() + mesh = default_mesh(extent=(5, 4)) return _Context( mesh=mesh, boundary_conditions=_boundary_conditions(), diff --git a/tests/unit/test_rk4_time_integration.py b/tests/unit/test_rk4_time_integration.py index 63aa8f5..668eded 100644 --- a/tests/unit/test_rk4_time_integration.py +++ b/tests/unit/test_rk4_time_integration.py @@ -30,6 +30,8 @@ from pyflow.engine.numerics.time_integrator import RK4Integrator from pyflow.engine.scalar_field import ScalarField +from ._numerics import default_mesh + scenarios("rk4_time_integration.feature") _DECAY_RATE = 2.0 @@ -43,12 +45,6 @@ _Derivative = Callable[[Mapping[str, Field]], Mapping[str, torch.Tensor]] -def _mesh() -> StructuredCartesianMesh: - # Non-"nice" origin/spacing and a non-square extent, matching every - # other contract suite's fixture in this repository. - return StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(3, 2)) - - def _decay_derivative(rate: float) -> _Derivative: """`dy/dt = -rate * y`, evaluated at whatever state it's called with -- never the real mesh/`AdvectionScheme`/`DiffusionScheme` @@ -124,7 +120,7 @@ class _Context: target_fixture="ctx", ) def _given_recording_derivative() -> _Context: - mesh = _mesh() + mesh = default_mesh() return _Context( mesh=mesh, y0=_Y0, recording=_RecordingDerivative(_decay_derivative(_DECAY_RATE)) ) @@ -136,7 +132,7 @@ def _given_recording_derivative() -> _Context: target_fixture="ctx", ) def _given_convergence_setup() -> _Context: - mesh = _mesh() + mesh = default_mesh() return _Context(mesh=mesh, y0=_Y0, dts=[0.2, 0.1, 0.05, 0.025]) diff --git a/tests/unit/test_simulation.py b/tests/unit/test_simulation.py index 11de315..09b41ac 100644 --- a/tests/unit/test_simulation.py +++ b/tests/unit/test_simulation.py @@ -22,7 +22,6 @@ import inspect from collections.abc import Callable, Mapping from dataclasses import dataclass -from typing import Literal import torch from pytest_bdd import given, scenarios, then, when @@ -42,29 +41,17 @@ from pyflow.engine.simulation import accumulate_flux_to_cells from pyflow.engine.vector_field import VectorField +from ._numerics import ( + FixedValueCondition, + default_mesh, +) + scenarios("simulation_orchestrator.feature") # -- Test-only implementations ----------------------------------------- -class _FixedValueCondition(BoundaryCondition): - """The Dirichlet shape, same as the boundary-condition contract - suite's own test double: supplies a fixed face value. - """ - - def __init__(self, value: float) -> None: - self._value = value - - @property - def kind(self) -> Literal["value", "gradient"]: - return "value" - - def evaluate(self, field: Field, face: int) -> float: - self._check_boundary_face(field, face) - return self._value - - class _EchoAdvection(AdvectionScheme): """Boundary-aware, hand-checkable: at an interior face, the flux is the owner cell's own value; at a boundary face, it is this scheme's @@ -167,17 +154,10 @@ class _Context: accumulated: torch.Tensor | None = None -def _mesh(nx: int = 3, ny: int = 2) -> StructuredCartesianMesh: - # Non-"nice" origin/spacing and a non-square extent, matching every - # other contract suite's fixture in this repository -- a check that - # only holds for convenient numbers is not a check. - return StructuredCartesianMesh(origin=(0.5, -1.0), spacing=(0.2, 0.3), extent=(nx, ny)) - - def _assembled(boundary_value: float) -> AssembledNumerics: solver = _InertLinearSolver() return AssembledNumerics( - advection=_EchoAdvection(_FixedValueCondition(boundary_value)), + advection=_EchoAdvection(FixedValueCondition(boundary_value)), diffusion=_ZeroDiffusion(), time_integration=_EulerIntegrator(), linear_solver=solver, @@ -192,7 +172,7 @@ def _assembled(boundary_value: float) -> AssembledNumerics: @given("a mesh-sharing set of fields and an AssembledNumerics", target_fixture="ctx") def _given_default_context() -> _Context: - mesh = _mesh() + mesh = default_mesh() velocity = VectorField(mesh, "velocity", num_components=2, initial_value=(1.0, 0.0)) field = ScalarField(mesh, "temperature", initial_value=2.0) return _Context( diff --git a/tools/validators/check_references.py b/tools/validators/check_references.py index 7c475b9..b9674fe 100644 --- a/tools/validators/check_references.py +++ b/tools/validators/check_references.py @@ -79,6 +79,15 @@ "examples/golden-demos/empty_window.py", "prompts/code/CLAUDE.md", "prompts/docs/CLAUDE.md", + # Deliberately absent, and two documents say so at length: the Stage + # 4 exit audit (2026-08-28) rejected a shared `tests/unit/conftest.py` + # of `pytest-bdd` step definitions, because a shared step must + # populate a shared context and that would force one `_Context` type + # across nine binding modules. `tests/unit/_numerics.py` holds the + # shared *building blocks* instead. Named as absent in + # `docs/planning/roadmap.md`'s Stage 4 Criterion 6 and in + # `tests/unit/CLAUDE.md`; if one is ever added, delete this entry. + "tests/unit/conftest.py", } # Artifacts a roadmap task promises but has not built yet. Each entry is