From 9061579defc18591351bca7a992f9c4e01b3a2bc Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Sun, 13 Sep 2026 13:08:02 +0100 Subject: [PATCH 1/2] TASK-054: warn when the configured timestep exceeds the stability limit `stable_timestep()` has existed since Stage 5 and no live run reached it. A 64x64 lid-driven cavity at the shipped `timestep: 0.008` is 2.05x its own stability limit and diverges at step 17, with nothing said beforehand. `build_simulation_state` now computes the limit for the mesh it is about to run on and warns when the configured timestep exceeds it, naming both numbers and their ratio. It lives in `simulation_run.py` rather than `bootstrap.py` so `run`, `record` and `resume` all get it -- `record` is the path a long unattended run uses. Non-fatal by design: `stable_timestep`'s 0.25 safety factor is conservative and 32x32 at ratio 1.02 demonstrably runs to completion. `_characteristic_velocity` reads only what the configuration prescribes -- `simulation.velocity` and the `velocity.*` entries of each boundary face's `field_values`. The `velocity.` prefix filter is load-bearing and was untested until mutation testing found it: replacing it with an unconditional `True` left all eleven tests passing, while letting a declared scalar's wall value (`temperature: 300.0`) be read as a speed of 300, shrinking the limit by two orders of magnitude. That is the same scalar/velocity conflation TASK-052 fixed one layer down, reappearing in new code reading the same mapping. `test_a_declared_scalars_wall_value_is_not_read_as_a_speed` closes it and fails under that mutation. Verified by hand on all three subcommands at 64x64: configured numerics.timestep 0.008 exceeds this mesh's own stability limit 0.0039062 (2.05x) and silent at the shipped 16x16 (0.51x). Stage 9 stays `opened` rather than closing at 7-of-7: TASK-055 is drafted to carry Criterion 3's remaining half (`BoundaryFaceConfig.velocity` is still validated and then ignored) and Criterion 6's documentation grep. It carries an open design question -- wire the field or reject it -- that wants a maintainer's answer before implementation. Co-Authored-By: Claude Opus 5 --- README.md | 12 ++- docs/planning/roadmap.md | 143 ++++++++++++++++++++++++-- docs/planning/status.md | 13 +-- planning/data/features.yaml | 10 ++ src/pyflow/simulation_run.py | 93 ++++++++++++++++- tests/unit/test_simulation_run.py | 164 +++++++++++++++++++++++++++++- 6 files changed, 413 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 0d9abd4..629c85e 100644 --- a/README.md +++ b/README.md @@ -152,8 +152,16 @@ change was physics rather than a re-fitted tolerance. Its own demo is `uv run python -m pyflow run --demos sealed_box`. **TASK-053 has landed too**: a run whose frames raise now exits non-zero with the engine's own diagnostic instead of printing `pyflow exited cleanly` and returning 0, and a `--max-frames` interactive run that -blows up terminates in 22 s rather than hanging past 300. TASK-054 (the -timestep stability warning) is drafted and not yet built. It is placed before Better Numerics by dependency, +blows up terminates in 22 s rather than hanging past 300. **TASK-054 has landed as well**: a configured timestep above the +stability limit for its own mesh is now reported before the run starts, +on `run`, `record` and `resume` alike, naming the configured value, the +derived limit and their ratio. Non-fatal, and silent below the limit. + +Stage 9 is **not** closed. TASK-055 is drafted and not started: Criterion +3 asks that no configuration field be validated and then ignored, and +`BoundaryFaceConfig.velocity` still is -- validated for mutual +exclusivity and zero net flux, read by no engine code. Whether to wire it +or reject it is an open design question recorded in that task. It is placed before Better Numerics by dependency, not preference -- Stage 10's own Rayleigh-Bénard criterion measures convection between heated walls, which is not meaningful while those walls leak. diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index fc9daeb..ae7555f 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -381,12 +381,15 @@ 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): **1234 tests as of 2026-09-12**, up from 1209 on 2026-09-11 +(C1a/C1b): **1239 tests as of 2026-09-12**, up from 1209 on 2026-09-11 (TASK-052, Stage 9: 9 in `tests/unit/test_boundary_velocity.py` and 3 in `tests/golden/test_sealed_box.py` for the wall-permeability fix and its own golden demo, plus 5 the fixtures those changed gained along the way; then TASK-053: 5 in `tests/integration/test_frame_failure.py` and 3 in -`tests/unit/test_rendering.py` for a frame that raises failing the run). +`tests/unit/test_rendering.py` for a frame that raises failing the run; +then TASK-054: 5 in `tests/unit/test_simulation_run.py` for the timestep +stability warning, the fifth of them added because mutation testing found +the other four did not hold the `velocity.*` filter). Before that, 1209 on 2026-09-11 (the Stage 8 exit audit, closing the gap between three Completion Criteria and what actually checked them: 4 in @@ -12854,12 +12857,19 @@ claim. |-----------|------| | 1. One source for a boundary's normal velocity | TASK-052 | | 2. A prescribed velocity reaches the solver | TASK-052 | -| 3. No validated-then-ignored config field | TASK-052 | +| 3. No validated-then-ignored config field | TASK-052 (part), TASK-055 | | 4. A failed frame fails the run | TASK-053 | | 5. Timestep stability reported up front | TASK-054 | -| 6. Documentation matches the tree | Whichever task lands last | +| 6. Documentation matches the tree | TASK-055 | | 7. Re-baselined numbers recorded with their predecessors | TASK-052 | +Criterion 3 is split across two tasks and says so in each: TASK-052 +closed the half that was wrong (a scalar boundary value being read as a +velocity); TASK-055 owns the sweep and the still-dead +`BoundaryFaceConfig.velocity`. **The stage does not close until it +does** -- which is why TASK-055 exists as a drafted entry rather than +the stage being written up at six of seven. + ## TASK-052 — Prescribed Boundary Velocity Reaches The Schemes @@ -13130,8 +13140,7 @@ stated above rather than glossed. ## TASK-054 — Timestep Stability Warning -**Status: Not started, drafted 2026-09-12.** Will discharge Completion -Criterion 5. +**Status: Done, 2026-09-12.** Discharges Completion Criterion 5. ### Purpose @@ -13172,18 +13181,130 @@ explosion into a warning up front and a loud failure after. that -- or an embedded error-estimating integrator. See `docs/planning/backlog.md` §15. +### Artifacts Produced + +- `src/pyflow/simulation_run.py` -- `_characteristic_velocity` (the flow + speed the CFL limit is measured against, read off the configuration) + and `_warn_if_timestep_exceeds_stability_limit`, called from + `build_simulation_state` once a configuration is known to have + something to run. +- Tests: 5 in `tests/unit/test_simulation_run.py`. + ### Acceptance Criteria Prose bullets, same scope judgement as TASK-053. - The warning names the configured timestep, the stable one, and their ratio. A warning that says only "unstable" tells a user nothing they - can act on. -- Emitted on `run`, `record` and `resume` alike. + can act on. **Met** -- all three asserted separately, and verified by + hand to read: `configured numerics.timestep 0.008 exceeds this mesh's + own stability limit 0.0039062 (2.05x)`. +- Emitted on `run`, `record` and `resume` alike. **Met** -- verified by + hand on all three against a real 64x64 configuration, which is why the + check lives in `simulation_run.py` rather than `bootstrap.py`. - **Absent** below the limit, checked as its own case -- a warning that - always fires is a warning nobody reads. -- Non-fatal: the run proceeds. -- Verified by hand against the real CLI at 64x64 and at 16x16. + always fires is a warning nobody reads. **Met**, and mutation-verified + in both directions: never warning fails the first case, always warning + fails this one. +- Non-fatal: the run proceeds. **Met** -- its own test, since the whole + design decision was to warn rather than reject. +- Verified by hand against the real CLI at 64x64 and at 16x16. **Done**; + 16x16 (the shipped cavity, 0.51x the limit) is silent. + +**A fifth test exists because mutation testing found the fourth one +missing.** Replacing `_characteristic_velocity`'s own +`name.startswith("velocity.")` filter with an unconditional `True` left +every test above passing -- so nothing held the one line that stops a +declared scalar's wall value (`temperature: 300.0`) being read as a +speed of 300, which would shrink the CFL limit by two orders of +magnitude and fire this warning on configurations that are perfectly +stable. That is the same conflation TASK-052 fixed one layer down, +reappearing in the new code that reads the same mapping. +`test_a_declared_scalars_wall_value_is_not_read_as_a_speed` closes it, +and fails under that mutation. + +### Discharges + +Completion Criterion 5 in full, with one limit stated rather than +glossed: `_characteristic_velocity` reads what the *configuration* +prescribes, so it cannot see a flow the configuration does not describe +-- a buoyancy-driven plume accelerating well past anything at a +boundary, say. That is part of why the criterion asks for a warning and +`stable_timestep`'s safety factor stays conservative, rather than this +becoming a gate. + + +## TASK-055 — Every Boundary Field Reaches A Scheme Or Is Rejected + +**Status: Not started, drafted 2026-09-12.** Will discharge Completion +Criteria 3 and 6. + +### Purpose + +Close the half of Criterion 3 that TASK-052 deliberately left open, and +run this stage's own documentation grep. + +`BoundaryFaceConfig.velocity` is validated for mutual exclusivity with +`pressure` and for zero net flux (`schema.py`'s own +`_validate_boundary_conditions_jointly`) and is then read by **no engine +code at all** -- confirmed by grep, not assumed: every reader is inside +`schema.py` itself. A user who prescribes an inlet through the field the +schema documents for exactly that purpose ("the boundary-*normal* +component only, positive = outward") gets it validated for mass +conservation and then silently ignored. + +TASK-052 made the engine read a wall's normal velocity from the +per-component channel (`field_values["velocity.0"]`/`["velocity.1"]`) +instead, which is what a real configuration already uses for the lid. +That fixed the measurable defect and left two fields describing the same +quantity, one of which does nothing. + +### Dependencies + +TASK-052 (the resolver this either feeds or is rejected alongside). + +### Design question, open + +**Wire it, or reject it?** Both are defensible and the choice is a +maintainer's: + +- **Wire it.** `boundary_normal_velocity` gains `BoundaryFaceConfig. + velocity` as a source, taking precedence over (or falling back to) the + per-component channel. Makes the documented field real. Costs a + decision about which wins when both are set, and reintroduces the + per-edge/per-face tension TASK-052's own Design decision 1 records -- + `velocity` is one number per named edge, and a linear or parabolic + inlet profile varies along one. +- **Reject it.** Delete the field, or reject a configuration that sets + it, and let `field_values` be the single channel. Smaller, and removes + a second way to say one thing -- but it deletes the only field the + zero-net-flux rule can read, so that rule needs rewriting against + `field_values` in the same change. + +**Do not pick the easier one silently** (`docs/practices.md`, "Where the +intent is not clear enough to write a failing check for, stop and hold a +design session"). Either answer wants recording before implementation. + +### Acceptance Criteria + +Criterion 3's own text asks for a sweep over +`dataclasses.fields(BoundaryFaceConfig)` rather than a hand-kept list, so +a field added later is covered without anybody remembering, plus the +guard that the sweep reaches something at all -- a sweep over an empty +set passes silently +(`tests/unit/test_golden_demo_annotations.py`'s own precedent). + +Criterion 6 is this stage's documentation grep, run as a grep rather +than a diff review. **Two known items for it already**, both found +during this stage rather than at its exit: + +- `docs/architecture/icds.md`'s Boundary Conditions ICD carries a fifth + Compatibility requirement recording that `velocity` reaches no scheme. + Whichever way the design question goes, that paragraph changes. +- TASK-052's own documentation sweep added new prose beside contradicting + old prose in three places and a later grep caught it + (commit `d15c4f4`). Grep for the claims this stage made false, not for + the files it touched. --- diff --git a/docs/planning/status.md b/docs/planning/status.md index 0bc1f56..020b879 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -17,13 +17,13 @@ demand, not part of this file. ## Progress -**54/55 tasks complete (98%)** across 17 planned stages. For the full plan, including +**55/56 tasks complete (98%)** across 17 planned stages. For the full plan, including stages below not yet broken into tasks: [roadmap.md](roadmap.md). ```mermaid pie showData title "Tasks across the roadmap" - "Done" : 54 + "Done" : 55 "Not started" : 1 ``` @@ -41,12 +41,12 @@ pie showData ### Up next -**Stage 9 -- Solver & Run Integrity** is next, starting with TASK-054 (Timestep Stability Warning). +**Stage 9 -- Solver & Run Integrity** is next, starting with TASK-055 (Every Boundary Field Reaches A Scheme Or Is Rejected). ## Live repository facts - **49** `CLAUDE.md` files -- **1234** tests collected +- **1239** tests collected - **156** Gherkin scenarios (`tests/features/*.feature`) ## Stages @@ -168,13 +168,14 @@ pie showData ### Stage 9 -- Solver & Run Integrity -**no status recorded** -- `███████░░░` 2/3 tasks; 7 criteria defined, no status line yet +**no status recorded** -- `████████░░` 3/4 tasks; 7 criteria defined, no status line yet | Task | Status | Date | Artifact | |------|--------|------|----------| | TASK-052 -- Prescribed Boundary Velocity Reaches The Schemes | Done | 2026-09-12 | `examples/golden-demos/smoke_transport.yaml` | | TASK-053 -- A Failed Frame Fails The Run | Done | 2026-09-12 | `tools/validators/check_dates.py` | -| TASK-054 -- Timestep Stability Warning | Not started | | | +| TASK-054 -- Timestep Stability Warning | Done | 2026-09-12 | `engine/simulation.py` | +| TASK-055 -- Every Boundary Field Reaches A Scheme Or Is Rejected | Not started | | | ### Stage 10 -- Better Numerics diff --git a/planning/data/features.yaml b/planning/data/features.yaml index 85985c4..0104710 100644 --- a/planning/data/features.yaml +++ b/planning/data/features.yaml @@ -713,3 +713,13 @@ entities: to: stage-9 - type: depends_on to: task-034 + + - id: task-055 + name: "TASK-055 — Every Boundary Field Reaches A Scheme Or Is Rejected" + documented_in: docs/planning/roadmap.md + must_appear_in: docs/planning/roadmap.md + edges: + - type: belongs_to + to: stage-9 + - type: depends_on + to: task-052 diff --git a/src/pyflow/simulation_run.py b/src/pyflow/simulation_run.py index d97f7aa..25fbe8a 100644 --- a/src/pyflow/simulation_run.py +++ b/src/pyflow/simulation_run.py @@ -48,13 +48,16 @@ import pyflow.physics.buoyancy # noqa: F401 from pyflow.configuration.schema import MeshConfig, PyFlowConfig from pyflow.engine.field import Field -from pyflow.engine.mesh import Mesh +from pyflow.engine.logging_setup import get_logger +from pyflow.engine.mesh import Mesh, StructuredCartesianMesh from pyflow.engine.numerics.assembly import AssembledNumerics, assemble_numerics from pyflow.engine.scalar_field import ScalarField -from pyflow.engine.simulation import navier_stokes_step +from pyflow.engine.simulation import navier_stokes_step, stable_timestep from pyflow.engine.simulation import step as simulation_step from pyflow.engine.vector_field import VectorField +logger = get_logger(__name__) + _Bounds = tuple[float, float, float, float] StepMode = Literal["passive", "solved"] @@ -150,6 +153,90 @@ class SimulationState: velocity_field: VectorField | None = None +_BOUNDARY_FACE_NAMES = ("north", "south", "east", "west") +"""Local to this module, deliberately -- the same "private here rather +than imported from `schema.py`'s own identically-shaped tuple" convention +`assembly.py` already follows. +""" + + +def _characteristic_velocity(config: PyFlowConfig) -> float: + """The flow speed `stable_timestep`'s own CFL limit should be measured + against (TASK-054, Stage 9), read off the configuration. + + The largest of any prescribed wall velocity (a moving lid, an inlet) + and a prescribed uniform velocity pattern -- falling back to `1.0` + when a configuration prescribes no motion anywhere. That fallback is + a bound rather than a measurement, and is part of why this is a + warning threshold rather than a rejection. + + **Only `velocity.*` entries of `field_values` count.** That mapping + is keyed by field name and carries a declared scalar's own wall value + too (`temperature: 300.0`, say), and reading one of those as a speed + would throw the limit off by orders of magnitude -- in the direction + that warns about nothing. The same shape of conflation TASK-052 fixed + one layer down, where a scalar's boundary value was being read as a + velocity. + + It cannot see a flow the configuration does not describe -- a + buoyancy-driven plume accelerating well past anything prescribed at a + boundary, say. That is a real limit on what this warning can promise, + and the reason `stable_timestep`'s own safety factor stays + conservative rather than this becoming a gate. + """ + speeds = [0.0] + if config.simulation.velocity_pattern is not None: + speeds.append(math.hypot(*config.simulation.velocity)) + for face_name in _BOUNDARY_FACE_NAMES: + face = getattr(config.numerics.boundary_conditions, face_name) + components = [ + abs(value) for name, value in face.field_values.items() if name.startswith("velocity.") + ] + if components: + speeds.append(math.hypot(*components)) + fastest = max(speeds) + return fastest if fastest > 0.0 else 1.0 + + +def _warn_if_timestep_exceeds_stability_limit(mesh: Mesh, config: PyFlowConfig) -> None: + """Log a warning if `numerics.timestep` is above what this scheme + combination's own explicit stability limit allows on `mesh` + (TASK-054, Stage 9). + + **Warns rather than rejecting** (maintainer's call, 2026-09-12). + `stable_timestep`'s own `_STABILITY_SAFETY_FACTOR` is `0.25` against a + measured stable edge of `0.3`, so a configured timestep above the + derived limit is not automatically unstable: refining the shipped + cavity and leaving its timestep alone, 32x32 (1.02x the limit) and + 48x48 (1.54x) both run 150 steps, while 64x64 (2.05x) diverges at step + 17. Rejecting the first two would make a deliberately conservative + heuristic load-bearing. + + **Here rather than in `bootstrap.py`, so every entry point gets it.** + `build_simulation_state` is what `pyflow run`, `pyflow record` and + `pyflow resume` all go through, and `record` is the path a long + unattended run uses -- where a silent explosion costs the most. + + Called only once a configuration is known to have something to run, so + a render-only demo (Empty Mesh, Field Display) says nothing about a + timestep nothing uses. + """ + if not isinstance(mesh, StructuredCartesianMesh): + return + configured = config.numerics.timestep + limit = stable_timestep(mesh, config.fluid.viscosity, _characteristic_velocity(config)) + if configured <= limit: + return + logger.warning( + "configured numerics.timestep %g exceeds this mesh's own stability limit %.5g (%.2fx) " + "-- the run may diverge; see stable_timestep in " + "src/pyflow/engine/simulation.py for the derivation", + configured, + limit, + configured / limit, + ) + + def build_simulation_state(mesh: Mesh, config: PyFlowConfig) -> SimulationState | None: """The initial `SimulationState` for `config`, or `None` if it declares nothing that changes frame to frame (`config.fields` empty @@ -189,6 +276,8 @@ def build_simulation_state(mesh: Mesh, config: PyFlowConfig) -> SimulationState if not (run_scalar_simulation or run_velocity_only_simulation): return None + _warn_if_timestep_exceeds_stability_limit(mesh, config) + if run_scalar_simulation: fields: dict[str, Field] = dict(declared_fields) if solved: diff --git a/tests/unit/test_simulation_run.py b/tests/unit/test_simulation_run.py index 09d92bd..82bd57a 100644 --- a/tests/unit/test_simulation_run.py +++ b/tests/unit/test_simulation_run.py @@ -9,7 +9,20 @@ from __future__ import annotations -from pyflow.configuration.schema import FieldConfig, MeshConfig, PyFlowConfig, SimulationConfig +import logging + +import pytest + +from pyflow.configuration.schema import ( + BoundaryConditionsConfig, + BoundaryFaceConfig, + FieldConfig, + FluidConfig, + MeshConfig, + NumericsConfig, + PyFlowConfig, + SimulationConfig, +) from pyflow.engine.mesh import StructuredCartesianMesh from pyflow.engine.numerics.assembly import assemble_numerics from pyflow.rendering.mesh_visualization import mesh_bounding_box @@ -112,3 +125,152 @@ def test_velocity_field_from_state_reassembles_the_named_vector_field() -> None: assert velocity.name == "velocity" assert velocity.mesh is mesh + + +# -- Timestep stability warning (TASK-054, Stage 9) -------------------------- +# +# Stage 9 Completion Criterion 5. `stable_timestep` has existed since +# TASK-034 and no live path called it -- a gap Stage 5's own Criterion 12 +# verdict noted and filed rather than fixed. Measured by refining the +# shipped cavity and leaving `numerics.timestep` alone: at 64x64 the +# configured 0.008 is 2.05x the derived limit and the run diverges at +# step 17, silently, with nothing said beforehand. + +_CAVITY_VISCOSITY = 0.01 + + +def _cavity_config(cells: int, timestep: float) -> PyFlowConfig: + """The shipped lid-driven cavity's own shape at a chosen resolution -- + a unit square, so `cells` alone sets both the mesh and the spacing. + """ + return PyFlowConfig( + mesh=MeshConfig( + origin=(0.0, 0.0), spacing=(1.0 / cells, 1.0 / cells), extent=(cells, cells) + ), + numerics=NumericsConfig( + timestep=timestep, + boundary_conditions=BoundaryConditionsConfig( + north=BoundaryFaceConfig( + type="dirichlet", field_values={"velocity.0": 1.0, "velocity.1": 0.0} + ), + south=BoundaryFaceConfig(type="dirichlet"), + east=BoundaryFaceConfig(type="dirichlet"), + west=BoundaryFaceConfig(type="dirichlet"), + ), + ), + simulation=SimulationConfig(velocity_solved=True), + fluid=FluidConfig(viscosity=_CAVITY_VISCOSITY), + ) + + +def test_a_timestep_above_the_stability_limit_is_reported( + caplog: pytest.LogCaptureFixture, +) -> None: + # 64x64 with the shipped 0.008: measured at 2.05x the derived limit, + # and the resolution at which a real run diverges (at step 17). + config = _cavity_config(cells=64, timestep=0.008) + mesh = StructuredCartesianMesh.from_config(config.mesh) + + with caplog.at_level(logging.WARNING, logger="pyflow.simulation_run"): + build_simulation_state(mesh, config) + + warnings = [r for r in caplog.records if r.levelno == logging.WARNING] + assert warnings, "expected a stability warning at 2.05x the derived limit" + message = warnings[0].getMessage() + # All three numbers, per the criterion: a warning that says only + # "unstable" tells a user nothing they can act on. + assert "0.008" in message, message + assert "0.0039" in message, message + assert "2.0" in message, message + + +def test_a_timestep_below_the_stability_limit_is_not_reported( + caplog: pytest.LogCaptureFixture, +) -> None: + # The other half of the claim, and its own case: a warning that always + # fires is a warning nobody reads. 16x16 with the same 0.008 is 0.51x + # the limit -- the shipped demo's own configuration, which must stay + # silent. + config = _cavity_config(cells=16, timestep=0.008) + mesh = StructuredCartesianMesh.from_config(config.mesh) + + with caplog.at_level(logging.WARNING, logger="pyflow.simulation_run"): + build_simulation_state(mesh, config) + + assert [r for r in caplog.records if r.levelno == logging.WARNING] == [] + + +def test_the_warning_is_not_fatal(caplog: pytest.LogCaptureFixture) -> None: + config = _cavity_config(cells=64, timestep=0.008) + mesh = StructuredCartesianMesh.from_config(config.mesh) + + with caplog.at_level(logging.WARNING, logger="pyflow.simulation_run"): + state = build_simulation_state(mesh, config) + + # The run proceeds: `_STABILITY_SAFETY_FACTOR` is deliberately + # conservative (0.25 against a measured stable edge of 0.3), so a + # configured timestep over the derived limit is not automatically + # unstable -- 32x32 and 48x48 demonstrably run. + assert state is not None + assert state.mode == "solved" + + +def test_a_configuration_with_nothing_to_run_reports_nothing( + caplog: pytest.LogCaptureFixture, +) -> None: + # `build_simulation_state` returns `None` when a config declares + # nothing that changes frame to frame. There is no timestep to warn + # about in that case, and warning anyway would fire on every + # render-only demo (Empty Mesh, Field Display). + config = PyFlowConfig( + mesh=MeshConfig(origin=(0.0, 0.0), spacing=(0.01, 0.01), extent=(64, 64)), + numerics=NumericsConfig(timestep=10.0), + ) + mesh = StructuredCartesianMesh.from_config(config.mesh) + + with caplog.at_level(logging.WARNING, logger="pyflow.simulation_run"): + assert build_simulation_state(mesh, config) is None + + assert [r for r in caplog.records if r.levelno == logging.WARNING] == [] + + +def test_a_declared_scalars_wall_value_is_not_read_as_a_speed( + caplog: pytest.LogCaptureFixture, +) -> None: + """`field_values` is keyed by field name and carries a declared + scalar's own wall value alongside any velocity component's. + + **Added because mutation testing found the filter untested.** + Replacing `_characteristic_velocity`'s own `name.startswith( + "velocity.")` with an unconditional `True` left all of the tests + above passing -- so nothing held the one line that stops a + temperature of 300 being read as a speed of 300, which would shrink + the CFL limit by two orders of magnitude and make this warning fire + on configurations that are perfectly stable. The same shape of + conflation TASK-052 fixed one layer down. + """ + config = PyFlowConfig( + mesh=MeshConfig(origin=(0.0, 0.0), spacing=(0.0625, 0.0625), extent=(16, 16)), + numerics=NumericsConfig( + timestep=0.008, + boundary_conditions=BoundaryConditionsConfig( + north=BoundaryFaceConfig(type="dirichlet", field_values={"temperature": 300.0}), + south=BoundaryFaceConfig(type="dirichlet", field_values={"temperature": 300.0}), + east=BoundaryFaceConfig(type="dirichlet", field_values={"temperature": 300.0}), + west=BoundaryFaceConfig(type="dirichlet", field_values={"temperature": 300.0}), + ), + ), + fields=[FieldConfig(name="temperature", diffusion_coefficient=0.01)], + fluid=FluidConfig(viscosity=_CAVITY_VISCOSITY), + ) + mesh = StructuredCartesianMesh.from_config(config.mesh) + + # Same mesh and timestep as the shipped cavity, which is 0.51x the + # limit and silent. Read as a speed, 300 would put the CFL limit at + # dx/300 and this would warn. + with caplog.at_level(logging.WARNING, logger="pyflow.simulation_run"): + build_simulation_state(mesh, config) + + assert [r for r in caplog.records if r.levelno == logging.WARNING] == [], ( + "a declared scalar's wall value must not be read as a flow speed" + ) From 72930954d1eb11aef745e0caa43c1b1f316163bc Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Sun, 13 Sep 2026 17:16:19 +0100 Subject: [PATCH 2/2] Date TASK-054 by its commit, and close the claim it made false Merge Gate criterion 2, run as a grep rather than remembered, on the branch that was already green. **Dates.** TASK-054's commit landed 2026-09-13T12:08 UTC, and the entry dated itself the 12th -- the local wall clock at drafting time, which is exactly the failure `check_dates.py`'s own docstring now warns about ("date a change by its UTC commit time, not by the wall clock you are looking at"). The rule was written yesterday after CI rejected the opposite error; this is the same mistake in the other direction. TASK-054's status, TASK-055's drafted date, and the test-count date move to the 13th. The maintainer's warn-rather-than-reject call stays on the 12th: that decision really was taken then. **Two claims this branch made false and left standing.** `stable_timestep` being engine code no live run reaches was stated in two places outside TASK-054's own entry -- Stage 5's Criterion 12 verdict and Stage 10's Design Question Two -- both in the present tense, both now wrong. Amended rather than deleted: the Stage 5 verdict keeps what it said on 2026-08-29 and records that the one gap it chose to note rather than file has since been closed. Stage 10's question now says what TASK-054 deliberately did not do (derive the timestep) instead of restating a gap that is gone. **And one error of my own.** TASK-054's Purpose said Stage 5's verdict "noted and filed" the gap. The verdict says the opposite in as many words -- noted *rather than* filed as a violation -- and the distinction is the whole point of the surrounding paragraph, which argues that a gap recorded against no criterion is how it survives. Corrected to what the source actually says, with the reasoning Stage 5 gave, and with why the outcome was still a user's run blowing up with nothing said. Co-Authored-By: Claude Opus 5 --- docs/planning/roadmap.md | 30 +++++++++++++++++++++--------- docs/planning/status.md | 2 +- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index ae7555f..d94dbf9 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -381,7 +381,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): **1239 tests as of 2026-09-12**, up from 1209 on 2026-09-11 +(C1a/C1b): **1239 tests as of 2026-09-13**, up from 1209 on 2026-09-11 (TASK-052, Stage 9: 9 in `tests/unit/test_boundary_velocity.py` and 3 in `tests/golden/test_sealed_box.py` for the wall-permeability fix and its own golden demo, plus 5 the fixtures those changed gained along the way; @@ -7841,7 +7841,7 @@ since a local pass is not that evidence. | 9. `make ci` green on a real runner | **Met.** Two runs, both `success` on `ubuntu-latest` and `windows-latest`, read from `gh run view`'s own per-job output rather than inferred from a merged PR: `33269489214` (PR #47) and `33270312866` (its merge to `main`). *(The first table recorded this as the audit's one honest gap, correctly: it was written before either run's result was checked.)* | | 10. Documentation matches the tree, capability map included | **Not met on 2026-08-29 as first claimed; met after this audit.** Six stale claims, in six files no Stage 5 task opened -- the exact failure mode `docs/practices.md`'s "A stage's documentation sweep is a grep, not a diff review" names, and which Stage 4's own audit produced that rule after finding seven of. **`README.md`'s "Current Phase" section said "Stage 5 -- First Fluid Solver -- not yet started", on the day Stage 5 closed**, alongside "Stage 5 will solve incompressible flow" and a "most recent demonstration" that was two demos out of date. **`docs/architecture/sequences.md`** -- the document whose only stated job is "in what order do things actually happen when PyFlow runs" -- contained no mention of pressure, predictor, corrector or `navier_stokes_step` at all, and still asked to be updated "once TASK-034 lands". **`docs/architecture/rendering.md`** claimed the field-to-GPU conversion happens "not per frame and not per timestep... Stage 4 onward, not this", stale since TASK-030 and now doubly so -- the third stale claim in that one file about a stage that had already closed. **`src/pyflow/configuration/schema.py`**'s `NumericsConfig` docstring said "the other four still resolve to their own reference implementation", flatly contradicted by `assembly.py`'s own "zero `_Null*` classes remain". **`adr/ADR-003`** still described `PISO` in the present tense as a single correction pass. And **`docs/implementation/golden-demos.md`** pointed `mvp.md`'s "golden demo exists" criterion at "the Initial Golden Demo below (Capability Level 1)" -- wrong in both halves once TASK-034 landed, and contradicted by `planning/data/demos.yaml`'s own `demo-lid-driven-cavity -> capability-level-2` edge, which had been right all along. All six fixed in the audit's own change, and README's own half made mechanical rather than remembered (above). Capability map: verified directly -- `planning/data/demos.yaml`/`capabilities.yaml` already carried both demos with `validates -> capability-level-2` edges, and `docs/planning/capability-map.md` is deliberately status-free, so nothing was owed there. | | 11. `mvp.md`'s Definition of Done discharged item by item | **Met as amended 2026-08-29.** Every row of its own table holds, re-read against the tree rather than against the first table. **But neither `docs/implementation/mvp.md` nor `docs/planning/releases.md` said the MVP had been reached** -- and `releases.md` names "Reaching the MVP" as one of exactly three concrete triggers for defining a release process, with a Maintenance section instructing that it be updated "the moment any trigger condition above is met". The trigger fired when TASK-034 landed. Fixed in the audit's own change, at the maintainer's direction: `mvp.md` records the MVP as reached, and `releases.md` is rewritten with a real release process rather than a restated trigger. | -| 12. Everything this stage adds is configuration-driven, validated, documented | **Not met on 2026-08-29 as first claimed; met after this audit.** `simulation.velocity_solved` had two live paths and meant two different things. With no `scalar_pattern`, `bootstrap.py`'s `_add_solved_velocity_rendering` called `navier_stokes_step` and produced a genuinely incompressible velocity. **With a `scalar_pattern`, `_add_passive_scalar_transport` transported velocity's components like any other scalar and never pressure-corrected them** -- so a configuration saying "solved" produced a velocity that was not, chosen by whether a scalar happened to be configured, with no error and nothing rendered differently. Measured, not argued: maximum divergence sat at 9.16 -> 8.24 -> 6.95 over 1, 10 and 40 frames uncorrected, against 2.30 -> 0.47 -> 0.057 corrected. **TASK-031 and TASK-034 both knew** -- it is recorded in `src/pyflow/configuration/CLAUDE.md` and in `bootstrap.py`'s own docstrings as a "real, pre-existing gap this task did not close" -- **and it was recorded against no criterion, which is exactly how it survived this row being marked met the first time.** A gap written down in a `CLAUDE.md` is a gap somebody chose not to fix; a gap written down against a criterion is a gap the stage cannot close over. **Fixed in the audit's own change** (maintainer's call: route it through, rather than reject the combination or record it): both live paths now call `navier_stokes_step`, with a regression test asserting the scalar-plus-solved-velocity path's own divergence collapses, measured against both behaviours before its bound was chosen. Otherwise met, with one deliberate exception recorded rather than silently narrowed: run-length/steadiness stayed a validation-scenario constant, not a config field (TASK-034's own Discharges explain why neither the demos nor the direct-engine Ghia scenario need one). `fluid:`, the corrector-loop tunables, solved-vs-prescribed velocity, and per-field wall values (superseding `velocity_tangential`) are all real, validated, documented config surface. `simulation.stable_timestep` is engine code no live run reaches -- noted rather than filed as a violation, since the *capability* it serves (choosing a timestep) is configured, and the helper is a stated, documented derivation rather than a hidden one. | +| 12. Everything this stage adds is configuration-driven, validated, documented | **Not met on 2026-08-29 as first claimed; met after this audit.** `simulation.velocity_solved` had two live paths and meant two different things. With no `scalar_pattern`, `bootstrap.py`'s `_add_solved_velocity_rendering` called `navier_stokes_step` and produced a genuinely incompressible velocity. **With a `scalar_pattern`, `_add_passive_scalar_transport` transported velocity's components like any other scalar and never pressure-corrected them** -- so a configuration saying "solved" produced a velocity that was not, chosen by whether a scalar happened to be configured, with no error and nothing rendered differently. Measured, not argued: maximum divergence sat at 9.16 -> 8.24 -> 6.95 over 1, 10 and 40 frames uncorrected, against 2.30 -> 0.47 -> 0.057 corrected. **TASK-031 and TASK-034 both knew** -- it is recorded in `src/pyflow/configuration/CLAUDE.md` and in `bootstrap.py`'s own docstrings as a "real, pre-existing gap this task did not close" -- **and it was recorded against no criterion, which is exactly how it survived this row being marked met the first time.** A gap written down in a `CLAUDE.md` is a gap somebody chose not to fix; a gap written down against a criterion is a gap the stage cannot close over. **Fixed in the audit's own change** (maintainer's call: route it through, rather than reject the combination or record it): both live paths now call `navier_stokes_step`, with a regression test asserting the scalar-plus-solved-velocity path's own divergence collapses, measured against both behaviours before its bound was chosen. Otherwise met, with one deliberate exception recorded rather than silently narrowed: run-length/steadiness stayed a validation-scenario constant, not a config field (TASK-034's own Discharges explain why neither the demos nor the direct-engine Ghia scenario need one). `fluid:`, the corrector-loop tunables, solved-vs-prescribed velocity, and per-field wall values (superseding `velocity_tangential`) are all real, validated, documented config surface. `simulation.stable_timestep` is engine code no live run reaches -- noted rather than filed as a violation, since the *capability* it serves (choosing a timestep) is configured, and the helper is a stated, documented derivation rather than a hidden one. **That note stopped being true on 2026-09-13**: Stage 9's TASK-054 made `build_simulation_state` call it on every `run`, `record` and `resume`, so a configured timestep above the limit is now reported before the run rather than discovered when it diverges. The verdict above stands as written on 2026-08-29; this sentence records that the one gap it chose to note rather than file has since been closed, by a stage that existed because *not* filing gaps against criteria is how they survive. | | 13. The solver runs through ADR-003's seams, checked by substitution | **Not met on 2026-08-29 as first claimed; met after this audit.** This criterion names **two** substitution checks. The `PressureCoupling` one was built and is real. **The `LinearSolver` one -- "which reaches the timestep only through the coupling and has never been exercised end-to-end either" -- was not**: `register_linear_solver` was never called anywhere outside `assembly.py`'s own built-in registration, so a `PISO` that constructed its own `ConjugateGradientSolver` instead of using the resolved one would have passed every scenario in this repository. Confirmed by mutation, not argued: making `PISO.__init__` discard its injected solver leaves the whole suite green. **Fixed in the audit's own change** -- `navier_stokes_timestep.feature` gains a scenario registering a recording `LinearSolver` under its own name, selected through `NumericsConfig`, and asserting the timestep's own pressure solve asked it; verified to fail under exactly that mutation and to pass without it. This is the criterion its own text called "the one an otherwise-passing Stage 5 is most likely to fail silently", and it was half-failing silently. | ## TASK-041 — Fluid Configuration Section @@ -13140,15 +13140,21 @@ stated above rather than glossed. ## TASK-054 — Timestep Stability Warning -**Status: Done, 2026-09-12.** Discharges Completion Criterion 5. +**Status: Done, 2026-09-13.** Discharges Completion Criterion 5. ### Purpose Tell a user their configured timestep is above the stability limit *before* the run, rather than leaving them to infer it from the explosion. `stable_timestep` (`engine/simulation.py`) already computes -that limit and is well-derived, but no live path calls it -- a gap Stage -5's own Criterion 12 verdict noted and filed rather than fixed. Measured +that limit and is well-derived, but **no live path called it** -- a gap +Stage 5's own Criterion 12 verdict noted *without* filing it as a +violation, on the reasoning that the capability it serves (choosing a +timestep) is configured and the helper is a documented derivation rather +than a hidden one. That reasoning is defensible and the outcome was +still a user's run blowing up with nothing said: this is the same shape +as that stage's own finding that "a gap written down against no +criterion is how it survives". Measured by refining the shipped cavity and leaving `numerics.timestep` alone: | mesh | configured dt | stable dt | ratio | outcome | @@ -13236,7 +13242,7 @@ becoming a gate. ## TASK-055 — Every Boundary Field Reaches A Scheme Or Is Rejected -**Status: Not started, drafted 2026-09-12.** Will discharge Completion +**Status: Not started, drafted 2026-09-13.** Will discharge Completion Criteria 3 and 6. ### Purpose @@ -13781,9 +13787,15 @@ from the current velocity field and a configured CFL limit, which already does the arithmetic for -- and which needs no interface change at all, because nothing has to travel back out of `advance`. `docs/implementation/upgrade-paths.md` names "adaptive RK", which points -at the first; the second is cheaper and closes a real gap Stage 5 left -behind (`simulation.stable_timestep` is engine code no live run reaches, -noted in that stage's own Criterion 12 verdict). Both are defensible; +at the first; the second is cheaper and picks up where Stage 9's +TASK-054 stopped: that task made `stable_timestep` reachable from every +live run and had it *warn*, deliberately leaving "derive the timestep +from it" to this question rather than preempting it +(`docs/planning/backlog.md`'s own `numerics.timestep: auto` item states +the same boundary from the other side). The gap Stage 5 left behind -- +`simulation.stable_timestep` being engine code no live run reached, +noted in that stage's own Criterion 12 verdict -- is closed; what +remains open is only the choice below. Both are defensible; they are not the same amount of work. **Three. What can an additional linear solver actually claim here?** diff --git a/docs/planning/status.md b/docs/planning/status.md index 020b879..2503bb2 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -174,7 +174,7 @@ pie showData |------|--------|------|----------| | TASK-052 -- Prescribed Boundary Velocity Reaches The Schemes | Done | 2026-09-12 | `examples/golden-demos/smoke_transport.yaml` | | TASK-053 -- A Failed Frame Fails The Run | Done | 2026-09-12 | `tools/validators/check_dates.py` | -| TASK-054 -- Timestep Stability Warning | Done | 2026-09-12 | `engine/simulation.py` | +| TASK-054 -- Timestep Stability Warning | Done | 2026-09-13 | `engine/simulation.py` | | TASK-055 -- Every Boundary Field Reaches A Scheme Or Is Rejected | Not started | | | ### Stage 10 -- Better Numerics