From b65694981fd4949e6246cd7189739415b6e8230a Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Mon, 7 Sep 2026 22:10:26 +0100 Subject: [PATCH 1/2] Add modular, rank-based field display panels Replaces the single render_field/show_equalized_panel pair with field_display.panels: a list of independently configured panels, each naming its own field, colour mode (linear or equalized/rank-based histogram equalization), value range and label -- so a run can show several different fields side by side instead of one field at most twice. Rank-based coloring (rank_scalar_field_colors) replaces the earlier log10-based designs: it colours each cell by its rank among the field's current values, which keeps full contrast between peaks and valleys regardless of how close together they are in magnitude, with no range/percentile parameter to tune. Migrates all 6 golden demos and 4 experiment configs off the retired render_field/show_equalized_panel fields (rejected at load with a named error). Also fixes a real layout bug found while testing the 128x128 mesh-scaling experiment config: a panel's own legend/caption could be drawn over by the stats block below it, since the panel-list refactor widened the camera frame rightward for extra panels but not downward for a panel's own legend -- fixed, with a regression test that reproduces the exact reported symptom. Co-Authored-By: Claude Sonnet 5 --- docs/architecture/sequences.md | 2 +- docs/implementation/config-template.yaml | 24 +- docs/planning/roadmap.md | 96 ++++- docs/planning/status.md | 2 +- .../experiments/smoke_transport_high_res.yaml | 7 +- .../experiments/smoke_transport_mesh128.yaml | 7 +- .../experiments/smoke_transport_mesh64.yaml | 7 +- .../experiments/smoke_transport_re1000.yaml | 7 +- examples/golden-demos/heat_diffusion.yaml | 22 +- examples/golden-demos/heat_transport.yaml | 15 +- examples/golden-demos/multi_field_plume.yaml | 24 +- .../passive_scalar_transport.yaml | 21 +- examples/golden-demos/smoke_transport.yaml | 31 +- examples/golden-demos/thermal_buoyancy.yaml | 15 +- src/pyflow/bootstrap.py | 375 +++++++++++++++--- src/pyflow/configuration/CLAUDE.md | 57 ++- src/pyflow/configuration/loader.py | 28 +- src/pyflow/configuration/schema.py | 186 ++++++--- src/pyflow/rendering/CLAUDE.md | 97 ++++- src/pyflow/rendering/field_visualization.py | 38 ++ tests/features/field_declaration.feature | 8 +- tests/unit/test_bootstrap.py | 232 ++++++++++- tests/unit/test_configuration.py | 151 ++++++- .../test_field_declaration_configuration.py | 44 +- tests/unit/test_field_visualization.py | 67 ++++ tests/unit/test_golden_demo_annotations.py | 74 ++-- tests/unit/test_main.py | 2 +- tests/unit/test_temperature_field.py | 2 +- tools/generators/generate_config_template.py | 27 +- 29 files changed, 1389 insertions(+), 279 deletions(-) diff --git a/docs/architecture/sequences.md b/docs/architecture/sequences.md index 473c4e9..28707f0 100644 --- a/docs/architecture/sequences.md +++ b/docs/architecture/sequences.md @@ -208,7 +208,7 @@ sequenceDiagram Advance->>Step: step(state, velocity, numerics, dt) Step-->>Advance: new state Advance->>Window: simulation_fields = new state - Advance->>Viz: scalar_field_colors(new render_field, low, high, range) + Advance->>Viz: _panel_colors(new state, panel) per field_display.panels entry Viz-->>Advance: per-cell RGBA colors Advance->>Window: scene.remove(old object); scene.add(new object) Window->>Hud: on_frame() -- HUD half diff --git a/docs/implementation/config-template.yaml b/docs/implementation/config-template.yaml index 224e639..203bafe 100644 --- a/docs/implementation/config-template.yaml +++ b/docs/implementation/config-template.yaml @@ -110,19 +110,29 @@ field_display: arrow_scale: 0.3 # Valid: true or false. show_legend: true - # Valid: null (no live field is coloured) or the name of one field - # declared under fields: below -- the renderer never infers which one to - # show. Invalid: naming a field fields: does not declare. - render_field: null - # Valid: null (fall back to render_field's own name) or any string -- a - # human-readable legend caption, e.g. "Temperature (K)". Invalid: a non- - # string value. + # Valid: null (no caption at all) or any string -- a human-readable legend + # caption for the static scalar_pattern display only, e.g. "Distance from + # centre". A live panel (field_display.panels below) has its own, separate + # label instead. Invalid: a non-string value. field_label: null # Valid: null (no vector-scale HUD line at all) or any string -- what the # arrow display represents, e.g. "Velocity". When set, the HUD states this # label alongside arrow_scale wherever arrows are actually drawn. Invalid: # a non-string value. vector_label: null + # Valid: a list of live colour-mapped panel declarations, each a mapping + # with field (a non-empty string naming one field declared under fields: + # below -- the renderer never infers which one to show), mode (linear or + # equalized -- linear maps value_range onto low_color/high_color; + # equalized colours by each cell's rank among the field's current values + # instead, so peaks and valleys stay distinguishable even when both are + # numerically tiny, no range needed), value_range (a [min, max] pair, + # linear mode only), and label (null falls back to field's own name for a + # linear panel, or the constant "equalized" for an equalized one). Drawn + # left to right in list order. [] (the default) draws nothing. Invalid: + # naming a field fields: does not declare, an unrecognised mode, or a + # degenerate value_range (max <= min). + panels: [] # Valid: a list of per-field declarations, each a mapping with name (a non- # empty string, not reused by another declaration and not one of the diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index a3963ed..f450da1 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -306,7 +306,101 @@ 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): **1131 tests as of 2026-09-07**, up from 1052 the day before. +(C1a/C1b): **1154 tests as of 2026-09-07**, up from 1153 slightly +earlier the same day (below), then 1143, 1137, 1131, and 1052 the day +before that. + +**The 1 most recent is a real-bug regression test, found by a user +report rather than by any check in this repository -- the panel-list +migration just below widened `overall_bounds` rightward for extra +panels but never downward for a panel's own legend and caption, so +`_add_hud`'s stats block landed at the same world-space height the +caption already occupied.** Reported as "the legends all clip over each +other" while running `examples/experiments/smoke_transport_mesh128.yaml` +-- reproduced directly (a single-panel run's own rendered frame showed +the stats block drawn over "Smoke concentration (model units)"), root- +caused by reading `_add_declared_field_transport`'s own bounds +arithmetic against `_add_hud`'s (the live-panels path no longer flows +through `_add_hud`'s generic legend-widening block at all, since panels +caption themselves directly, and nothing replaced the widening that +block used to do), and fixed by folding each panel's own legend bottom +into `overall_bounds`'s y-minimum directly, the same margin +(`_LEGEND_LABEL_MARGIN_FRACTION`) the static path's own `_add_hud` block +already uses. `test_bootstrap_panel_stats_block_does_not_overlap_the_ +legend_caption` (`tests/unit/test_bootstrap.py`) reproduces the +single-panel case directly (no `.feature` file -- a rendering-layout +defect, the same "not physics" category `adr/ADR-007`'s scope excludes) +and was confirmed to fail without the fix before being trusted: reverting +just the new widening block reproduces the exact reported symptom +(stats at y=-0.06 against the legend's own bottom edge at y=-0.6). + +**The 10 before that replace `render_field`/`show_equalized_panel` with +a modular `field_display.panels: list[FieldPanelConfig]`** -- a further +same-day user request ("can we have the visibility [of] each of these +plots configurable too in a modular fashion? Later we may want [to] show +different fields than concentration too"), since the pair below could +only ever show one field, optionally twice, with no way to show two +different fields side by side or toggle either panel independently. Net ++10 across two files: `tests/unit/test_bootstrap.py` (8 tests replacing +the prior 8 named just below -- panel-list construction, one/two/no +panels, different fields per panel, the equalized caption/legend- +disabled/camera-widening/rebuild cases carried forward under new names; +net +2) and `tests/unit/test_configuration.py` (12 tests replacing the +prior 4 named just below -- reading a full panel declaration, the mode +default, each rejection surface (non-list, non-mapping, empty/non-string +field, invalid mode, degenerate value_range, non-string label), and the +two retired-setting migration-rejection tests; net +8). All six golden +demos that used `render_field` (`heat_diffusion`, `heat_transport`, +`multi_field_plume`, `passive_scalar_transport`, `smoke_transport`, +`thermal_buoyancy`) and four experiment configs under `examples/ +experiments/` were migrated to `panels:` in the same change, verified by +loading each directly, not merely by the test suite passing. +`tests/features/field_declaration.feature`'s own two `render_field` +scenarios were reworded to describe `field_display.panels` instead +(`adr/ADR-007`'s "the scenario is the criterion" -- the underlying +config surface genuinely changed, so the acceptance-criteria text +changes with it), with no new scenario count. + +**The 6 before those are `tests/unit/test_bootstrap.py`'s own coverage of +the equalized-panel wiring in `bootstrap.py` itself**, added once the +panel's own colour math and schema field already had tests (below) but +`bootstrap.py`'s own construction/legend/per-frame-rebuild/camera- +framing code did not -- found by `make ci`'s own coverage report +showing exactly those lines missed. `test_bootstrap_without_show_ +equalized_panel_adds_no_second_field_mesh`, `test_bootstrap_with_show_ +equalized_panel_adds_a_second_field_mesh_shifted_right`, `test_ +bootstrap_equalized_panel_legend_caption_is_just_equalized_not_the_ +field_label` (guards the caption-wrap fix below), `test_bootstrap_ +equalized_panel_with_legend_disabled_adds_no_equalized_legend`, `test_ +bootstrap_equalized_panel_widens_the_camera_framing`, and `test_ +bootstrap_equalized_panel_field_mesh_is_rebuilt_not_accumulated_across_ +frames` (the one that exercises `_advance`'s own per-frame path); six +tests, one per line/branch the coverage report named missing, verified +directly against `--cov-report=term-missing` (bootstrap.py: 86% -> 100%) +rather than assumed sufficient. + +**The 6 before those are not tied to any roadmap stage or task +either** -- the same rendering feature +(`field_display.show_equalized_panel`, `rank_scalar_field_colors`, +`src/pyflow/rendering/CLAUDE.md`'s own "Equalized (rank-based) field +panel" entry) added directly at a user's request while watching the +Smoke Transport demo, the same "visualisation work, not physics, not +`adr/ADR-007`-gated" category the HUD/axis-label work already occupies. +4 in `tests/unit/test_field_visualization.py` (`rank_scalar_field_ +colors`'s own rank-not-magnitude, tied-value-averaging, single-cell, +and shape/dtype cases), 2 in `tests/unit/test_configuration.py` +(`show_equalized_panel`'s own read and non-bool-rejection cases); +4 + 2 = 6. **The equalized panel's own legend caption was found and +fixed in the same follow-up change that added the six `test_bootstrap.py` +cases above**: an early version captioned it `f"{field_label} +(equalized)"`, which risked exactly the wrapped-caption-drawn-over-the- +mesh defect this file's HUD history already recorded once, the moment a +real demo's own `field_label` (`smoke_transport.yaml`'s "Smoke +concentration (model units)") got long enough -- changed to the plain +constant `"equalized"` before `smoke_transport.yaml` was updated to turn +the panel on, not after. + +**1131 tests as of 2026-09-07**, up from 1052 the day before. **30 of those 79 are TASK-046/047's own windowed-replay/playback addition** (below the `resume` breakdown); 49 are TASK-045's own, and **16 of those 49 are TASK-045's own `resume` addition** (below); the diff --git a/docs/planning/status.md b/docs/planning/status.md index a091c24..3058633 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -46,7 +46,7 @@ pie showData ## Live repository facts - **47** `CLAUDE.md` files -- **1131** tests collected +- **1154** tests collected - **144** Gherkin scenarios (`tests/features/*.feature`) ## Stages diff --git a/examples/experiments/smoke_transport_high_res.yaml b/examples/experiments/smoke_transport_high_res.yaml index 42569c8..2e60b92 100644 --- a/examples/experiments/smoke_transport_high_res.yaml +++ b/examples/experiments/smoke_transport_high_res.yaml @@ -53,11 +53,12 @@ fluid: viscosity: 0.01 field_display: - render_field: smoke low_color: "#0a0a2a" high_color: "#e8e8ff" - value_range: [0.0, 1.0] - field_label: Smoke concentration (model units) + panels: + - field: smoke + value_range: [0.0, 1.0] + label: Smoke concentration (model units) rendering: title: Smoke Transport (High Resolution) diff --git a/examples/experiments/smoke_transport_mesh128.yaml b/examples/experiments/smoke_transport_mesh128.yaml index 789bde7..8c24ef2 100644 --- a/examples/experiments/smoke_transport_mesh128.yaml +++ b/examples/experiments/smoke_transport_mesh128.yaml @@ -48,11 +48,12 @@ fluid: viscosity: 0.01 field_display: - render_field: smoke low_color: "#0a0a2a" high_color: "#e8e8ff" - value_range: [0.0, 1.0] - field_label: Smoke concentration (model units) + panels: + - field: smoke + value_range: [0.0, 1.0] + label: Smoke concentration (model units) rendering: title: Smoke Transport (128x128) diff --git a/examples/experiments/smoke_transport_mesh64.yaml b/examples/experiments/smoke_transport_mesh64.yaml index 8d61ef4..8af7e15 100644 --- a/examples/experiments/smoke_transport_mesh64.yaml +++ b/examples/experiments/smoke_transport_mesh64.yaml @@ -45,11 +45,12 @@ fluid: viscosity: 0.01 field_display: - render_field: smoke low_color: "#0a0a2a" high_color: "#e8e8ff" - value_range: [0.0, 1.0] - field_label: Smoke concentration (model units) + panels: + - field: smoke + value_range: [0.0, 1.0] + label: Smoke concentration (model units) rendering: title: Smoke Transport (64x64) diff --git a/examples/experiments/smoke_transport_re1000.yaml b/examples/experiments/smoke_transport_re1000.yaml index 1694171..94d2380 100644 --- a/examples/experiments/smoke_transport_re1000.yaml +++ b/examples/experiments/smoke_transport_re1000.yaml @@ -82,11 +82,12 @@ fluid: viscosity: 0.001 field_display: - render_field: smoke low_color: "#0a0a2a" high_color: "#e8e8ff" - value_range: [0.0, 1.0] - field_label: Smoke concentration (model units) + panels: + - field: smoke + value_range: [0.0, 1.0] + label: Smoke concentration (model units) rendering: title: Smoke Transport (Re 1000) diff --git a/examples/golden-demos/heat_diffusion.yaml b/examples/golden-demos/heat_diffusion.yaml index 57a5b90..506d9fa 100644 --- a/examples/golden-demos/heat_diffusion.yaml +++ b/examples/golden-demos/heat_diffusion.yaml @@ -13,8 +13,8 @@ # spreads". No velocity at all (`SimulationConfig.velocity_pattern` # unset): this demo is pure diffusion, the reading # `docs/handbook/numerical-methods/diffusion.md` describes for a single -# mode on a periodic domain. `field_display.render_field` names the -# declared field the live colour map shows. +# mode on a periodic domain. `field_display.panels` names the declared +# field(s) the live colour map(s) show, each its own modular panel. # # Run it exactly the way any user would: # @@ -50,16 +50,18 @@ fields: diffusion_coefficient: 0.05 field_display: - render_field: tracer low_color: "#0a0a2a" high_color: "#ff4400" - value_range: [-1.0, 1.0] - # Stage 7 (Rendering Annotations): named on screen, not just "tracer" - # (real feedback: "isn't sufficient"). Not a real thermal unit -- this - # is an anonymous diffusing scalar mode (`heat_transport.yaml` is the - # demo with a real-named `temperature` field), so the label says - # "model units" rather than implying a calibrated Kelvin value. - field_label: Diffusing scalar amplitude (model units) + panels: + - field: tracer + value_range: [-1.0, 1.0] + # Stage 7 (Rendering Annotations): named on screen, not just + # "tracer" (real feedback: "isn't sufficient"). Not a real thermal + # unit -- this is an anonymous diffusing scalar mode + # (`heat_transport.yaml` is the demo with a real-named + # `temperature` field), so the label says "model units" rather + # than implying a calibrated Kelvin value. + label: Diffusing scalar amplitude (model units) rendering: title: Heat Diffusion diff --git a/examples/golden-demos/heat_transport.yaml b/examples/golden-demos/heat_transport.yaml index cef47fb..834aca4 100644 --- a/examples/golden-demos/heat_transport.yaml +++ b/examples/golden-demos/heat_transport.yaml @@ -40,15 +40,16 @@ fields: diffusion_coefficient: 0.05 field_display: - render_field: temperature low_color: "#0a0a2a" high_color: "#ff4400" - value_range: [-1.0, 1.0] - # Stage 7 (Rendering Annotations): "model units" rather than a real - # Kelvin value -- nothing calibrates this field to real temperature, - # only its name and its diffusion equation match the physics it's - # named after. - field_label: Temperature (model units) + panels: + - field: temperature + value_range: [-1.0, 1.0] + # Stage 7 (Rendering Annotations): "model units" rather than a real + # Kelvin value -- nothing calibrates this field to real temperature, + # only its name and its diffusion equation match the physics it's + # named after. + label: Temperature (model units) rendering: title: Heat Transport diff --git a/examples/golden-demos/multi_field_plume.yaml b/examples/golden-demos/multi_field_plume.yaml index 6b35981..11b2681 100644 --- a/examples/golden-demos/multi_field_plume.yaml +++ b/examples/golden-demos/multi_field_plume.yaml @@ -118,21 +118,23 @@ fluid: viscosity: 0.1 field_display: + low_color: "#0a0a2a" + high_color: "#ff8844" # One field can be colour-mapped, and it is the one driving the flow. # The other three are transported all the same -- which is why this # demo's own report, not its rendered frame, is what demonstrates it # (`tests/features/multi_field_plume.feature`). - render_field: temperature - low_color: "#0a0a2a" - high_color: "#ff8844" - value_range: [0.0, 1.0] - # One line at this mesh's own HUD font size, and that is a constraint - # rather than a preference: a wrapped caption's second line is drawn - # over the mesh's bottom row (`src/pyflow/rendering/CLAUDE.md`, and - # `docs/planning/backlog.md` for the fix that is owed). The "1 of 4" - # claim this label first carried belongs in the run's own report - # anyway, which is where this demo actually demonstrates it. - field_label: Temperature (model units) + panels: + - field: temperature + value_range: [0.0, 1.0] + # One line at this mesh's own HUD font size, and that is a + # constraint rather than a preference: a wrapped caption's second + # line is drawn over the mesh's bottom row + # (`src/pyflow/rendering/CLAUDE.md`, and `docs/planning/ + # backlog.md` for the fix that is owed). The "1 of 4" claim this + # label first carried belongs in the run's own report anyway, + # which is where this demo actually demonstrates it. + label: Temperature (model units) rendering: title: Multi-Field Plume diff --git a/examples/golden-demos/passive_scalar_transport.yaml b/examples/golden-demos/passive_scalar_transport.yaml index a7a4356..379521f 100644 --- a/examples/golden-demos/passive_scalar_transport.yaml +++ b/examples/golden-demos/passive_scalar_transport.yaml @@ -12,9 +12,9 @@ # `fields:` declares the blob's own name, initial condition and # diffusivity (`FieldConfig`, `src/pyflow/configuration/schema.py`, # TASK-042); `simulation:` names the prescribed velocity that transports -# it; `field_display.render_field` names which declared field the live -# colour map shows (there is only one here, but the renderer never -# infers that -- it must always be named). `north`/`south` are +# it; `field_display.panels` names which declared field each live colour +# map shows (there is only one panel here, but the renderer never infers +# that -- it must always be named). `north`/`south` are # `neumann` with a zero gradient (an insulated wall) rather than # periodic -- the prescribed velocity is purely horizontal, so nothing # ever flows across them, but diffusion still needs *some* condition @@ -48,15 +48,16 @@ simulation: velocity: [1.0, 0.0] field_display: - render_field: tracer low_color: "#0a0a2a" high_color: "#ff8c00" - value_range: [0.0, 1.0] - # Stage 7 (Rendering Annotations): real feedback on the first cut of - # this demo's own HUD -- "'Tracer' isn't sufficient." "model units": - # concentration here is a bare transported scalar, not calibrated to - # any real concentration unit. - field_label: Tracer concentration (model units) + panels: + - field: tracer + value_range: [0.0, 1.0] + # Stage 7 (Rendering Annotations): real feedback on the first cut + # of this demo's own HUD -- "'Tracer' isn't sufficient." "model + # units": concentration here is a bare transported scalar, not + # calibrated to any real concentration unit. + label: Tracer concentration (model units) rendering: title: Passive Scalar Transport diff --git a/examples/golden-demos/smoke_transport.yaml b/examples/golden-demos/smoke_transport.yaml index 15a1ea4..49969f8 100644 --- a/examples/golden-demos/smoke_transport.yaml +++ b/examples/golden-demos/smoke_transport.yaml @@ -51,14 +51,33 @@ fluid: viscosity: 0.01 field_display: - render_field: smoke low_color: "#0a0a2a" high_color: "#e8e8ff" - value_range: [0.0, 1.0] - # Stage 7 (Rendering Annotations): named on screen, same reasoning as - # every other demo's own `field_label` -- "model units" since nothing - # calibrates this to a real smoke-density unit. - field_label: Smoke concentration (model units) + # Two modular panels (`field_display.panels`), both showing the same + # `smoke` field -- added at a user's direct request after watching + # this exact demo: smoke decays toward zero over the run, and a fixed + # [0, 1] linear range washes that decay into a single near-black shade + # long before the field has actually vanished. See `src/pyflow/ + # rendering/CLAUDE.md`'s "Equalized (rank-based) field panel" entry + # for the full design history (two magnitude-based scales -- log10, + # then percentile-trimmed log10 -- were tried and rejected first) and + # why keeping the plain linear panel alongside the equalized one, not + # replacing it, matters: it's what makes the equalized panel's own + # adaptive range legible as *adaptive* rather than just "the demo's + # colours changed". The panel list itself (not just the equalized mode) + # was added the same day, at a further user request for each panel's + # visibility to be independently configurable, and for a future run to + # be able to show *different* fields side by side, not only one field + # coloured two ways. + panels: + - field: smoke + value_range: [0.0, 1.0] + # Stage 7 (Rendering Annotations): named on screen, same reasoning + # as every other demo's own panel label -- "model units" since + # nothing calibrates this to a real smoke-density unit. + label: Smoke concentration (model units) + - field: smoke + mode: equalized rendering: title: Smoke Transport diff --git a/examples/golden-demos/thermal_buoyancy.yaml b/examples/golden-demos/thermal_buoyancy.yaml index ad44285..6997ce6 100644 --- a/examples/golden-demos/thermal_buoyancy.yaml +++ b/examples/golden-demos/thermal_buoyancy.yaml @@ -61,15 +61,16 @@ fluid: viscosity: 0.1 field_display: - render_field: temperature low_color: "#0a0a2a" high_color: "#ff4400" - value_range: [0.0, 1.0] - # Stage 7 (Rendering Annotations): "model units", the same reasoning - # `heat_transport.yaml` already states -- nothing calibrates this - # field to real Kelvin, only its name and its buoyancy coupling match - # the physics it's named after. - field_label: Temperature (model units) + panels: + - field: temperature + value_range: [0.0, 1.0] + # Stage 7 (Rendering Annotations): "model units", the same + # reasoning `heat_transport.yaml` already states -- nothing + # calibrates this field to real Kelvin, only its name and its + # buoyancy coupling match the physics it's named after. + label: Temperature (model units) rendering: title: Thermal Buoyancy diff --git a/src/pyflow/bootstrap.py b/src/pyflow/bootstrap.py index 3c0f8b4..468a8b2 100644 --- a/src/pyflow/bootstrap.py +++ b/src/pyflow/bootstrap.py @@ -26,13 +26,15 @@ **`_add_declared_field_transport` is TASK-042's generalisation of TASK-030's own `_add_passive_scalar_transport` (Stage 6, 2026-08-30)**: one hardcoded field named `"tracer"` became one `ScalarField` per -`config.fields` declaration, and which one (if any) gets a live colour -map is `field_display.render_field`, named explicitly rather than -inferred (`src/pyflow/configuration/CLAUDE.md`'s own `FieldConfig` -entry) -- the same rename this docstring's own two paragraphs above -apply throughout. It needed no further change for TASK-035's own -solved-velocity-plus-declared-field combination (Thermal Buoyancy): it -already assembled that combination generically, for TASK-042. +`config.fields` declaration, and which ones (if any) get a live colour +map, and how, is `field_display.panels` (originally `render_field`, +named explicitly rather than inferred -- `src/pyflow/configuration/ +CLAUDE.md`'s own `FieldConfig` entry; widened to a modular multi-panel +list 2026-09-07, see `FieldPanelConfig`'s own docstring) -- the same +rename this docstring's own two paragraphs above apply throughout. It +needed no further change for TASK-035's own solved-velocity-plus- +declared-field combination (Thermal Buoyancy): it already assembled +that combination generically, for TASK-042. **This module composed a fourth package as of TASK-035 (Stage 6, 2026-08-30): `physics`, via a `pyflow.physics.buoyancy` import for its @@ -82,12 +84,14 @@ from collections.abc import Callable from pathlib import Path +import numpy as np import pygfx as gfx from pyflow import __version__ from pyflow.configuration import load_config from pyflow.configuration.schema import ( FieldDisplayConfig, + FieldPanelConfig, PyFlowConfig, RenderBackend, UnitsConfig, @@ -101,6 +105,7 @@ build_field_legend, build_scalar_field_mesh, build_vector_field_arrows, + rank_scalar_field_colors, scalar_field_colors, ) from pyflow.rendering.hud import ( @@ -175,6 +180,13 @@ _LEGEND_Z = 0.02 _HUD_Z = 0.03 +# Each live panel (`FieldDisplayConfig.panels`) sits to the *right* of +# the previous one, gap sized the same "fraction of the dimension it's +# relative to" way every other margin here is -- `_LEGEND_GAP_FRACTION` +# is a fraction of mesh *height* since the legend sits below the mesh; +# this is a fraction of mesh *width* since panels sit beside each other. +_PANEL_GAP_FRACTION = 0.15 + _Bounds = tuple[float, float, float, float] @@ -232,9 +244,147 @@ def _add_legend( return legend_bounds +def _panel_caption(panel: FieldPanelConfig) -> str: + """A panel's own legend caption -- `panel.label` if set, explicitly; + otherwise `panel.field`'s own name for a `"linear"` panel (the same + fallback `field_label`/`render_field` used to give one top-level + caption), or the plain constant `"equalized"` for an `"equalized"` + one, never the field name repeated with a suffix. Deliberately not + `f"{panel.field} (equalized)"`: an early cut of the equalized panel + captioned itself that way and it risked exactly the wrapped-caption- + drawn-over-the-mesh defect this file's HUD history already hit once, + the moment a real demo's own field name/label got long enough + (`src/pyflow/rendering/CLAUDE.md`'s "Equalized (rank-based) field + panel" entry). A viewer looking at several panels of related fields + only needs telling what's different about each one, not the full + name repeated -- and an explicit `panel.label` always overrides this + default outright, so nothing stops a config author choosing a longer + caption deliberately. + """ + if panel.label is not None: + return panel.label + return panel.field if panel.mode == "linear" else "equalized" + + +def _add_panel_legend( + window: RenderWindow, + low_color: str, + high_color: str, + show_legend: bool, + mesh_bounds: _Bounds, + offset_x: float, + caption: str, + initial_min: float, + initial_max: float, +) -> tuple[_Bounds | None, Callable[[float, float], None] | None]: + """One live panel's own legend -- a gradient strip below that + panel's own field mesh, shifted `offset_x` to the right of the + mesh's own left edge, captioned `caption`. Every live panel + (`FieldDisplayConfig.panels`) builds its own legend this way, + regardless of `mode` -- generalised from what used to be two + separate functions (`_add_legend`'s own live-path use, for the one + linear panel a run could have; `_add_equalized_panel_legend`, for + the one optional second panel) into one, now that any number of + panels can exist side by side. + + For a `"linear"` panel, `initial_min`/`initial_max` are + `panel.value_range`'s own fixed bounds -- the legend never needs + updating after the first frame, so callers simply never invoke the + returned `update_labels` again. For an `"equalized"` panel, there is + no fixed `(min, max)` the ramp actually means (colour depends on + *rank*, not magnitude) -- what gets labelled instead is the field's + own current min/max *value*, purely for context, and `update_labels` + is what keeps those two numbers honest as the field's own live + spread moves. + + **The gradient strip itself is built once, not per frame, even for + an equalized panel whose labelled min/max changes every frame** -- + `build_field_legend`'s own colour ramp is a pure `low_color`-to- + `high_color` interpolation over whatever range it's given, so its + *rendered pixels* are identical for every valid `(min, max)` pair; + only what the two ends are *labelled* as changes. Built here with a + placeholder `(0.0, 1.0)` range for exactly that reason -- rebuilding + a mesh whose own appearance provably never changes would be pure + waste, the same "don't do work whose result can't differ" reasoning + the returned update closure applies to the labels, which *do* need + it for an equalized panel. + + Returns `(legend_bounds, update_labels)`, or `(None, None)` if + `show_legend` is false. `gfx.Text.set_text` mutates in place, the + same per-frame-update mechanism `_add_hud`'s own stats block already + uses, so this needs no rebuild-the-object dance the field mesh + itself can't avoid (its *positions*, not just text, change frame to + frame). + """ + if not show_legend: + return None, None + min_x, min_y, max_x, max_y = mesh_bounds + mesh_height = max_y - min_y + legend_height = mesh_height * _LEGEND_HEIGHT_FRACTION + gap = mesh_height * _LEGEND_GAP_FRACTION + legend_bottom = min_y - gap - legend_height + legend_bounds = (min_x + offset_x, legend_bottom, max_x + offset_x, min_y - gap) + legend = build_field_legend( + low_color, + high_color, + (0.0, 1.0), # placeholder -- see docstring: the ramp's own pixels don't depend on this + legend_bounds, + ) + legend.local.position = (0.0, 0.0, _LEGEND_Z) + window.scene.add(legend) + + font_size = mesh_height * 0.05 + low_text, high_text, *_rest = build_legend_labels( + f"{initial_min:.3g}", + f"{initial_max:.3g}", + caption, + legend_bounds, + font_size=font_size, + max_width=max_x - min_x, + ) + for label in (low_text, high_text, *_rest): + label.local.position = (label.local.position[0], label.local.position[1], _HUD_Z) + window.scene.add(label) + + def _update_labels(field_min: float, field_max: float) -> None: + low_text.set_text(f"{field_min:.3g}") + high_text.set_text(f"{field_max:.3g}") + + return legend_bounds, _update_labels + + +class _PanelRenderState: + """Mutable per-panel render state `_add_declared_field_transport` + threads through its own initial build and `_advance`'s per-frame + rebuild -- one instance per `FieldDisplayConfig.panels` entry. + `mesh_object`/`update_labels` start `None` and are filled in by the + initial build below; kept as a small object rather than parallel + lists so each panel's own state stays together under one name. + """ + + def __init__(self, panel: FieldPanelConfig, offset_x: float) -> None: + self.panel = panel + self.offset_x = offset_x + self.mesh_object: gfx.Mesh | None = None + self.update_labels: Callable[[float, float], None] | None = None + + +def _panel_colors( + field: ScalarField, panel: FieldPanelConfig, low_color: str, high_color: str +) -> np.ndarray: + """A panel's own colour array, dispatched by `panel.mode` -- + `"linear"` (`scalar_field_colors`, `panel.value_range` fixed) or + `"equalized"` (`rank_scalar_field_colors`, no range needed, and + `panel.value_range` ignored). + """ + if panel.mode == "equalized": + return rank_scalar_field_colors(field, low_color, high_color) + return scalar_field_colors(field, low_color, high_color, panel.value_range) + + def _add_declared_field_transport( window: RenderWindow, mesh: Mesh, config: PyFlowConfig -) -> tuple[Callable[[], None], _Bounds | None]: +) -> tuple[Callable[[], None], _Bounds]: """Wires a real `simulation.step()` into a live `pyflow run` (Stage 4 Completion Criterion 1, TASK-030) -- the mechanism the Passive Scalar Transport golden demo needs and no demo before it @@ -248,19 +398,19 @@ def _add_declared_field_transport( `config.fields`' own declarations (TASK-042, Stage 6, 2026-08-30).** Every declared field is transported together, in the same `step`/ `navier_stokes_step` call -- Criterion 1's own claim that a - transported field is added by configuration, not by code. Which one - (if any) gets a live colour map is `config.field_display. - render_field`, named explicitly rather than inferred - (`src/pyflow/configuration/CLAUDE.md`'s own `FieldConfig` entry) -- - `None` renders nothing for the live simulation, the same "no display - configured, nothing drawn" shape `_add_field_display` already uses - for the static case. - - Rebuilds the rendered `gfx.Mesh` from scratch each frame (removes the - old one from `window.scene`, `build_scalar_field_mesh`s a new one) - rather than mutating the geometry's own colour buffer in place -- - `build_scalar_field_mesh`/`scalar_field_colors` are already proven - correct (TASK-017); an in-place buffer mutation would be new, + transported field is added by configuration, not by code. Which ones + (if any) get a live colour map, and how, is + `config.field_display.panels` -- any number of independently + configured panels (`FieldPanelConfig`), each naming its own declared + field and colour mode, drawn left to right. `[]` renders nothing for + the live simulation, the same "no display configured, nothing drawn" + shape `_add_field_display` already uses for the static case. + + Rebuilds each rendered `gfx.Mesh` from scratch every frame (removes + the old one from `window.scene`, `build_scalar_field_mesh`s a new + one) rather than mutating the geometry's own colour buffer in place + -- `build_scalar_field_mesh`/`scalar_field_colors` are already + proven correct (TASK-017); an in-place buffer mutation would be new, unverified pygfx-API surface for a small win on a small demo mesh (TASK-030's own Design decision). @@ -304,6 +454,40 @@ def _add_declared_field_transport( `if solved: navier_stokes_step(...) else: simulation_step(...)` branch) now lives in a module with no `rendering` import at all. This function keeps only the scene/legend/colour-map half. + + **`config.field_display.panels` (added 2026-09-07, replacing the + single `render_field`/`show_equalized_panel` pair the same day, at a + user's direct request for each panel's visibility to be + independently "configurable... in a modular fashion" -- not tied to + any roadmap stage or task, the same "rendering/visualisation work, + not ADR-007-gated" category the HUD/axis-label additions above + already fall into) draws any number of colour-mapped panels side by + side, each its own `FieldPanelConfig`.** Every panel is rebuilt every + frame the same way (remove old, `build_scalar_field_mesh` a new one + from `_panel_colors`, shift right via `.local.position`); each + panel's own legend and numeric labels (`_add_panel_legend`) are + built once, not per frame -- an equalized panel's own labels are + then kept current by its returned `update_labels` closure, since + only its *labels* change frame to frame, never its gradient strip's + own rendered pixels (see `_add_panel_legend`'s own docstring). + Returns the overall bounds (mesh, widened right by every panel drawn) + as its second value, since `bootstrap()`'s own camera framing needs + to know about it. **No longer returns a `legend_bounds` at all** + (previously a second, middle value -- the primary panel's own + strip bounds, for `_add_hud`'s generic numeric-label code to + caption): every live panel captions itself via `_add_panel_legend` + directly now, so there is no single "the" legend left for + `_add_hud`'s generic block to caption -- that block only ever fires + for the static `scalar_pattern` path, which still returns its own + `legend_bounds` from `_add_field_display` unaffected by this change. + + **The `"equalized"` mode itself, and the two magnitude-based designs + tried and rejected before it, are `FieldPanelConfig`'s/ + `rank_scalar_field_colors`'s own history to tell, not repeated + here** -- this function only wires whichever modes a config + declares; see those two docstrings, and `src/pyflow/rendering/ + CLAUDE.md`'s "Equalized (rank-based) field panel" entry, for the + full design record. """ assert window.assembled_numerics is not None numerics = window.assembled_numerics @@ -320,50 +504,108 @@ def _add_declared_field_transport( state: SimulationState = built_state window.simulation_fields = state.fields - render_field_name = config.field_display.render_field - rendered_object: gfx.Mesh | None = None - legend_bounds: _Bounds | None = None - if render_field_name is not None: - rendered_field = state.fields[render_field_name] + mesh_width = bounds[2] - bounds[0] + mesh_height = bounds[3] - bounds[1] + panel_states = [ + _PanelRenderState(panel, index * mesh_width * (1.0 + _PANEL_GAP_FRACTION)) + for index, panel in enumerate(config.field_display.panels) + ] + overall_bounds = bounds + for panel_state in panel_states: + panel = panel_state.panel + rendered_field = state.fields[panel.field] assert isinstance(rendered_field, ScalarField) - colors = scalar_field_colors( - rendered_field, + colors = _panel_colors( + rendered_field, panel, config.field_display.low_color, config.field_display.high_color + ) + panel_state.mesh_object = build_scalar_field_mesh(rendered_field, colors) + panel_state.mesh_object.local.position = (panel_state.offset_x, 0.0, 0.0) + window.scene.add(panel_state.mesh_object) + if panel.mode == "equalized": + initial_min = float(rendered_field.values.min()) + initial_max = float(rendered_field.values.max()) + else: + initial_min, initial_max = panel.value_range + panel_legend_bounds, panel_state.update_labels = _add_panel_legend( + window, config.field_display.low_color, config.field_display.high_color, - config.field_display.value_range, + config.field_display.show_legend, + bounds, + panel_state.offset_x, + _panel_caption(panel), + initial_min, + initial_max, ) - rendered_object = build_scalar_field_mesh(rendered_field, colors) - window.scene.add(rendered_object) - legend_bounds = _add_legend(window, config.field_display, bounds) + overall_bounds = ( + overall_bounds[0], + overall_bounds[1], + max(overall_bounds[2], bounds[2] + panel_state.offset_x), + overall_bounds[3], + ) + if panel_legend_bounds is not None: + # Every panel's own legend sits at the same height (only the + # x-offset differs), so this converges to one value across + # the loop -- computed per panel rather than once, since a + # panel with `show_legend` effectively off (there is no + # per-panel toggle) never happens today, but this stays + # correct if that changes. Mirrors `_add_hud`'s own "widen + # `min_y` past the legend's own label margin" step + # (`_LEGEND_LABEL_MARGIN_FRACTION`) for the static + # `scalar_pattern` path -- this function's own panels no + # longer flow through that generic block (they caption + # themselves directly), so the identical widening has to + # happen here instead, or `_add_hud`'s stats block below is + # placed as if no legend or caption exists at all and draws + # straight over them. **Found by a real user report** ("the + # legends all clip over each other" on the `mesh128` + # experiment config) after this fold-in was missed when + # `_add_declared_field_transport` stopped returning a + # `legend_bounds` for `_add_hud` to widen by itself. + overall_bounds = ( + overall_bounds[0], + min( + overall_bounds[1], + panel_legend_bounds[1] - mesh_height * _LEGEND_LABEL_MARGIN_FRACTION, + ), + overall_bounds[2], + overall_bounds[3], + ) def _advance() -> None: - nonlocal state, rendered_object + nonlocal state state = advance_simulation_state(state, numerics, config.numerics.timestep) window.simulation_fields = state.fields - if render_field_name is not None: - rendered_field = state.fields[render_field_name] + # Note for anyone inspecting `window.scene.children` order (found + # while fixing `tests/unit/test_field_declaration_configuration. + # py` for Stage 7's own legend addition): after each panel's own + # remove-then-add below, its field mesh sits *after* every + # legend added once, above, in scene-child order -- not before + # them, as a first render's own insertion order would suggest. + # Identify a panel's own field mesh by its geometry shape + # (`mesh.num_cells * 2` colour rows), not by scene position, if + # a future reader needs to find it again. + for panel_state in panel_states: + panel = panel_state.panel + rendered_field = state.fields[panel.field] assert isinstance(rendered_field, ScalarField) - colors = scalar_field_colors( + colors = _panel_colors( rendered_field, + panel, config.field_display.low_color, config.field_display.high_color, - config.field_display.value_range, ) - assert rendered_object is not None - window.scene.remove(rendered_object) - rendered_object = build_scalar_field_mesh(rendered_field, colors) - window.scene.add(rendered_object) - # Note for anyone inspecting `window.scene.children` order - # (found while fixing `tests/unit/ - # test_field_declaration_configuration.py` for Stage 7's own - # legend addition): after this remove-then-add, the field mesh - # sits *after* the legend added once, above, in scene-child - # order -- not before it, as a first render's own insertion - # order would suggest. Identify the field mesh by its own - # geometry shape (`mesh.num_cells * 2` colour rows), not by - # scene position, if a future reader needs to find it again. + assert panel_state.mesh_object is not None + window.scene.remove(panel_state.mesh_object) + panel_state.mesh_object = build_scalar_field_mesh(rendered_field, colors) + panel_state.mesh_object.local.position = (panel_state.offset_x, 0.0, 0.0) + window.scene.add(panel_state.mesh_object) + if panel.mode == "equalized" and panel_state.update_labels is not None: + panel_state.update_labels( + float(rendered_field.values.min()), float(rendered_field.values.max()) + ) - return _advance, legend_bounds + return _advance, overall_bounds def _add_solved_velocity_rendering( @@ -717,7 +959,17 @@ def _add_hud( # caption over the mesh's own bottom row in every demo setting # `field_label`. See `_LEGEND_GAP_FRACTION`'s own comment for # what that cost and how it was found. - field_label = config.field_display.field_label or config.field_display.render_field + # + # `legend_bounds` only ever reaches here from the *static* + # `scalar_pattern` path (`_add_field_display`) since 2026-09-07 -- + # the live per-field panels now caption themselves directly + # (`_add_declared_field_transport`'s own `_add_panel_legend` + # calls) and always return `None` here, because a config can + # declare several panels of several different fields and there + # is no longer one single field name to fall back to. No + # fallback needed for that reason: a static display has no + # underlying declared field at all to name. + field_label = config.field_display.field_label low_value, high_value = config.field_display.value_range labels = build_legend_labels( f"{low_value:.4g}", @@ -912,7 +1164,24 @@ def bootstrap( # Draws a colour map, never arrows, so it leaves # `show_vector_scale` alone (`_add_field_display`'s static # `vector_pattern` above may still have drawn some). - on_frame, legend_bounds = _add_declared_field_transport(window, mesh, config) + on_frame, declared_field_bounds = _add_declared_field_transport(window, mesh, config) + # `legend_bounds` is left exactly as `show_fields`'s own + # static overlay above set it (or `None`, if it didn't run): + # every live panel captions itself directly now + # (`_add_declared_field_transport`'s own docstring), so there + # is nothing this branch needs to contribute to it. + # Union, not overwrite: `show_fields`'s own static overlay + # above may have already widened `bounds` (its own legend, + # e.g.) -- `field_display.panels`'s rightward widening + # composes with that rather than discarding it, the same + # "two independent switches, bounds accumulate" shape arrows + # already use for `show_vector_scale`. + bounds = ( + min(bounds[0], declared_field_bounds[0]), + min(bounds[1], declared_field_bounds[1]), + max(bounds[2], declared_field_bounds[2]), + max(bounds[3], declared_field_bounds[3]), + ) elif run_velocity_only_simulation: # Joined with whatever `_add_field_display` reported above, # never replacing it: a static `vector_pattern` and a diff --git a/src/pyflow/configuration/CLAUDE.md b/src/pyflow/configuration/CLAUDE.md index b3513bc..b924820 100644 --- a/src/pyflow/configuration/CLAUDE.md +++ b/src/pyflow/configuration/CLAUDE.md @@ -254,20 +254,43 @@ engine name it would silently become (`"pressure"` -- `PressureField`'s own fixed name; `"velocity.0"`/`"velocity.1"` -- `VectorField.component_name("velocity", i)`'s fixed output, both hardcoded in `schema.py` rather than imported, since `configuration` has -no dependency on `engine`), and `field_display.render_field` (below), if -set, actually names one of them. +no dependency on `engine`), and every `field_display.panels[].field` +(below), if any, actually names one of them. **`FieldDisplayConfig.render_field: str | None` (TASK-042, added -2026-08-30) is the declared field whose live colour map `bootstrap.py` -renders -- named explicitly, never inferred.** With one field there was +2026-08-30) was the declared field whose live colour map `bootstrap.py` +rendered -- named explicitly, never inferred.** With one field there was nothing to choose between; with several, inferring one (first declared, alphabetically first) would be a rule a reader has to know rather than read. A separate field from `scalar_pattern` above, deliberately: that one seeds a synthetic static pattern for a demo with no live simulation; -this one selects among fields a run actually transports. Cross-checked -against `PyFlowConfig.fields` in `_validate_field_declarations`, not in +this one selected among fields a run actually transported. + +**Retired 2026-09-07, replaced by `FieldDisplayConfig.panels: list[ +FieldPanelConfig]` -- read the rest of this entry as history, not the +live schema.** `render_field` (together with `show_equalized_panel`, +below) could only ever show one declared field, optionally twice +(linear, and rank-equalized); a user asked directly for each panel's +visibility to be independently "configurable... in a modular fashion", +specifically so a future run could show *different* fields side by +side, not only one field coloured two ways. `FieldPanelConfig` (`field`, +`mode: "linear" | "equalized"`, `value_range`, `label`) is that modular +replacement -- see its own docstring in `schema.py` for the full shape +and design history, and `src/pyflow/rendering/CLAUDE.md`'s "Equalized +(rank-based) field panel" entry for `bootstrap.py`'s own side of it. +`field_display.value_range`/`field_label` (both still live fields on +this class) are now scoped to the *static* `scalar_pattern` display +only -- each live panel carries its own `value_range`/`label` instead, +and there is no longer a single top-level caption that could mean +either path. A configuration still setting `render_field` or +`show_equalized_panel` is rejected at load with a named error pointing +at `field_display.panels`, the same "malformed input produces a +field-named error" migration shape `NumericsConfig.diffusion_coefficient`'s +own move to `FluidConfig` already used. Cross-checked against +`PyFlowConfig.fields` in `_validate_field_declarations`, not in `FieldDisplayConfig.validate()` itself, since that class alone cannot -see what `fields:` declares. +see what `fields:` declares -- unchanged by the migration, just checking +every panel's own `field` now instead of one `render_field`. **Deliberately does not declare boundary treatment.** That already has a real per-field mechanism (`BoundaryFaceConfig.field_values`/ @@ -341,14 +364,18 @@ bare look, and asks for it explicitly now (`show_title: false`, implicit side effect of the old gate. **`FieldDisplayConfig.field_label: str | None = None` (Stage 7, added -2026-08-31)** is a human-readable legend caption (e.g. `"Temperature -(K)"`), shown above the legend strip by `rendering/hud.py`'s -`build_legend_labels`. `None` falls back to `render_field`'s own name -- -a deliberately separate field, since `render_field` is an internal -transport-path key (`engine/simulation.py`'s `state` mapping) and isn't -always what a viewer should read on screen. Every shipped golden demo -that colour-maps a field now sets this explicitly (real feedback: "The -Field quantity isn't specified... 'Tracer' isn't sufficient") -- +2026-08-31; scope narrowed to the static `scalar_pattern` display only +on 2026-09-07, when `FieldPanelConfig.label` took over this role for +every live panel -- see the `render_field` entry above)** is a +human-readable legend caption (e.g. `"Distance from centre"`), shown +above the legend strip by `rendering/hud.py`'s `build_legend_labels`. +`None` shows no caption at all for that path -- there is no field name +to fall back to the way a live panel falls back to its own `field`, +since `scalar_pattern` seeds a synthetic pattern with no underlying +declared field. Every shipped golden demo that colour-maps a field sets +an equivalent caption explicitly, on whichever of the two mechanisms +applies to it (real feedback: "The Field quantity isn't specified... +'Tracer' isn't sufficient") -- `"(model units)"` where the field isn't calibrated to a real physical unit (which is every demo so far; nothing in this schema ties a transported scalar to SI), stated honestly rather than implying a diff --git a/src/pyflow/configuration/loader.py b/src/pyflow/configuration/loader.py index bcf6b15..3d27dc1 100644 --- a/src/pyflow/configuration/loader.py +++ b/src/pyflow/configuration/loader.py @@ -19,6 +19,7 @@ BoundaryFaceConfig, FieldConfig, FieldDisplayConfig, + FieldPanelConfig, FluidConfig, LoggingConfig, MeshConfig, @@ -84,6 +85,31 @@ def _fields_from_raw(raw: object) -> list[FieldConfig]: return declared +def _field_panels_from_raw(raw: object) -> list[FieldPanelConfig]: + if not isinstance(raw, list): + raise TypeError(f"field_display.panels must be a list, got {type(raw).__name__}") + panels = [] + for index, item in enumerate(raw): + if not isinstance(item, dict): + raise TypeError( + f"field_display.panels[{index}] must be a mapping, got {type(item).__name__}" + ) + panels.append(FieldPanelConfig(**item)) + return panels + + +def _field_display_config_from_raw(raw: dict[str, Any]) -> FieldDisplayConfig: + raw = dict(raw) + if "render_field" in raw or "show_equalized_panel" in raw: + raise ValueError( + "field_display.render_field/show_equalized_panel have moved to field_display.panels " + "(a list of {field, mode, value_range, label} entries) -- update your configuration " + "file" + ) + panels_raw = raw.pop("panels", []) + return FieldDisplayConfig(panels=_field_panels_from_raw(panels_raw), **raw) + + def _config_from_raw(raw: dict[str, Any], *, source: str) -> PyFlowConfig: """Shared by `load_config` (`raw` from `yaml.safe_load`) and `config_from_dict` (`raw` from `dataclasses.asdict()`, a checkpoint's @@ -117,7 +143,7 @@ def _config_from_raw(raw: dict[str, Any], *, source: str) -> PyFlowConfig: logging=LoggingConfig(**raw.get("logging", {})), rendering=RenderingConfig(**raw.get("rendering", {})), mesh=MeshConfig(**raw.get("mesh", {})), - field_display=FieldDisplayConfig(**raw.get("field_display", {})), + field_display=_field_display_config_from_raw(raw.get("field_display", {})), fields=_fields_from_raw(raw.get("fields", [])), simulation=_simulation_config_from_raw(raw.get("simulation", {})), fluid=FluidConfig(**raw.get("fluid", {})), diff --git a/src/pyflow/configuration/schema.py b/src/pyflow/configuration/schema.py index 6468e7c..38386d9 100644 --- a/src/pyflow/configuration/schema.py +++ b/src/pyflow/configuration/schema.py @@ -264,6 +264,75 @@ def validate(self) -> None: _VALID_SCALAR_PATTERNS = frozenset(get_args(ScalarDisplayPattern)) _VALID_VECTOR_PATTERNS = frozenset(get_args(VectorDisplayPattern)) +PanelMode = Literal["linear", "equalized"] +_VALID_PANEL_MODES = frozenset(get_args(PanelMode)) + + +@dataclass +class FieldPanelConfig: + """One colour-mapped panel in the live-run field display -- a + modular replacement for the single `render_field`/ + `show_equalized_panel` pair this schema used to carry (both retired + in the same change, 2026-09-07), added directly at a user's request + for each panel's visibility to be "configurable... in a modular + fashion", specifically so a future run can show several *different* + fields side by side, not only one field coloured two ways. + + `field` names a declared field (`PyFlowConfig.fields`) -- the same + role `render_field` used to play, now per-panel rather than once for + the whole display. Cross-checked against `PyFlowConfig.fields` in + `_validate_field_declarations`, not here: this class alone cannot + see what `fields:` declares. `mode` selects the colour function: + `"linear"` (`field_visualization.scalar_field_colors`, `value_range` + decides the mapping and never changes frame to frame) or + `"equalized"` (`field_visualization.rank_scalar_field_colors`, ranks + the field's own current values every frame -- no range to set, and + `value_range` is ignored for this mode). `value_range` defaults to + `(0.0, 1.0)`, the same default the old top-level `field_display. + value_range` used for this purpose; still validated even for an + `"equalized"` panel where it's unused, rather than special-cased -- + the simpler rule to state and to implement, and a degenerate range + is still worth rejecting regardless of whether anything reads it. + `label` is this panel's own legend caption; `None` falls back to + `field`'s own name, the same "internal key vs. on-screen text" split + `field_label`/`render_field` used to draw between two top-level + settings, now between two fields on the same panel declaration. + + `FieldDisplayConfig.panels: list[FieldPanelConfig]` (default `[]`, + meaning "render nothing", same as `render_field: null` used to) + draws its panels left to right in declaration order -- + `bootstrap.py`'s own `_add_declared_field_transport` positions each + one `mesh_width * (1 + _PANEL_GAP_FRACTION)` to the right of the + previous, the same spacing the old two-panel layout already used, + generalised from exactly two panels to however many are declared. + """ + + field: str = "" + mode: PanelMode = "linear" + value_range: tuple[float, float] = (0.0, 1.0) + label: str | None = None + + def __post_init__(self) -> None: + self.value_range = _number_pair(self.value_range, "field_display.panels[].value_range") + + def validate(self, index: int) -> None: + _require_str(self.field, f"field_display.panels[{index}].field") + if not self.field: + raise ValueError(f"field_display.panels[{index}].field must be a non-empty string") + if self.mode not in _VALID_PANEL_MODES: + raise ValueError( + f"field_display.panels[{index}].mode must be one of " + f"{sorted(_VALID_PANEL_MODES)}, got {self.mode!r}" + ) + v_min, v_max = self.value_range + if v_max <= v_min: + raise ValueError( + f"field_display.panels[{index}].value_range must have max > min, " + f"got {self.value_range}" + ) + if self.label is not None: + _require_str(self.label, f"field_display.panels[{index}].label") + @dataclass class FieldDisplayConfig: @@ -279,12 +348,29 @@ class FieldDisplayConfig: (Stage 4 onward) construct fields directly in Python, where the general callable API already applies in full. - `low_color`/`high_color`/`value_range` parameterise the scalar - colour ramp (`src/pyflow/rendering/field_visualization.py`'s - `scalar_field_colors`); `arrow_color`/`arrow_scale` the vector - arrows. `show_legend` toggles the legend strip -- its screen + `low_color`/`high_color`/`value_range` parameterise the *static* + scalar colour ramp (`src/pyflow/rendering/field_visualization.py`'s + `scalar_field_colors`, used by `scalar_pattern` above) -- + `arrow_color`/`arrow_scale` the vector arrows, shared by both the + static and live paths since arrows have no per-panel counterpart to + `panels` below. `show_legend` toggles the legend strip -- its screen position is computed from the mesh's own bounding box, not - separately configurable, keeping this schema small. + separately configurable, keeping this schema small. `low_color`/ + `high_color` are also what every live panel below is drawn with: + one shared palette across every panel a run declares, not a + per-panel colour choice -- nothing has asked for that yet (P-016). + + **`render_field: str | None` and `show_equalized_panel: bool` lived + here until 2026-09-07, when both were replaced by `panels` below.** + That pair could only ever show one declared field, optionally + twice (linear, and rank-equalized) -- added directly at a user's + request for each panel's visibility to be independently + configurable "in a modular fashion", specifically so a future run + could show *different* fields side by side rather than one field + twice. A configuration still setting either name is rejected at + load with a named error pointing here, the same "malformed input + produces a field-named error" migration shape `NumericsConfig. + diffusion_coefficient`'s own move to `FluidConfig` already used. """ scalar_pattern: ScalarDisplayPattern | None = None @@ -295,28 +381,19 @@ class FieldDisplayConfig: arrow_color: str = "#ffffff" arrow_scale: float = 0.3 show_legend: bool = True - render_field: str | None = None - """The declared field (`PyFlowConfig.fields`, TASK-042) whose live - colour map `bootstrap.py` renders -- `None` (the default) renders - none. A separate field from `scalar_pattern` above, deliberately: - that one seeds a synthetic static pattern for a demo with no live - simulation; this one selects among fields a run actually transports. - Named explicitly rather than inferred (first declared, alphabetical) - -- with one field there was nothing to choose, with several there - is, and inferring it is a rule a reader has to know rather than - read. Cross-checked against `PyFlowConfig.fields` in - `_validate_field_declarations` below, not here: this class alone - cannot see what `fields:` declares. - """ field_label: str | None = None - """A human-readable legend caption (Stage 7, Rendering Annotations -- - e.g. `"Temperature (K)"`), shown above the legend strip - (`rendering/hud.py`'s `build_legend_labels`). `None` (the default) - falls back to `render_field`'s own field name -- a separate field - from `render_field` deliberately, since that one is an internal - transport-path key (`engine/simulation.py`'s `state` mapping) and not - always what a viewer should read on screen. + """A human-readable legend caption for the *static* `scalar_pattern` + display only (Stage 7, Rendering Annotations -- e.g. `"Distance from + centre"`), shown above the legend strip (`rendering/hud.py`'s + `build_legend_labels`). `None` (the default) shows no caption at + all for that path -- there is no field name to fall back to the way + a live panel falls back to its own `field` (`FieldPanelConfig. + label`, below): `scalar_pattern` seeds a synthetic pattern with no + underlying declared field. The live per-panel display has its own, + separate caption mechanism (`FieldPanelConfig.label`) precisely + because a single top-level caption stopped making sense the moment + more than one field could be shown. """ vector_label: str | None = None @@ -324,13 +401,23 @@ class FieldDisplayConfig: real user feedback that arrows alone give no way to read direction's *meaning* or magnitude's *scale* -- e.g. `"Velocity"`). `None` (the default) shows no vector-scale HUD line at all -- there is no - internal field name to fall back to the way `field_label` falls back - to `render_field`, since velocity-only live rendering - (`_add_solved_velocity_rendering`) has no `FieldConfig` of its own to - name. When set, `bootstrap.py`'s HUD adds a line stating this label - and `arrow_scale` (`"{vector_label}: length = {arrow_scale} x - magnitude"`) wherever arrows are drawn -- static (`vector_pattern`) - or live (a solved velocity field rendered as arrows) alike. + internal field name to fall back to, since velocity-only live + rendering (`_add_solved_velocity_rendering`) has no `FieldConfig` of + its own to name. When set, `bootstrap.py`'s HUD adds a line stating + this label and `arrow_scale` (`"{vector_label}: length = + {arrow_scale} x magnitude"`) wherever arrows are drawn -- static + (`vector_pattern`) or live (a solved velocity field rendered as + arrows) alike. + """ + + panels: list[FieldPanelConfig] = field(default_factory=list) + """The live-run field display, modular and multi-panel -- see + `FieldPanelConfig`'s own docstring for the full shape and design + history. `[]` (the default) draws nothing, the same as the old + `render_field: null` did. Cross-checked against `PyFlowConfig.fields` + (every panel's `field` must name a real declaration) in + `_validate_field_declarations`, not in `FieldDisplayConfig.validate` + below -- this class alone cannot see what `fields:` declares. """ def __post_init__(self) -> None: @@ -375,12 +462,12 @@ def validate(self) -> None: raise ValueError( f"field_display.show_legend must be true or false, got {self.show_legend!r}" ) - if self.render_field is not None: - _require_str(self.render_field, "field_display.render_field") if self.field_label is not None: _require_str(self.field_label, "field_display.field_label") if self.vector_label is not None: _require_str(self.vector_label, "field_display.vector_label") + for index, panel in enumerate(self.panels): + panel.validate(index) ScalarTransportPattern = Literal["gaussian_blob", "sinusoidal_mode"] @@ -415,11 +502,13 @@ class SimulationConfig: `None` (the default) means no prescribed velocity pattern -- every existing demo (`field_display`, `numerics_assembly`) is unaffected. Colouring a live field reuses `field_display.low_color`/`high_color`/ - `value_range`/`show_legend` as-is, deliberately not duplicated here: - those already answer "how is a scalar field coloured", a question - this section has no reason to answer twice; `field_display. - render_field` (also TASK-042) is what selects *which* declared field - that colouring applies to, now that more than one can exist. + `show_legend` as-is, deliberately not duplicated here: those already + answer "how is a scalar field coloured", a question this section has + no reason to answer twice; `field_display.panels` (originally + `render_field`, TASK-042; replaced by the modular panel list + 2026-09-07 -- see `FieldPanelConfig`'s own docstring) is what selects + *which* declared field(s) get coloured, and each panel's own + `value_range` decides its linear mapping. `velocity` is a prescribed (not solved) constant vector by default -- `velocity_solved` (TASK-031, added 2026-08-29) is what lets a run ask @@ -578,11 +667,13 @@ def has_buoyancy_coupling(self) -> bool: # that could drift independently of this constant. -def _validate_field_declarations(fields: Sequence[FieldConfig], render_field: str | None) -> None: +def _validate_field_declarations( + fields: Sequence[FieldConfig], panels: Sequence[FieldPanelConfig] +) -> None: """The whole-`fields:`-list checks no single declaration can make on its own: no two declarations share a name, no declaration's name collides with a fixed engine name it would silently become, and - `field_display.render_field` (if set) actually names one of them. + every `field_display.panels[].field` actually names one of them. Same shape as `_validate_boundary_conditions_jointly` above -- a module-level function called from `PyFlowConfig.validate()`, not a method on any one `FieldConfig`, since none of these are checkable @@ -599,11 +690,12 @@ def _validate_field_declarations(fields: Sequence[FieldConfig], render_field: st if declared.name in seen: raise ValueError(f"fields declares {declared.name!r} more than once") seen.add(declared.name) - if render_field is not None and render_field not in seen: - raise ValueError( - f"field_display.render_field {render_field!r} does not name a declared field " - f"(declared: {sorted(seen)})" - ) + for index, panel in enumerate(panels): + if panel.field not in seen: + raise ValueError( + f"field_display.panels[{index}].field {panel.field!r} does not name a declared " + f"field (declared: {sorted(seen)})" + ) _NO_SOURCE_TERM: SourceTermName = "none" @@ -1196,7 +1288,7 @@ def validate(self) -> None: self.units.validate() self.recording.validate() _validate_boundary_conditions_jointly(self.mesh, self.numerics.boundary_conditions) - _validate_field_declarations(self.fields, self.field_display.render_field) + _validate_field_declarations(self.fields, self.field_display.panels) _validate_buoyancy_couplings( self.fields, self.simulation.velocity_solved, self.numerics.source_term ) diff --git a/src/pyflow/rendering/CLAUDE.md b/src/pyflow/rendering/CLAUDE.md index 1b3d122..4244558 100644 --- a/src/pyflow/rendering/CLAUDE.md +++ b/src/pyflow/rendering/CLAUDE.md @@ -434,11 +434,19 @@ strip at all -- the live-stepping path (`_add_declared_field_transport`) colour-mapped a declared field every frame with no legend beside it, which is exactly backwards from what a viewer watching a live run needs most. `_add_legend(window, field_display, mesh_bounds) -> _Bounds | None` -is the shared strip-drawing logic both paths now call, so a live run -with `field_display.render_field` set gets the same labelled legend a -static demo does. `_add_solved_velocity_rendering` (arrows only, no -scalar) always returns `None` for its own legend bounds -- there is -nothing to label. +was the shared strip-drawing logic both paths called at the time, so a +live run with `field_display.render_field` set got the same labelled +legend a static demo does. `_add_solved_velocity_rendering` (arrows +only, no scalar) always returns `None` for its own legend bounds -- +there is nothing to label. + +**No longer shared, since 2026-09-07's modular panel list.** `_add_legend` +is now static-only; every live panel builds its own legend directly via +`_add_panel_legend` instead (this file's own "Equalized (rank-based) +field panel" entry, below), since a run can now declare several panels +of several different fields with no single "the" legend left for one +shared function to build. `_add_field_display`'s own use of `_add_legend` +is unaffected. **The HUD activates on its own, independent of what else is configured -- reversed the same day it first shipped, after real user @@ -703,3 +711,82 @@ shaft from the tail instead -- clear of both the arrowhead cluster near the tip and the tail endpoint's own known rasterisation artefact -- rather than continuing to widen a tolerance meant for a different effect. + +## Equalized (rank-based) field panel, added 2026-09-07 + +Not tied to any roadmap stage or task -- rendering/visualisation work +requested directly by a user watching the Smoke Transport demo, the same +"not physics, not `adr/ADR-007-executable-acceptance-criteria.md`-gated" +category the HUD/axis-label work above already falls into. + +**`field_visualization.rank_scalar_field_colors(field, low_color, +high_color)`** colours each cell by its *rank* among the field's current +values (histogram equalization), not by magnitude -- the field's current +smallest value always maps to `low_color` and its largest always to +`high_color`, however close together the two are in absolute terms. Tied +values get the *average* of the ranks they'd otherwise split (a +perfectly uniform field maps to one shared midpoint colour, not an +arbitrary spread), and a single-cell field -- nothing to rank against -- +is defined to map to `low_color`. No range, floor/ceiling, or percentile +parameter, unlike `scalar_field_colors`'s `value_range`: rank is +invariant to any monotonic rescaling of the underlying values, so it has +nothing left to tune. + +**`bootstrap.py`'s `config.field_display.panels` (originally a single +`show_equalized_panel: bool` toggle on one `render_field`; generalised +to a modular panel list the same day, below)** wires an `"equalized"`- +mode panel into a live run as one colour-mapped panel among however +many `panels` declares (`_add_declared_field_transport`, +`_add_panel_legend`), rebuilt every frame the same "remove old, build +new" way every panel already is. The legend's two numeric end-labels +show the field's own current min/max *value*, for context only -- the +gradient strip between them represents equal steps of rank, not equal +steps of value, so a value exactly halfway between the two labelled +numbers is not, in general, the value coloured at the strip's midpoint. +**The caption defaults to the plain string `"equalized"`, never the +field name repeated with a suffix** -- an adjacent linear panel of the +same field already captions itself by name, so this one only needs to +say what's different about it; found worth deciding explicitly, not +just simply, after noticing `f"{field} (equalized)"` risked exactly the +wrapped-caption-drawn-over-the-mesh defect this file's own "not fixed +here" note above already documents once, the moment a demo's own field +name/label was long enough (`smoke_transport.yaml`'s "Smoke +concentration (model units)"). An explicit `FieldPanelConfig.label` +always overrides this default outright (`_panel_caption`, +`bootstrap.py`). + +**Two earlier designs were tried and rejected first, both on real user +feedback against real rendered frames of the Smoke Transport demo, not +decided in the abstract.** A fixed `value_range`-derived log10 ceiling +washed out once the field decayed below it. A live floor/ceiling from +each frame's own percentile-trimmed positive values (still log10) fixed +that, but not the real complaint -- "peaks and valleys... both quite +high above sea level": two values close together in *magnitude*, however +that magnitude is scaled, still compress toward one shade, because a +magnitude-based scale answers "how big is this value," never "how does +this value compare to its neighbours right now." Rank answers exactly +that second question, and is what made the log10 transform in both +earlier designs redundant once adopted (ranking `log(x)` gives the same +order as ranking `x`, since log is monotonic) -- see +`rank_scalar_field_colors`'s own docstring and `bootstrap.py`'s +`_add_declared_field_transport` docstring for the full history. + +**The panel list itself is a third, later change, same day, at a +further user request: "can we have the visibility [of] each of these +plots configurable too in a modular fashion? Later we may want [to] +show different fields than concentration too."** `show_equalized_panel` +could only ever add a *second* panel of the *same* field +`render_field` already named -- no way to show two different fields +side by side, and no way to turn the first (linear) panel off +independently of the second. `FieldDisplayConfig.panels: list[ +FieldPanelConfig]` replaces both: each panel is a full, independent +declaration (`field`, `mode`, `value_range`, `label`), drawn left to +right in list order, any number of them, each naming its own field. +`bootstrap.py`'s own `_PanelRenderState`/`_panel_colors`/ +`_add_panel_legend` are the generalised mechanism -- one panel-building +loop instead of one hardcoded linear-panel block plus one hardcoded +equalized-panel block. `[]` (the default) draws nothing, the same as +`render_field: null` used to. See `src/pyflow/configuration/CLAUDE.md`'s +`FieldDisplayConfig.render_field` entry for the schema side of this +migration, including the 6 golden demos it required migrating and the +load-error a config still setting either retired field now gets. diff --git a/src/pyflow/rendering/field_visualization.py b/src/pyflow/rendering/field_visualization.py index 65a1e77..a5993ef 100644 --- a/src/pyflow/rendering/field_visualization.py +++ b/src/pyflow/rendering/field_visualization.py @@ -77,6 +77,44 @@ def scalar_field_colors( return _map_values_to_colors(field.values.numpy(), low_color, high_color, value_range) +def rank_scalar_field_colors(field: ScalarField, low_color: str, high_color: str) -> np.ndarray: + """`scalar_field_colors`'s own counterpart for histogram-equalised + (rank-based) colouring: each cell is coloured by its *rank* among + the field's current values, not by its magnitude -- the lowest value + this frame always maps to `low_color`, the highest always to + `high_color`, evenly spaced by rank in between. No floor, ceiling, or + percentile parameter is needed, unlike a magnitude-based scale + (`scalar_field_colors`'s fixed `value_range`, or an earlier + log10/percentile-trimmed design this replaced) -- rank is invariant + to distribution shape and to any monotonic transform of the values + (in particular, ranking `log(x)` gives the same order as ranking `x` + itself), so it can't wash out: whatever the field's peaks and valleys + are, this frame's smallest and largest always get full contrast. + + Tied values receive the *average* of the ranks they'd otherwise + split -- found necessary, not just tidier: without it, a perfectly + uniform field (every cell equal, no real ordering to speak of) would + have its ties broken arbitrarily and get spread across the entire + low-to-high range for no real reason. Averaging ties instead maps a + uniform field to one shared midpoint colour, and a field with `n` + unique values but many repeats still gets its `n` distinct colours + correctly spaced. + + A single-cell field has nothing to be ranked against; defined to map + to `low_color` rather than raising or dividing by zero. + """ + values = field.values.numpy() + n = values.size + if n <= 1: + return _map_values_to_colors(np.zeros(n), low_color, high_color, (0.0, 1.0)) + unique_values, inverse, counts = np.unique(values, return_inverse=True, return_counts=True) + block_starts = np.cumsum(counts) - counts + average_rank_per_unique_value = block_starts + (counts - 1) / 2.0 + ranks = average_rank_per_unique_value[inverse] + normalized_ranks = ranks / (n - 1) + return _map_values_to_colors(normalized_ranks, low_color, high_color, (0.0, 1.0)) + + def _cell_corners(mesh: Mesh, cell: int) -> np.ndarray: """`cell`'s four corners, `(4, 2)`, ordered bottom-left/bottom-right/ top-right/top-left -- derived generically from `face_vertices` over diff --git a/tests/features/field_declaration.feature b/tests/features/field_declaration.feature index 732c281..9f893a2 100644 --- a/tests/features/field_declaration.feature +++ b/tests/features/field_declaration.feature @@ -61,12 +61,12 @@ Feature: Field Declaration Configuration When the configuration is loaded Then loading is rejected with a named error naming the field and the valid initial conditions - Scenario: Naming which declared field the renderer colours produces that field's colour map - Given a configuration declaring two named fields and naming one of them as field_display.render_field + Scenario: Naming which declared field a display panel colours produces that field's colour map + Given a configuration declaring two named fields and a field_display.panels entry naming one of them When the configuration is loaded and run for one real timestep Then the named field's own colour map is rendered and the other field's is not - Scenario: Naming an undeclared field as the renderer's field is rejected - Given a configuration whose field_display.render_field names a field nothing declares + Scenario: Naming an undeclared field in a display panel is rejected + Given a configuration whose field_display.panels entry names a field nothing declares When the configuration is loaded Then loading is rejected with a named error naming the undeclared field diff --git a/tests/unit/test_bootstrap.py b/tests/unit/test_bootstrap.py index 21ec707..812d4ae 100644 --- a/tests/unit/test_bootstrap.py +++ b/tests/unit/test_bootstrap.py @@ -315,12 +315,12 @@ def test_bootstrap_scalar_display_legend_disabled_adds_no_numeric_labels(tmp_pat assert "5" not in contents -def test_bootstrap_legend_field_label_defaults_to_render_field_name(tmp_path: Path) -> None: +def test_bootstrap_legend_caption_defaults_to_panel_field_name(tmp_path: Path) -> None: config_file = tmp_path / "config.yaml" config_file.write_text( "rendering:\n backend: offscreen\n" "fields:\n - name: temperature\n initial_condition: gaussian_blob\n" - "field_display:\n render_field: temperature\n" + "field_display:\n panels:\n - field: temperature\n" ) window = bootstrap(config_file, max_frames=1) @@ -329,12 +329,12 @@ def test_bootstrap_legend_field_label_defaults_to_render_field_name(tmp_path: Pa assert "temperature" in contents -def test_bootstrap_legend_field_label_overrides_render_field_name(tmp_path: Path) -> None: +def test_bootstrap_legend_caption_overrides_panel_field_name(tmp_path: Path) -> None: config_file = tmp_path / "config.yaml" config_file.write_text( "rendering:\n backend: offscreen\n" "fields:\n - name: temperature\n initial_condition: gaussian_blob\n" - "field_display:\n render_field: temperature\n field_label: Temperature (K)\n" + "field_display:\n panels:\n - field: temperature\n label: Temperature (K)\n" ) window = bootstrap(config_file, max_frames=1) @@ -344,6 +344,230 @@ def test_bootstrap_legend_field_label_overrides_render_field_name(tmp_path: Path assert "temperature" not in contents +def _field_mesh_children(scene: gfx.Scene, num_cells: int) -> list[gfx.Mesh]: + """`gfx.Mesh` scene children built by `build_scalar_field_mesh` for a + field over a mesh of `num_cells` cells -- `num_cells * 2` triangles, + which distinguishes a field's own colour-mapped mesh from the + legend's fixed-32-quad (64-triangle) gradient strip regardless of + how many cells the field's own mesh has, as long as neither equals + the other (true for every mesh size these tests use). + """ + return [ + child + for child in scene.children + if isinstance(child, gfx.Mesh) and child.geometry.indices.data.shape[0] == num_cells * 2 + ] + + +def test_bootstrap_with_no_panels_adds_no_field_mesh(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text( + "rendering:\n backend: offscreen\n" + "mesh:\n extent: [4, 3]\n" + "fields:\n - name: smoke\n initial_condition: gaussian_blob\n" + ) + + window = bootstrap(config_file, max_frames=1) + + assert len(_field_mesh_children(window.scene, num_cells=12)) == 0 + + +def test_bootstrap_with_one_panel_adds_one_field_mesh(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text( + "rendering:\n backend: offscreen\n" + "mesh:\n extent: [4, 3]\n" + "fields:\n - name: smoke\n initial_condition: gaussian_blob\n" + "field_display:\n panels:\n - field: smoke\n" + ) + + window = bootstrap(config_file, max_frames=1) + + assert len(_field_mesh_children(window.scene, num_cells=12)) == 1 + + +def test_bootstrap_with_two_panels_adds_two_field_meshes_shifted_right( + tmp_path: Path, +) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text( + "rendering:\n backend: offscreen\n" + "mesh:\n extent: [4, 3]\n" + "fields:\n - name: smoke\n initial_condition: gaussian_blob\n" + "field_display:\n panels:\n - field: smoke\n" + " - field: smoke\n mode: equalized\n" + ) + + window = bootstrap(config_file, max_frames=1) + + meshes = _field_mesh_children(window.scene, num_cells=12) + assert len(meshes) == 2 + positions_x = sorted(float(mesh.local.position[0]) for mesh in meshes) + assert positions_x[0] == pytest.approx(0.0) + assert positions_x[1] > 0.0, "the second panel must sit to the right of the first" + + +def test_bootstrap_panels_can_show_different_fields(tmp_path: Path) -> None: + """The whole point of the modular panel list: each panel names its + own field independently, so a run can show several *different* + fields side by side, not only one field coloured two ways. + """ + config_file = tmp_path / "config.yaml" + config_file.write_text( + "rendering:\n backend: offscreen\n" + "mesh:\n extent: [4, 3]\n" + "fields:\n - name: smoke\n initial_condition: gaussian_blob\n" + " - name: heat\n initial_condition: sinusoidal_mode\n" + "field_display:\n panels:\n" + " - field: smoke\n label: Smoke\n" + " - field: heat\n label: Heat\n" + ) + + window = bootstrap(config_file, max_frames=1) + + meshes = _field_mesh_children(window.scene, num_cells=12) + assert len(meshes) == 2 + contents = [_text_content(t) for t in _text_children(window.scene)] + assert "Smoke" in contents + assert "Heat" in contents + + +def test_bootstrap_equalized_panel_legend_caption_is_just_equalized_not_the_field_label( + tmp_path: Path, +) -> None: + """An equalized panel's own default legend caption is deliberately + just "equalized", never the field name repeated with a suffix -- + repeating a long field name/label risked the wrapped-caption-drawn- + over-the-mesh defect this project's HUD history already hit once + (`src/pyflow/rendering/CLAUDE.md`'s "Equalized (rank-based) field + panel" entry), and the linear panel's own legend already names the + field. + """ + config_file = tmp_path / "config.yaml" + config_file.write_text( + "rendering:\n backend: offscreen\n" + "mesh:\n extent: [4, 3]\n" + "fields:\n - name: smoke\n initial_condition: gaussian_blob\n" + "field_display:\n panels:\n" + " - field: smoke\n label: Smoke concentration (model units)\n" + " - field: smoke\n mode: equalized\n" + ) + + window = bootstrap(config_file, max_frames=1) + + contents = [_text_content(t) for t in _text_children(window.scene)] + assert "equalized" in contents + assert not any("Smoke concentration" in c and "equalized" in c for c in contents) + + +def test_bootstrap_equalized_panel_with_legend_disabled_adds_no_equalized_legend( + tmp_path: Path, +) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text( + "rendering:\n backend: offscreen\n" + "mesh:\n extent: [4, 3]\n" + "fields:\n - name: smoke\n initial_condition: gaussian_blob\n" + "field_display:\n show_legend: false\n panels:\n" + " - field: smoke\n - field: smoke\n mode: equalized\n" + ) + + window = bootstrap(config_file, max_frames=1) + + assert len(_field_mesh_children(window.scene, num_cells=12)) == 2 + contents = [_text_content(t) for t in _text_children(window.scene)] + assert "equalized" not in contents + + +def test_bootstrap_panel_stats_block_does_not_overlap_the_legend_caption( + tmp_path: Path, +) -> None: + """Real bug, found by a user running `smoke_transport_mesh128.yaml`: + "the legends all clip over each other." A single-panel run's own + legend caption and the stats block below it were drawn at the same + world-space height -- `_add_declared_field_transport` widened + `overall_bounds` rightward for extra panels, but never downward for + a panel's own legend/caption, so `_add_hud`'s stats block (placed + just below whatever `bounds` it's handed) started from the mesh's + own bare bottom edge, exactly where the caption already sat. Two + panels happened not to show it in earlier manual renders only + because a passing wide reading distracted from it, not because the + bug depends on panel count -- this reproduces the single-panel case, + the simplest one that has it. + """ + config_file = tmp_path / "config.yaml" + config_file.write_text( + "rendering:\n backend: offscreen\n" + "mesh:\n extent: [4, 3]\n" + "fields:\n - name: smoke\n initial_condition: gaussian_blob\n" + "field_display:\n panels:\n - field: smoke\n" + ) + + window = bootstrap(config_file, max_frames=1) + + legend_mesh = next( + child + for child in window.scene.children + if isinstance(child, gfx.Mesh) and child.geometry.indices.data.shape[0] == 32 * 2 + ) + legend_bottom_y = float(legend_mesh.geometry.positions.data[:, 1].min()) + stats_text = next(t for t in _text_children(window.scene) if "cell" in _text_content(t).lower()) + stats_y = float(stats_text.local.position[1]) + + assert stats_y < legend_bottom_y, ( + f"stats block (y={stats_y}) must sit below the legend strip's own bottom edge " + f"(y={legend_bottom_y}), not overlap the caption drawn just above it" + ) + + +def test_bootstrap_panels_widen_the_camera_framing(tmp_path: Path) -> None: + base_config = tmp_path / "base.yaml" + base_config.write_text( + "rendering:\n backend: offscreen\n" + "mesh:\n extent: [4, 3]\n" + "fields:\n - name: smoke\n initial_condition: gaussian_blob\n" + "field_display:\n panels:\n - field: smoke\n" + ) + two_panel_config = tmp_path / "two_panels.yaml" + two_panel_config.write_text( + "rendering:\n backend: offscreen\n" + "mesh:\n extent: [4, 3]\n" + "fields:\n - name: smoke\n initial_condition: gaussian_blob\n" + "field_display:\n panels:\n - field: smoke\n - field: smoke\n mode: equalized\n" + ) + + base_window = bootstrap(base_config, max_frames=1) + two_panel_window = bootstrap(two_panel_config, max_frames=1) + + assert two_panel_window.camera.width > base_window.camera.width + + +def test_bootstrap_panel_field_meshes_are_rebuilt_not_accumulated_across_frames( + tmp_path: Path, +) -> None: + """Exercises `_advance`'s own per-frame panel-rebuild path -- every + panel is removed and rebuilt every frame (`bootstrap.py`'s own + "remove old, build new" convention), not accumulated as a growing + pile of stale meshes. + """ + config_file = tmp_path / "config.yaml" + config_file.write_text( + "rendering:\n backend: offscreen\n" + "mesh:\n extent: [4, 3]\n" + "fields:\n - name: smoke\n initial_condition: gaussian_blob\n" + "field_display:\n panels:\n - field: smoke\n - field: smoke\n mode: equalized\n" + "simulation:\n velocity_solved: true\n" + "numerics:\n boundary_conditions:\n north:\n type: dirichlet\n" + " field_values:\n velocity.0: 1.0\n" + " south:\n type: dirichlet\n east:\n type: dirichlet\n" + " west:\n type: dirichlet\n" + ) + + window = bootstrap(config_file, max_frames=5) + + assert len(_field_mesh_children(window.scene, num_cells=12)) == 2 + + def test_bootstrap_stats_use_configured_physical_units(tmp_path: Path) -> None: config_file = tmp_path / "config.yaml" config_file.write_text( diff --git a/tests/unit/test_configuration.py b/tests/unit/test_configuration.py index bc27158..8d667de 100644 --- a/tests/unit/test_configuration.py +++ b/tests/unit/test_configuration.py @@ -34,9 +34,9 @@ def test_defaults_are_valid() -> None: assert config.field_display.arrow_color == "#ffffff" assert config.field_display.arrow_scale == 0.3 assert config.field_display.show_legend is True - assert config.field_display.render_field is None assert config.field_display.field_label is None assert config.field_display.vector_label is None + assert config.field_display.panels == [] assert config.fields == [] assert config.simulation.velocity_pattern is None assert config.simulation.velocity == (1.0, 0.0) @@ -683,8 +683,8 @@ def test_load_config_rejects_a_non_numeric_gravity_component(tmp_path: Path) -> # -- FieldConfig / fields: (TASK-042) -------------------------------------- # # The higher-level, cross-field claims (duplicate names, reserved-name -# collisions, the simulation.scalar_pattern migration, field_display. -# render_field naming an undeclared field) are +# collisions, the simulation.scalar_pattern migration, a +# field_display.panels[].field naming an undeclared field) are # `tests/features/field_declaration.feature` # (`tests/unit/test_field_declaration_configuration.py`); per-field type # validation stays here beside every other field's own, the same split @@ -758,52 +758,161 @@ def test_load_config_rejects_a_non_numeric_field_diffusion_coefficient(tmp_path: load_config(config_file) -def test_load_config_reads_render_field(tmp_path: Path) -> None: +def test_load_config_reads_field_label(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text("field_display:\n field_label: Temperature (K)\n") + + assert load_config(config_file).field_display.field_label == "Temperature (K)" + + +def test_load_config_rejects_a_non_string_field_label(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text("field_display:\n field_label: 5\n") + + with pytest.raises(ValueError, match="field_label"): + load_config(config_file) + + +def test_load_config_reads_vector_label(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text("field_display:\n vector_label: Velocity\n") + + assert load_config(config_file).field_display.vector_label == "Velocity" + + +def test_load_config_rejects_a_non_string_vector_label(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text("field_display:\n vector_label: 5\n") + + with pytest.raises(ValueError, match="vector_label"): + load_config(config_file) + + +# -- FieldDisplayConfig.panels --------------------------------------------- +# +# The higher-level, cross-field claim (a panel's own `field` naming an +# undeclared field) is `tests/features/field_declaration.feature` +# (`tests/unit/test_field_declaration_configuration.py`), the same split +# `fields:` above already uses; per-panel type/shape validation stays +# here. + + +def test_load_config_reads_panels(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text( + "fields:\n - name: temperature\nfield_display:\n panels:\n" + " - field: temperature\n mode: equalized\n" + " value_range: [1.0, 2.0]\n label: Temperature (K)\n" + ) + + config = load_config(config_file) + + assert len(config.field_display.panels) == 1 + panel = config.field_display.panels[0] + assert panel.field == "temperature" + assert panel.mode == "equalized" + assert panel.value_range == (1.0, 2.0) + assert panel.label == "Temperature (K)" + + +def test_load_config_panels_defaults_to_an_empty_list(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text("rendering:\n backend: offscreen\n") + + config = load_config(config_file) + + assert config.field_display.panels == [] + + +def test_load_config_panel_mode_defaults_to_linear(tmp_path: Path) -> None: config_file = tmp_path / "config.yaml" config_file.write_text( - "fields:\n - name: temperature\nfield_display:\n render_field: temperature\n" + "fields:\n - name: temperature\nfield_display:\n panels:\n - field: temperature\n" ) config = load_config(config_file) - assert config.field_display.render_field == "temperature" + assert config.field_display.panels[0].mode == "linear" -def test_load_config_rejects_a_non_string_render_field(tmp_path: Path) -> None: +def test_load_config_rejects_a_non_list_panels_section(tmp_path: Path) -> None: config_file = tmp_path / "config.yaml" - config_file.write_text("field_display:\n render_field: 5\n") + config_file.write_text("field_display:\n panels:\n field: temperature\n") - with pytest.raises(ValueError, match="render_field"): + with pytest.raises(ValueError, match="panels"): load_config(config_file) -def test_load_config_reads_field_label(tmp_path: Path) -> None: +def test_load_config_rejects_a_non_mapping_panel_declaration(tmp_path: Path) -> None: config_file = tmp_path / "config.yaml" - config_file.write_text("field_display:\n field_label: Temperature (K)\n") + config_file.write_text("field_display:\n panels:\n - temperature\n") - assert load_config(config_file).field_display.field_label == "Temperature (K)" + with pytest.raises(ValueError, match=r"panels\[0\]"): + load_config(config_file) -def test_load_config_rejects_a_non_string_field_label(tmp_path: Path) -> None: +def test_load_config_rejects_an_empty_panel_field(tmp_path: Path) -> None: config_file = tmp_path / "config.yaml" - config_file.write_text("field_display:\n field_label: 5\n") + config_file.write_text("field_display:\n panels:\n - field: ''\n") - with pytest.raises(ValueError, match="field_label"): + with pytest.raises(ValueError, match=r"panels\[0\].field"): load_config(config_file) -def test_load_config_reads_vector_label(tmp_path: Path) -> None: +def test_load_config_rejects_a_non_string_panel_field(tmp_path: Path) -> None: config_file = tmp_path / "config.yaml" - config_file.write_text("field_display:\n vector_label: Velocity\n") + config_file.write_text("field_display:\n panels:\n - field: 5\n") - assert load_config(config_file).field_display.vector_label == "Velocity" + with pytest.raises(ValueError, match=r"panels\[0\].field"): + load_config(config_file) -def test_load_config_rejects_a_non_string_vector_label(tmp_path: Path) -> None: +def test_load_config_rejects_an_invalid_panel_mode(tmp_path: Path) -> None: config_file = tmp_path / "config.yaml" - config_file.write_text("field_display:\n vector_label: 5\n") + config_file.write_text( + "fields:\n - name: temperature\n" + "field_display:\n panels:\n - field: temperature\n mode: logarithmic\n" + ) - with pytest.raises(ValueError, match="vector_label"): + with pytest.raises(ValueError, match=r"panels\[0\].mode"): + load_config(config_file) + + +def test_load_config_rejects_a_degenerate_panel_value_range(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text( + "fields:\n - name: temperature\n" + "field_display:\n panels:\n - field: temperature\n value_range: [5.0, 1.0]\n" + ) + + with pytest.raises(ValueError, match=r"panels\[0\].value_range"): + load_config(config_file) + + +def test_load_config_rejects_a_non_string_panel_label(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text( + "fields:\n - name: temperature\n" + "field_display:\n panels:\n - field: temperature\n label: 5\n" + ) + + with pytest.raises(ValueError, match=r"panels\[0\].label"): + load_config(config_file) + + +def test_load_config_rejects_the_retired_render_field_setting(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text("field_display:\n render_field: temperature\n") + + with pytest.raises(ValueError, match="field_display.panels"): + load_config(config_file) + + +def test_load_config_rejects_the_retired_show_equalized_panel_setting(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text("field_display:\n show_equalized_panel: true\n") + + with pytest.raises(ValueError, match="field_display.panels"): load_config(config_file) diff --git a/tests/unit/test_field_declaration_configuration.py b/tests/unit/test_field_declaration_configuration.py index 8e2245d..c8f8615 100644 --- a/tests/unit/test_field_declaration_configuration.py +++ b/tests/unit/test_field_declaration_configuration.py @@ -50,14 +50,14 @@ class _Context: ) -def _render_field_config(render_field: str) -> str: +def _panel_config(field_name: str) -> str: return ( "rendering:\n backend: offscreen\n" "mesh:\n extent: [4, 4]\n" "fields:\n" " - name: alpha\n initial_condition: gaussian_blob\n" " - name: beta\n initial_condition: sinusoidal_mode\n" - f"field_display:\n render_field: {render_field}\n" + f"field_display:\n panels:\n - field: {field_name}\n" ) @@ -138,27 +138,27 @@ def _given_unrecognised_initial_condition(tmp_path: Path) -> _Context: @given( - "a configuration declaring two named fields and naming one of them as " - "field_display.render_field", + "a configuration declaring two named fields and a field_display.panels entry naming one " + "of them", target_fixture="ctx", ) -def _given_render_field_selected(tmp_path: Path) -> _Context: +def _given_panel_field_selected(tmp_path: Path) -> _Context: config_path = tmp_path / "selected.yaml" - config_path.write_text(_render_field_config("alpha")) + config_path.write_text(_panel_config("alpha")) alternate_path = tmp_path / "alternate.yaml" - alternate_path.write_text(_render_field_config("beta")) + alternate_path.write_text(_panel_config("beta")) return _Context(config_path=config_path, alternate_config_path=alternate_path) @given( - "a configuration whose field_display.render_field names a field nothing declares", + "a configuration whose field_display.panels entry names a field nothing declares", target_fixture="ctx", ) -def _given_undeclared_render_field(tmp_path: Path) -> _Context: +def _given_undeclared_panel_field(tmp_path: Path) -> _Context: config_path = tmp_path / "config.yaml" config_path.write_text( "fields:\n - name: alpha\n initial_condition: gaussian_blob\n" - "field_display:\n render_field: nowhere\n" + "field_display:\n panels:\n - field: nowhere\n" ) return _Context(config_path=config_path) @@ -264,7 +264,7 @@ def _rendered_meshes(window: RenderWindow) -> list[gfx.Mesh]: return [child for child in window.scene.children if isinstance(child, gfx.Mesh)] -# The `_render_field_config` mesh is 4x4 = 16 cells, two triangles each -- +# The `_panel_config` mesh is 4x4 = 16 cells, two triangles each -- # distinguishes the field-fill mesh from the legend strip (Stage 7, # Rendering Annotations, on by default) below by shape, not by scene # insertion order. Order is not reliable here: `_advance()` (called once @@ -285,20 +285,20 @@ def _field_fill_mesh(window: RenderWindow) -> gfx.Mesh: @then("the named field's own colour map is rendered and the other field's is not") -def _then_render_field_selected(ctx: _Context) -> None: +def _then_panel_field_selected(ctx: _Context) -> None: assert ctx.late_window is not None assert ctx.alternate_window is not None # Two meshes each since Stage 7 (Rendering Annotations): the field - # fill and the legend strip (`bootstrap._add_legend`, on by default -- - # `field_display.show_legend` is not set in this scenario's own - # config). Was exactly one before that stage added a legend to this - # live-stepping path. + # fill and the legend strip (`bootstrap._add_panel_legend`, on by + # default -- `field_display.show_legend` is not set in this + # scenario's own config). Was exactly one before that stage added a + # legend to this live-stepping path. assert len(_rendered_meshes(ctx.late_window)) == 2 assert len(_rendered_meshes(ctx.alternate_window)) == 2 # Different declared fields (a gaussian blob vs. a sinusoidal mode) on - # the same mesh produce different colour maps -- if `render_field` - # were ignored, or always picked the same field regardless of which - # name was configured, both runs would render identical colours. + # the same mesh produce different colour maps -- if the panel's own + # `field` were ignored, or always picked the same field regardless of + # which name was configured, both runs would render identical colours. selected_colors = _field_fill_mesh(ctx.late_window).geometry.colors.data alternate_colors = _field_fill_mesh(ctx.alternate_window).geometry.colors.data assert selected_colors.shape == alternate_colors.shape @@ -306,8 +306,8 @@ def _then_render_field_selected(ctx: _Context) -> None: @then("loading is rejected with a named error naming the undeclared field") -def _then_rejected_naming_undeclared_render_field(ctx: _Context) -> None: - assert ctx.error is not None, "expected load_config to reject the undeclared render_field" +def _then_rejected_naming_undeclared_panel_field(ctx: _Context) -> None: + assert ctx.error is not None, "expected load_config to reject the undeclared panel field" message = str(ctx.error) - assert "field_display.render_field" in message + assert "field_display.panels[0].field" in message assert "nowhere" in message diff --git a/tests/unit/test_field_visualization.py b/tests/unit/test_field_visualization.py index 3d22ab1..c65b9d0 100644 --- a/tests/unit/test_field_visualization.py +++ b/tests/unit/test_field_visualization.py @@ -23,6 +23,7 @@ build_field_legend, build_scalar_field_mesh, build_vector_field_arrows, + rank_scalar_field_colors, scalar_field_colors, ) @@ -87,6 +88,72 @@ def test_scalar_field_colors_rejects_a_degenerate_range() -> None: scalar_field_colors(field, _LOW, _HIGH, value_range=(5.0, 1.0)) +# -- rank_scalar_field_colors ------------------------------------------------ + + +def test_rank_scalar_field_colors_orders_by_rank_not_magnitude() -> None: + """Three values spanning six orders of magnitude get exactly the + same three colours as three evenly-spaced values would -- rank, not + the size of the gap between values, decides the colour. This is the + whole point of the function: a field whose peaks and valleys are + both tiny in absolute terms (e.g. both far down a decay curve) still + gets full low-to-high contrast between them, which no magnitude-based + scale (linear or log, clamped or percentile-trimmed) can guarantee. + """ + mesh = _mesh(nx=3, ny=1) + field = ScalarField(mesh, "s") + field.set_value_at(0, 1e-6) # smallest -> low_color + field.set_value_at(1, 1e-3) # middle rank -> exact midpoint colour + field.set_value_at(2, 1.0) # largest -> high_color + + colors = rank_scalar_field_colors(field, _LOW, _HIGH) + + assert colors[0].tolist() == [10, 20, 30, 255] + assert colors[2].tolist() == [200, 150, 100, 255] + expected_mid = [ + round((lo + hi) / 2) for lo, hi in zip((10, 20, 30, 255), (200, 150, 100, 255), strict=True) + ] + assert colors[1].tolist() == expected_mid + + +def test_rank_scalar_field_colors_shape_and_dtype() -> None: + mesh = _mesh() + field = ScalarField(mesh, "s", initial_value=1.0) + colors = rank_scalar_field_colors(field, _LOW, _HIGH) + assert colors.shape == (mesh.num_cells, 4) + assert colors.dtype == np.uint8 + + +def test_rank_scalar_field_colors_averages_tied_ranks_to_the_midpoint() -> None: + """A perfectly uniform field (every cell equal) has no real ordering + at all -- tied ranks average out to the same midpoint colour for + every cell, rather than an arbitrary tie-break spreading them across + the full low-to-high range for no real reason. + """ + mesh = _mesh(nx=2, ny=1) + field = ScalarField(mesh, "s", initial_value=7.0) + + colors = rank_scalar_field_colors(field, _LOW, _HIGH) + + expected_mid = [ + round((lo + hi) / 2) for lo, hi in zip((10, 20, 30, 255), (200, 150, 100, 255), strict=True) + ] + assert colors[0].tolist() == expected_mid + assert colors[1].tolist() == expected_mid + + +def test_rank_scalar_field_colors_handles_a_single_cell_field() -> None: + """One cell has nothing to be ranked against -- defined to map to + `low_color` rather than raising or dividing by zero. + """ + mesh = _mesh(nx=1, ny=1) + field = ScalarField(mesh, "s", initial_value=42.0) + + colors = rank_scalar_field_colors(field, _LOW, _HIGH) + + assert colors[0].tolist() == [10, 20, 30, 255] + + # -- build_scalar_field_mesh ----------------------------------------------- diff --git a/tests/unit/test_golden_demo_annotations.py b/tests/unit/test_golden_demo_annotations.py index 4a1ec1b..06fd2dc 100644 --- a/tests/unit/test_golden_demo_annotations.py +++ b/tests/unit/test_golden_demo_annotations.py @@ -55,14 +55,23 @@ def demo_config(request: pytest.FixtureRequest) -> PyFlowConfig: def _colour_maps_a_field(config: PyFlowConfig) -> bool: """The two ways a demo puts a colour map on screen: a static - `scalar_pattern` (`_add_field_display`) or a live-transported - `render_field` (`_add_declared_field_transport`). Both draw the - legend strip `_add_legend` builds, and so both need a caption. + `scalar_pattern` (`_add_field_display`) or one or more live panels + (`field_display.panels`, `_add_declared_field_transport`). Both draw + a legend strip, and so both need a caption. """ - return ( - config.field_display.scalar_pattern is not None - or config.field_display.render_field is not None - ) + return config.field_display.scalar_pattern is not None or bool(config.field_display.panels) + + +def _panel_caption(field: str, mode: str, label: str | None) -> str: + """`bootstrap._panel_caption`'s own fallback rule, duplicated here + rather than imported -- this file's own established convention + (`_draws_arrows` above already duplicates `bootstrap.py`'s own + gating logic the same way) for a config-inspection module that + otherwise has no reason to import rendering internals. + """ + if label is not None: + return label + return field if mode == "linear" else "equalized" def _draws_arrows(config: PyFlowConfig) -> bool: @@ -94,19 +103,26 @@ def _renders_a_mesh_view(config: PyFlowConfig) -> bool: def test_every_demo_that_colour_maps_a_field_names_the_quantity( demo_config: PyFlowConfig, ) -> None: - """P-019's legend half. `_add_hud` captions the legend with - `field_label or render_field`, so a static `scalar_pattern` demo - setting neither renders a gradient strip with numbers at its ends - and no statement of what is being measured. + """P-019's legend half. A static `scalar_pattern` demo setting no + `field_display.field_label` renders a gradient strip with numbers at + its ends and no statement of what is being measured -- the one case + this still has real teeth for. Every live panel is guaranteed a + non-empty caption structurally (`_panel_caption`'s own field-name/ + "equalized" fallback), so that half is checked for completeness + (protects against a future fallback regression) rather than because + any demo could fail it today. """ if not _colour_maps_a_field(demo_config) or not demo_config.field_display.show_legend: pytest.skip("draws no legend") - caption = demo_config.field_display.field_label or demo_config.field_display.render_field - assert caption, ( - "a demo that colour-maps a field must name the quantity " - "(field_display.field_label, or render_field as the fallback) -- P-019" - ) + if demo_config.field_display.scalar_pattern is not None: + assert demo_config.field_display.field_label, ( + "a demo that colour-maps a static scalar_pattern must name the quantity via " + "field_display.field_label -- P-019" + ) + for panel in demo_config.field_display.panels: + caption = _panel_caption(panel.field, panel.mode, panel.label) + assert caption, f"panel {panel!r} must resolve to a non-empty legend caption -- P-019" def test_every_demo_that_draws_arrows_states_what_they_are(demo_config: PyFlowConfig) -> None: @@ -181,10 +197,19 @@ def test_every_legend_caption_fits_on_one_line(demo_config: PyFlowConfig) -> Non """ if not _colour_maps_a_field(demo_config) or not demo_config.field_display.show_legend: pytest.skip("draws no legend") - caption = demo_config.field_display.field_label or demo_config.field_display.render_field - if caption is None or not caption: + + captions: list[str] = [] + if ( + demo_config.field_display.scalar_pattern is not None + and demo_config.field_display.field_label + ): + captions.append(demo_config.field_display.field_label) + captions.extend( + _panel_caption(panel.field, panel.mode, panel.label) + for panel in demo_config.field_display.panels + ) + if not captions: pytest.skip("no caption to measure") - assert isinstance(caption, str) width, height = demo_config.mesh.spacing extent_x, extent_y = demo_config.mesh.extent @@ -193,11 +218,12 @@ def test_every_legend_caption_fits_on_one_line(demo_config: PyFlowConfig) -> Non font_size = mesh_height * 0.05 characters_per_line = mesh_width / (font_size * 0.5) - assert len(caption) <= characters_per_line, ( - f"the caption {caption!r} is {len(caption)} characters against roughly " - f"{characters_per_line:.0f} that fit on one line at this mesh's own HUD font " - "size, so it would wrap and its second line would be drawn over the mesh" - ) + for caption in captions: + assert len(caption) <= characters_per_line, ( + f"the caption {caption!r} is {len(caption)} characters against roughly " + f"{characters_per_line:.0f} that fit on one line at this mesh's own HUD font " + "size, so it would wrap and its second line would be drawn over the mesh" + ) def test_the_sweep_actually_covers_the_demos() -> None: diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index fb94fe5..c4f7eb5 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -379,9 +379,9 @@ def test_generate_config_with_no_output_prints_to_stdout( "arrow_color": "#ffffff", "arrow_scale": 0.3, "show_legend": True, - "render_field": None, "field_label": None, "vector_label": None, + "panels": [], }, "fields": [], "simulation": { diff --git a/tests/unit/test_temperature_field.py b/tests/unit/test_temperature_field.py index d1c8ba4..ac5fc55 100644 --- a/tests/unit/test_temperature_field.py +++ b/tests/unit/test_temperature_field.py @@ -265,7 +265,7 @@ def _given_periodic_temperature(tmp_path: Path) -> _Context: " - name: temperature\n" " initial_condition: sinusoidal_mode\n" " diffusion_coefficient: 0.05\n" - "field_display:\n render_field: temperature\n" + "field_display:\n panels:\n - field: temperature\n" ) return _Context(config_path=config_path) diff --git a/tools/generators/generate_config_template.py b/tools/generators/generate_config_template.py index add73e0..eac4186 100644 --- a/tools/generators/generate_config_template.py +++ b/tools/generators/generate_config_template.py @@ -195,15 +195,11 @@ ), "field_display.arrow_scale": "Valid: a positive number. Invalid: zero or negative.", "field_display.show_legend": "Valid: true or false.", - "field_display.render_field": ( - "Valid: null (no live field is coloured) or the name of one field " - "declared under fields: below -- the renderer never infers which " - "one to show. Invalid: naming a field fields: does not declare." - ), "field_display.field_label": ( - "Valid: null (fall back to render_field's own name) or any " - 'string -- a human-readable legend caption, e.g. "Temperature ' - '(K)". Invalid: a non-string value.' + "Valid: null (no caption at all) or any string -- a human-readable " + "legend caption for the static scalar_pattern display only, e.g. " + '"Distance from centre". A live panel (field_display.panels below) ' + "has its own, separate label instead. Invalid: a non-string value." ), "field_display.vector_label": ( "Valid: null (no vector-scale HUD line at all) or any string -- " @@ -211,6 +207,21 @@ "the HUD states this label alongside arrow_scale wherever arrows " "are actually drawn. Invalid: a non-string value." ), + "field_display.panels": ( + "Valid: a list of live colour-mapped panel declarations, each a " + "mapping with field (a non-empty string naming one field declared " + "under fields: below -- the renderer never infers which one to " + "show), mode (linear or equalized -- linear maps value_range " + "onto low_color/high_color; equalized colours by each cell's rank " + "among the field's current values instead, so peaks and valleys " + "stay distinguishable even when both are numerically tiny, no " + "range needed), value_range (a [min, max] pair, linear mode only), " + "and label (null falls back to field's own name for a linear " + 'panel, or the constant "equalized" for an equalized one). ' + "Drawn left to right in list order. [] (the default) draws " + "nothing. Invalid: naming a field fields: does not declare, an " + "unrecognised mode, or a degenerate value_range (max <= min)." + ), "fields": ( "Valid: a list of per-field declarations, each a mapping with " "name (a non-empty string, not reused by another declaration and " From 67e169f1190e26991b6d3dff9806e429093a47fb Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Tue, 8 Sep 2026 07:30:12 +0100 Subject: [PATCH 2/2] Let pyflow resume start a new recording from a config file Adds --config/config_path as a mutually exclusive alternative to --checkpoint/checkpoint_path on `pyflow resume`, at a user's direct request ("do pyflow resume from a config file and have it start from the first frame"). Given a config instead of a checkpoint, resume is a pure delegation to record (there's nothing yet to resume from, so it starts at frame 0) -- letting a caller use resume as the one command for a recording's whole lifecycle instead of branching on whether a checkpoint exists yet. This doesn't reopen the original "no --config at all" design: that reasoning was specifically about combining a checkpoint and a config in one call, which the new mutually exclusive, required argparse group still makes impossible. Co-Authored-By: Claude Sonnet 5 --- docs/planning/roadmap.md | 31 +++++++++++--- docs/planning/status.md | 2 +- src/pyflow/CLAUDE.md | 34 ++++++++++++---- src/pyflow/__main__.py | 58 ++++++++++++++++++++------- src/pyflow/recording.py | 39 +++++++++++++++--- tests/integration/test_record_cli.py | 60 +++++++++++++++++++++++++++- tests/unit/test_main.py | 40 ++++++++++++++++--- tests/unit/test_recording.py | 56 +++++++++++++++++++++++--- 8 files changed, 272 insertions(+), 48 deletions(-) diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index f450da1..a17d816 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -306,11 +306,32 @@ 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): **1154 tests as of 2026-09-07**, up from 1153 slightly -earlier the same day (below), then 1143, 1137, 1131, and 1052 the day -before that. - -**The 1 most recent is a real-bug regression test, found by a user +(C1a/C1b): **1160 tests as of 2026-09-07**, up from 1154 slightly +earlier the same day (below), then 1153, 1143, 1137, 1131, and 1052 the +day before that. + +**The 6 most recent are `pyflow resume`'s own new `--config`/`config_path` +alternative** -- a further same-day user request ("do pyflow resume +from a config file and have it start from the first frame"): a second, +mutually exclusive way to call `resume` (alongside its existing +`--checkpoint`/`checkpoint_path`) that starts a brand new recording at +frame 0, a pure delegation to `record` rather than a second copy of its +logic, so a caller can use `resume` as the one command name for a +recording's whole lifecycle. 3 in `tests/unit/test_recording.py` +(behaves exactly like `record`, checked against a real `record()` call +rather than merely not raising; rejects neither `checkpoint_path` nor +`config_path` given; rejects both given), 1 in `tests/unit/test_main.py` +(dispatches `--config` to `config_path`; the old "resume has no --config +flag at all" test is retired, replaced by a rejection test for +`--checkpoint`+`--config` together, and the existing "requires +checkpoint" test renamed to "requires checkpoint or config" -- both +already covered the same argparse mutually-exclusive-group error +message, unaffected in substance by the rename), 2 in `tests/integration/ +test_record_cli.py` (a real subprocess `pyflow resume --config` run, +and the same `--checkpoint`+`--config` rejection through the real CLI +rather than only in-process); 3 + 1 + 2 = 6. + +**The 1 before those is a real-bug regression test, found by a user report rather than by any check in this repository -- the panel-list migration just below widened `overall_bounds` rightward for extra panels but never downward for a panel's own legend and caption, so diff --git a/docs/planning/status.md b/docs/planning/status.md index 3058633..6ece6c6 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -46,7 +46,7 @@ pie showData ## Live repository facts - **47** `CLAUDE.md` files -- **1154** tests collected +- **1160** tests collected - **144** Gherkin scenarios (`tests/features/*.feature`) ## Stages diff --git a/src/pyflow/CLAUDE.md b/src/pyflow/CLAUDE.md index 4f052a8..0371372 100644 --- a/src/pyflow/CLAUDE.md +++ b/src/pyflow/CLAUDE.md @@ -176,14 +176,32 @@ checkpoints at the same policy `record` uses -- shared with it through a new `_advance_and_checkpoint` helper rather than a second copy of the "every `interval` frames, and at `max_frames`" logic, confirmed to genuinely share behaviour (not just source) by a deliberate off-by-one -mutation that broke both functions' own tests together. Takes no -`--config` at all: the checkpoint already carries one, validated exactly -as strictly as a config file (`checkpoint.py`'s own docstring). It is -not Stage 8's own second or third bullet (deterministic windowed replay, -now TASK-046; a playback path, now TASK-047, both built the same day) -- -neither renders anything or materializes dense per-frame data for a -watched range; `resume` only ever produces more of the identical sparse -checkpoint files `record` already produces, starting from a later frame. +mutation that broke both functions' own tests together. Given a +checkpoint, takes no `--config` at all: the checkpoint already carries +one, validated exactly as strictly as a config file (`checkpoint.py`'s +own docstring). It is not Stage 8's own second or third bullet +(deterministic windowed replay, now TASK-046; a playback path, now +TASK-047, both built the same day) -- neither renders anything or +materializes dense per-frame data for a watched range; `resume` only +ever produces more of the identical sparse checkpoint files `record` +already produces, starting from a later frame. + +**`resume`'s own `config_path` parameter, added 2026-09-07 at a further +user request** ("do pyflow resume from a config file and have it start +from the first frame") **-- a second, mutually exclusive way to call +`resume`, not a widening of the checkpoint-only path above.** Pass +exactly one of `checkpoint_path`/`config_path`; given `config_path`, +`resume` is a pure delegation to `record` (`return record(config_path, +...)`), not a second copy of its logic, since there is nothing yet to +resume *from*. Lets a caller use `resume` as the single command name for +a recording's whole lifecycle (`--config` the first time, `--checkpoint +` every time after) rather than having to branch on whether a +checkpoint exists yet. Does not reopen the "no `--config` at all" design +the paragraph above records: that reasoning was specifically about a +checkpoint and a config being combined in one call, which `__main__.py`'s +own mutually exclusive, required `argparse` group (`--checkpoint`/ +`--config`) still makes structurally impossible -- this adds an +alternate entry point, not a way to pass both at once. **`replay.py` (TASK-046) is the windowed-materialization library those two tasks needed** -- `MaterializedWindow`, `materialize_window`, diff --git a/src/pyflow/__main__.py b/src/pyflow/__main__.py index abfba3b..4ae3bfc 100644 --- a/src/pyflow/__main__.py +++ b/src/pyflow/__main__.py @@ -45,19 +45,33 @@ continues a headless recording from an existing checkpoint rather than from frame 0 -- still no rendering window, still writing further checkpoint files, not the dense per-frame replay TASK-046/047 still -owns. **Deliberately no `--config` flag at all** -- a checkpoint carries -its own, validated exactly as strictly as a config file -(`pyflow.checkpoint.read_checkpoint`), so naming one here would only -invite a mismatch between "the config this run resumes under" and -"the config a user happened to pass." `--checkpoint`/`--max-frames` are -`required=True`, the same reasoning `record`'s own required flags use; -`--max-frames` must additionally be past the checkpoint's own frame -count (`pyflow.recording.NothingToResumeError` otherwise). Dispatches to +owns. `--checkpoint`/`--max-frames` are each `required=True` within +their own mutually exclusive group (below); `--max-frames` must +additionally be past the checkpoint's own frame count +(`pyflow.recording.NothingToResumeError` otherwise). Dispatches to `pyflow.recording.resume`, which shares its checkpoint-writing policy with `record` (`recording.py`'s own `_advance_and_checkpoint`) so a `record` to frame 6 followed by a `resume` to frame 12 writes the same files an uninterrupted `record` to frame 12 would have. +**`--config `, a mutually exclusive alternative to `--checkpoint` +(added at a user's direct request: "do pyflow resume from a config file +and have it start from the first frame"), starts a brand new recording +at frame 0 -- exactly `pyflow record`'s own behaviour, reached through +`resume`'s own name instead.** This deliberately does not undo the +original "no `--config` flag at all" design -- that reasoning was +specifically about the risk of a checkpoint and a config being combined +in one call ("a mismatch between 'the config this run resumes under' +and 'the config a user happened to pass'"), which `argparse`'s own +mutually exclusive group here still makes structurally impossible: this +adds a second, alternate way to invoke `resume`, not a way to pass both +at once. The point is ergonomic -- a script that always calls `pyflow +resume` (with `--config` the first time there is no checkpoint yet, then +`--checkpoint ` every time after) never has to branch on which +of two command names applies. `pyflow.recording.resume`'s own +`config_path` parameter is a pure delegation to `record` in this case, +not a second copy of its logic. + `pyflow play --checkpoints-dir --to-frame N [--from-frame N] [--cache DIR] [--backend BACKEND] [--max-frames N]` (TASK-046/047, Stage 8, Recording & Playback -- deterministic windowed replay and @@ -165,8 +179,9 @@ def main(argv: list[str] | None = None) -> None: " pyflow resume --checkpoint checkpoints/checkpoint_00000100.pt " "--max-frames 500\n" " Continue a headless recording from an existing " - "checkpoint -- no --config,\n" - " the checkpoint carries its own.\n" + "checkpoint -- or pass\n" + " --config instead of --checkpoint to start a new one " + "at frame 0.\n" " pyflow play --checkpoints-dir checkpoints --to-frame 500\n" " Watch a checkpointed run in a real window -- Space to " "pause/resume,\n" @@ -280,20 +295,32 @@ def main(argv: list[str] | None = None) -> None: "resume", help="Read a checkpoint written by `record` (or a previous " "`resume`), and continue stepping headlessly from its own frame, " - "writing further checkpoints. No --config -- the checkpoint " - "carries its own.", + "writing further checkpoints -- or, given --config instead, start " + "a brand new recording at frame 0.", epilog=( "examples:\n" " pyflow resume --checkpoint checkpoints/checkpoint_00000100.pt " "--max-frames 500\n" + " pyflow resume --config examples/golden-demos/heat_diffusion.yaml " + "--max-frames 500\n" ), formatter_class=argparse.RawDescriptionHelpFormatter, ) - resume_parser.add_argument( + checkpoint_or_config = resume_parser.add_mutually_exclusive_group(required=True) + checkpoint_or_config.add_argument( "--checkpoint", type=Path, - required=True, - help="Path to a checkpoint file written by `pyflow record` or `pyflow resume`.", + default=None, + help="Path to a checkpoint file written by `pyflow record` or `pyflow resume`. " + "Continues stepping from its own frame.", + ) + checkpoint_or_config.add_argument( + "--config", + type=Path, + default=None, + help="Path to a YAML configuration file, instead of --checkpoint -- starts a " + "new recording at frame 0, exactly like `pyflow record`. Useful for a script " + "that always calls `pyflow resume` regardless of whether a checkpoint exists yet.", ) resume_parser.add_argument( "--max-frames", @@ -415,6 +442,7 @@ def main(argv: list[str] | None = None) -> None: if args.command == "resume": result = resume( args.checkpoint, + config_path=args.config, max_frames=args.max_frames, output_dir=args.output_dir, checkpoint_interval=args.checkpoint_interval, diff --git a/src/pyflow/recording.py b/src/pyflow/recording.py index 749465d..9359eb9 100644 --- a/src/pyflow/recording.py +++ b/src/pyflow/recording.py @@ -187,8 +187,9 @@ def record( def resume( - checkpoint_path: str | Path, + checkpoint_path: str | Path | None = None, *, + config_path: str | Path | None = None, max_frames: int, output_dir: str | Path | None = None, checkpoint_interval: int | None = None, @@ -202,11 +203,6 @@ def resume( uninterrupted `record(..., max_frames=12)` would have written after frame 6. - No `--config`/`config_path` parameter at all -- a checkpoint is - self-contained (`checkpoint.py`'s own docstring) and carries its own - validated config, read back through the identical - `checkpoint.read_checkpoint` a resumed run's config is checked with. - `output_dir`, given, overrides where further checkpoints are written; omitted, defaults to `checkpoint_path`'s own parent directory -- not the checkpoint's embedded `config.recording.output_dir`, which is the @@ -219,7 +215,38 @@ def resume( Raises `NothingToResumeError` if `max_frames` is not strictly greater than the checkpoint's own `frame_count`. + + **`config_path` (added at a user's direct request: "do pyflow resume + from a config file and have it start from the first frame") is a + second, mutually exclusive way to call this function -- pass exactly + one of `checkpoint_path`/`config_path`, never both, never neither.** + Given a config instead of a checkpoint, there is nothing yet to + resume *from*, so this is a pure alternate entry point into `record` + itself (`return record(config_path, ...)`, not a second copy of its + logic) -- what lets a caller use `resume` as the one command for an + entire recording's lifecycle (`pyflow resume --config X` the first + time, `pyflow resume --checkpoint ` every time after) without + having to remember which of two command names applies yet. This is + a different case from the "mismatch between the config a checkpoint + carries and a config a user might pass" concern the original + `--checkpoint`-only design recorded, above and in `src/pyflow/ + CLAUDE.md`: that risk is specifically about combining both at once, + which is exactly the combination this still rejects. """ + if (checkpoint_path is None) == (config_path is None): + raise ValueError( + "resume: exactly one of checkpoint_path or config_path must be given, got " + f"checkpoint_path={checkpoint_path!r}, config_path={config_path!r}" + ) + if config_path is not None: + return record( + config_path, + max_frames=max_frames, + output_dir=output_dir, + checkpoint_interval=checkpoint_interval, + ) + + assert checkpoint_path is not None # the exactly-one-of check above guarantees this checkpoint = read_checkpoint(checkpoint_path) if max_frames <= checkpoint.frame_count: raise NothingToResumeError( diff --git a/tests/integration/test_record_cli.py b/tests/integration/test_record_cli.py index dbb42f4..6a59c66 100644 --- a/tests/integration/test_record_cli.py +++ b/tests/integration/test_record_cli.py @@ -116,7 +116,65 @@ def test_resume_continues_a_real_recording_with_no_config_flag(tmp_path: Path) - assert set(payload["fields"]) == {"tracer"} -def test_resume_requires_checkpoint_and_max_frames() -> None: +def test_resume_starts_a_new_recording_from_a_config(tmp_path: Path) -> None: + """`--config`, added at a user's direct request ("do pyflow resume + from a config file and have it start from the first frame"): a + mutually exclusive alternative to `--checkpoint` that starts a brand + new recording at frame 0, exactly `pyflow record`'s own behaviour + reached through `resume`'s own name. + """ + output_dir = tmp_path / "checkpoints" + + result = subprocess.run( + [ + sys.executable, + "-m", + "pyflow", + "resume", + "--config", + "examples/golden-demos/heat_diffusion.yaml", + "--max-frames", + "5", + "--output-dir", + str(output_dir), + "--checkpoint-interval", + "5", + ], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0, result.stderr + assert "2" in result.stdout # frames 0 and 5 + assert (output_dir / "checkpoint_00000000.pt").is_file() + assert (output_dir / "checkpoint_00000005.pt").is_file() + + +def test_resume_rejects_checkpoint_and_config_together() -> None: + result = subprocess.run( + [ + sys.executable, + "-m", + "pyflow", + "resume", + "--checkpoint", + "checkpoints/checkpoint_00000005.pt", + "--config", + "examples/golden-demos/heat_diffusion.yaml", + "--max-frames", + "10", + ], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode != 0 + assert "not allowed with argument" in result.stderr + + +def test_resume_requires_checkpoint_or_config_and_max_frames() -> None: result = subprocess.run( [sys.executable, "-m", "pyflow", "resume"], capture_output=True, diff --git a/tests/unit/test_main.py b/tests/unit/test_main.py index c4f7eb5..e265272 100644 --- a/tests/unit/test_main.py +++ b/tests/unit/test_main.py @@ -213,6 +213,7 @@ def test_resume_dispatches_to_resume_with_parsed_args() -> None: mock_resume.assert_called_once_with( Path("checkpoints/checkpoint_00000006.pt"), + config_path=None, max_frames=12, output_dir=Path("out"), checkpoint_interval=3, @@ -226,22 +227,49 @@ def test_resume_output_dir_and_checkpoint_interval_default_to_none() -> None: mock_resume.assert_called_once_with( Path("checkpoints/checkpoint_00000006.pt"), + config_path=None, max_frames=9, output_dir=None, checkpoint_interval=None, ) -def test_resume_has_no_config_flag_at_all() -> None: - """`pyflow resume` never takes `--config` -- a checkpoint carries its - own (`recording.resume`'s own docstring); this pins the CLI surface - itself rather than only the underlying function's signature. +def test_resume_dispatches_with_config_instead_of_checkpoint() -> None: + """`--config`, added at a user's direct request for `pyflow resume` + to be able to start a brand new recording at frame 0 -- a mutually + exclusive alternative to `--checkpoint`, not a way to pass both. """ + with patch("pyflow.__main__.resume") as mock_resume: + mock_resume.return_value = SimpleNamespace(checkpoint_frames=[0, 9], output_dir=Path("out")) + main(["resume", "--config", "some-config.yaml", "--max-frames", "9", "--output-dir", "out"]) + + mock_resume.assert_called_once_with( + None, + config_path=Path("some-config.yaml"), + max_frames=9, + output_dir=Path("out"), + checkpoint_interval=None, + ) + + +def test_resume_rejects_checkpoint_and_config_together(capsys: pytest.CaptureFixture[str]) -> None: with pytest.raises(SystemExit): - main(["resume", "--config", "some-config.yaml", "--max-frames", "9"]) + main( + [ + "resume", + "--checkpoint", + "checkpoints/checkpoint_00000006.pt", + "--config", + "some-config.yaml", + "--max-frames", + "9", + ] + ) + + assert "not allowed with argument" in capsys.readouterr().err -def test_resume_requires_checkpoint(capsys: pytest.CaptureFixture[str]) -> None: +def test_resume_requires_checkpoint_or_config(capsys: pytest.CaptureFixture[str]) -> None: with pytest.raises(SystemExit): main(["resume", "--max-frames", "9"]) diff --git a/tests/unit/test_recording.py b/tests/unit/test_recording.py index 6895bcf..2695aff 100644 --- a/tests/unit/test_recording.py +++ b/tests/unit/test_recording.py @@ -222,12 +222,13 @@ def test_resume_rejects_max_frames_not_past_the_checkpoint(tmp_path: Path) -> No resume(output_dir / "checkpoint_00000006.pt", max_frames=3) -def test_resume_needs_no_config_path_at_all(tmp_path: Path) -> None: - """The property `pyflow resume`'s own CLI leans on for having no - `--config` flag: a checkpoint is self-contained - (`checkpoint.py`'s own docstring), so `resume` never takes one -- - checked here by calling it with only a checkpoint path and confirming - it works, not merely by the function signature lacking the parameter. +def test_resume_needs_no_config_path_at_all_when_resuming_from_a_checkpoint( + tmp_path: Path, +) -> None: + """A checkpoint is self-contained (`checkpoint.py`'s own docstring), + so resuming from one never needs a `config_path` -- checked here by + calling `resume` with only a checkpoint path and confirming it works, + not merely by `config_path` being optional in the signature. """ config_file = tmp_path / "config.yaml" config_file.write_text(_DECLARED_FIELD_CONFIG) @@ -238,3 +239,46 @@ def test_resume_needs_no_config_path_at_all(tmp_path: Path) -> None: result = resume(output_dir / "checkpoint_00000003.pt", max_frames=6, checkpoint_interval=3) assert result.checkpoint_frames == [6] + + +def test_resume_from_a_config_path_behaves_exactly_like_record(tmp_path: Path) -> None: + """`resume(config_path=...)` (added at a user's direct request: "do + pyflow resume from a config file and have it start from the first + frame") is a pure alternate entry point into the same recording -- + given a config instead of a checkpoint, there is nothing yet to + resume *from*, so it starts at frame 0 exactly like `record` does. + Checked by comparing against a real `record()` call on the same + config, not merely asserting `resume` runs without raising -- the two + must produce byte-identical output, not just superficially similar + output. + """ + config_file = tmp_path / "config.yaml" + config_file.write_text(_DECLARED_FIELD_CONFIG) + recorded_dir = tmp_path / "recorded" + record(config_file, max_frames=6, output_dir=recorded_dir, checkpoint_interval=3) + + resumed_dir = tmp_path / "resumed" + result = resume( + config_path=config_file, max_frames=6, output_dir=resumed_dir, checkpoint_interval=3 + ) + + assert result.checkpoint_frames == [0, 3, 6] + assert result.output_dir == resumed_dir + recorded = read_checkpoint(recorded_dir / "checkpoint_00000006.pt") + resumed = read_checkpoint(resumed_dir / "checkpoint_00000006.pt") + torch.testing.assert_close(resumed.fields["smoke"], recorded.fields["smoke"], rtol=0, atol=0) + + +def test_resume_rejects_neither_checkpoint_path_nor_config_path(tmp_path: Path) -> None: + with pytest.raises(ValueError, match="checkpoint_path.*config_path"): + resume(max_frames=6) + + +def test_resume_rejects_both_checkpoint_path_and_config_path(tmp_path: Path) -> None: + config_file = tmp_path / "config.yaml" + config_file.write_text(_DECLARED_FIELD_CONFIG) + output_dir = tmp_path / "checkpoints" + record(config_file, max_frames=3, output_dir=output_dir, checkpoint_interval=3) + + with pytest.raises(ValueError, match="checkpoint_path.*config_path"): + resume(output_dir / "checkpoint_00000003.pt", config_path=config_file, max_frames=6)