diff --git a/API.md b/API.md index 1d00a7f52..c9230ed5c 100644 --- a/API.md +++ b/API.md @@ -119,7 +119,9 @@ the same thing it used to. `evaluate_feature_value`, `evaluate_feature_gradient`, `evaluate_fault_displacements` - Solve: `update` -- Geometry: `scale`, `rescale`, `regular_grid`, `bounding_box` (property) +- Geometry: `scale` (deprecated, use `bounding_box.project`), `rescale` + (deprecated, use `bounding_box.reproject`), `regular_grid`, + `bounding_box` (property) - Stratigraphy: `stratigraphic_column` (property), `stratigraphic_ids` - Output: `get_fault_surfaces`, `get_stratigraphic_surfaces`, `get_block_model`, `save`, `to_file`, `to_dict` diff --git a/COMPAT.md b/COMPAT.md index 5c9e56b74..ae8883212 100644 --- a/COMPAT.md +++ b/COMPAT.md @@ -14,6 +14,8 @@ any module-path move/rename on the 1.x line gets a re-export shim with a | `LoopStructural.interpolators._interpolator_builder` | `loop_interpolation._interpolator_builder` | 2026-07-29 | 2026-07-29 | extracted-package integration | 2 minor releases after first shim release | Active | Core maintainers | | `LoopStructural.interpolators._interpolator_factory` | `loop_interpolation._interpolator_factory` | 2026-07-29 | 2026-07-29 | extracted-package integration | 2 minor releases after first shim release | Active | Core maintainers | | Loading a legacy interpolator payload with no choice of the caller (the loader assumes an identity frame, now with a `FutureWarning`) | Pass `local_origin` and `local_rotation`, or `declare_identity=True`, to `from_dict`, `from_json` or `from_yaml`; at the next major release a call with no choice raises `InterpolatorFormatError` | 2026-11-05 | TBD (next release containing this warning) | spec 005 stage S3 (pull request #397) | The next major release | Active | Core maintainers | +| `GeologicalModel.rescale(points, inplace=False)` | `GeologicalModel.bounding_box.reproject(points, inplace=False)` | 2026-11-05 | TBD (next release containing this warning) | spec 005 stage S4 | TBD (2 minor releases after the first release that warns) | Active | Core maintainers | +| `GeologicalModel.scale(points, inplace=False)` | `GeologicalModel.bounding_box.project(points, inplace=False)` | 2026-11-05 | TBD (next release containing this warning) | spec 005 stage S4 | TBD (2 minor releases after the first release that warns) | Active | Core maintainers | ## Deprecation lifecycle @@ -51,11 +53,12 @@ unchanged, but the new path is preferred going forward. | `create_and_add_fault(fault_center=...)` projected the centre into local coordinates; `fault_buffer`/`force_mesh_geometry` were replaced by a default 0.2 buffer; an inferred centre had z = 0; any 3x3 matrix was accepted as a rotation | `fault_center` is a world point; the requested buffer and forced geometry are kept; an inferred centre keeps its elevation; `set_local_transform` and `rotation_xy` raise `RigidRotationError` for a non-rigid matrix. Extra observations added to work around these defects are no longer necessary (spec 005) | 2026-11-04 | | Fold `mask_fn`, custom fold providers (`get_deformed_orientation`), `directional_regularisation` direction callables and arrays, and the P2 `steepness_wtfunc` received or returned local (computational-frame) points and directions | They receive world points and give world directions; the interpolator converts to its local frame once. With an identity frame nothing changes (spec 005, stage S2) | 2026-11-04 | | `copy()` returned an interpolator with no frame; a transform change after constraints existed gave a stale result | New payloads carry `format_version: 2`, `constraint_space`, `transform` and `bounding_box`. `copy()` keeps the frame. A transform change after constraints exist raises `StaleFrameError` at the next set, evaluate or save (spec 005, stage S3). Loading a legacy payload with no choice is deprecated: see the deprecation table | 2026-11-05 | +| `GeologicalFeature.get_data()` (used by `Loop2DView.add_data` and `ModelExporter.save`) returned locations moved by the local origin and rotated a second time; the intrusion classification evaluated the model at such points | Both use the stored world locations. With an identity frame nothing changes (spec 005, stage S4) | 2026-11-05 | The following stable methods remain part of the documented public API surface after the package extraction work and are covered by the snapshot-based contract test: add_onlap_unconformity, add_unconformity, create_and_add_domain_fault, create_and_add_fault, create_and_add_fold_frame, create_and_add_folded_fold_frame, create_and_add_folded_foliation, create_and_add_foliation, evaluate_model, evaluate_model_gradient, from_file, get_fault_surfaces, get_feature_by_name, get_stratigraphic_surfaces, rescale, save, scale. ## Compatibility debt summary -- Active shims: 8 +- Active shims: 10 - Oldest active shim added: 2026-07-24 - Next cleanup milestone: remove the interpolator-path shims after 2 minor releases from 2026-07-29 diff --git a/LoopStructural/export/exporters.py b/LoopStructural/export/exporters.py index ae767f6e3..b54a8f32b 100644 --- a/LoopStructural/export/exporters.py +++ b/LoopStructural/export/exporters.py @@ -89,7 +89,6 @@ def write_feat_surfs( model.bounding_box.bb[1, 2], ] ) - model.rescale(verts) surf = Surface( vertices=verts, triangles=faces, @@ -362,8 +361,6 @@ def _write_cubeface_evtk(model, file_name, data_label, nsteps, real_coords=True) # Evaluate model at points points, tri = create_box(model.bounding_box, nsteps) val = model.evaluate_model(points, scale=False) - if real_coords: - model.rescale(points) # Define vertices x = np.zeros(points.shape[0]) @@ -427,8 +424,6 @@ def _write_vol_evtk(model, file_name, data_label, nsteps, real_coords=True): # project world -> local internally) xyz = model.bounding_box.regular_grid(nsteps=nsteps, local=False) vals = model.evaluate_model(xyz, scale=False) - if real_coords: - model.rescale(xyz) # Define vertices - xyz.shape[0] is length of vector array x = np.zeros(xyz.shape[0]) @@ -475,10 +470,6 @@ def _write_vol_gocad(model, file_name, data_label, nsteps, real_coords=True): vol_vals = np.reshape(vals, nsteps, order="F") bbox = model.bounding_box[:] - # Convert bounding box to real world scale coords - if real_coords: - model.rescale(bbox) - # If integer values if type(vals[0]) is np.int64: d_type = np.int8 diff --git a/LoopStructural/modelling/core/_model_exporter.py b/LoopStructural/modelling/core/_model_exporter.py index 1fbadb351..2796d88c7 100644 --- a/LoopStructural/modelling/core/_model_exporter.py +++ b/LoopStructural/modelling/core/_model_exporter.py @@ -74,7 +74,7 @@ def get_block_model( grid = model.bounding_box.structured_grid(name=name) grid.cell_properties['stratigraphy'] = model.evaluate_model( - model.rescale(model.bounding_box.cell_centres()) + model.bounding_box.reproject(model.bounding_box.cell_centres()) ) return grid, model.stratigraphic_ids() diff --git a/LoopStructural/modelling/core/geological_model.py b/LoopStructural/modelling/core/geological_model.py index d8f2ece4e..cec10d228 100644 --- a/LoopStructural/modelling/core/geological_model.py +++ b/LoopStructural/modelling/core/geological_model.py @@ -1107,6 +1107,10 @@ def rescale(self, points: np.ndarray, *, inplace: bool = False) -> np.ndarray: Convert from model scale to real world scale - in the future this should also do transformations? + .. deprecated:: + Use ``model.bounding_box.reproject`` instead. + Deprecated by spec 005, stage S4. + Parameters ---------- points : np.array((N,3),dtype=double) @@ -1118,7 +1122,12 @@ def rescale(self, points: np.ndarray, *, inplace: bool = False) -> np.ndarray: points : np.array((N,3),dtype=double) """ - + # stacklevel 3 skips the public_api wrapper frame + warnings.warn( + "GeologicalModel.rescale is deprecated and will be removed in a future release; use model.bounding_box.reproject(points) instead", + DeprecationWarning, + stacklevel=3, + ) return self.bounding_box.reproject(points, inplace=inplace) # TODO move scale to bounding box/transformer @@ -1127,6 +1136,10 @@ def scale(self, points: np.ndarray, *, inplace: bool = False) -> np.ndarray: """Take points in UTM coordinates and reproject into scaled model space + .. deprecated:: + Use ``model.bounding_box.project`` instead. + Deprecated by spec 005, stage S4. + Parameters ---------- points : np.array((N,3),dtype=float) @@ -1138,6 +1151,12 @@ def scale(self, points: np.ndarray, *, inplace: bool = False) -> np.ndarray: points : np.a::rray((N,3),dtype=double) """ + # stacklevel 3 skips the public_api wrapper frame + warnings.warn( + "GeologicalModel.scale is deprecated and will be removed in a future release; use model.bounding_box.project(points) instead", + DeprecationWarning, + stacklevel=3, + ) return self.bounding_box.project(np.array(points).astype(float), inplace=inplace) @public_api(tier="stable") diff --git a/LoopStructural/modelling/features/_geological_feature.py b/LoopStructural/modelling/features/_geological_feature.py index 41c540bd9..22a17bf0f 100644 --- a/LoopStructural/modelling/features/_geological_feature.py +++ b/LoopStructural/modelling/features/_geological_feature.py @@ -303,6 +303,8 @@ def copy(self, name=None): def get_data(self, value_map: dict | None = None) -> list[ValuePoints | VectorPoints]: """Return the data associated with this geological feature + The locations and the vectors are in world coordinates, as the builder stores them. + Parameters ---------- value_map : Optional[dict], optional @@ -326,7 +328,7 @@ def get_data(self, value_map: dict | None = None) -> list[ValuePoints | VectorPo data.append( VectorPoints( - locations=self.model.rescale(gradient_constraints[:, :3]), + locations=gradient_constraints[:, :3], vectors=gradient_constraints[:, 3:6], name=f'{self.name}+_gradient', ) @@ -334,7 +336,7 @@ def get_data(self, value_map: dict | None = None) -> list[ValuePoints | VectorPo if norm_constraints.shape[0] > 0: data.append( VectorPoints( - locations=self.model.rescale(norm_constraints[:, :3]), + locations=norm_constraints[:, :3], vectors=norm_constraints[:, 3:6], name=f'{self.name}_norm', ) @@ -344,9 +346,7 @@ def get_data(self, value_map: dict | None = None) -> list[ValuePoints | VectorPo for name, v in value_map.items(): data.append( ValuePoints( - locations=self.model.rescale( - value_constraints[value_constraints == v, :3] - ), + locations=value_constraints[value_constraints == v, :3], values=value_constraints[value_constraints == v, 3], name=f"{name}_value", ) @@ -354,7 +354,7 @@ def get_data(self, value_map: dict | None = None) -> list[ValuePoints | VectorPo else: data.append( ValuePoints( - locations=self.model.rescale(value_constraints[:, :3]), + locations=value_constraints[:, :3], values=value_constraints[:, 3], name=f"{self.name}_value", ) diff --git a/LoopStructural/modelling/intrusions/intrusion_frame_builder.py b/LoopStructural/modelling/intrusions/intrusion_frame_builder.py index 9e100d6ae..ff5356ab6 100644 --- a/LoopStructural/modelling/intrusions/intrusion_frame_builder.py +++ b/LoopStructural/modelling/intrusions/intrusion_frame_builder.py @@ -307,6 +307,13 @@ def add_faults_anisotropies(self, fault_list: list | None = None): fault_i_std, ] + def _network_model_values(self, data: pd.DataFrame) -> np.ndarray: + """ + The id of the unit at each point. The model is evaluated at the world points; no conversion is applied. + """ + xyz = data.loc[:, ["X", "Y", "Z"]].to_numpy(dtype=float) + return self.model.evaluate_model(xyz) + def set_intrusion_steps_parameters(self): """ Use intrusion contact data (reference contact for intrusion frame) to compute the parameters related to each step. @@ -330,9 +337,6 @@ def set_intrusion_steps_parameters(self): # set data intrusion_network_data = self.intrusion_network_data.copy() - intrusion_network_data_xyz = ( - intrusion_network_data.loc[:, ["X", "Y", "Z"]].copy().to_numpy() - ) std_backup = 25 for step_i, step in self.intrusion_steps.items(): @@ -349,8 +353,8 @@ def set_intrusion_steps_parameters(self): "id" ) - intrusion_network_data.loc[:, "model_values"] = self.model.evaluate_model( - self.model.rescale(intrusion_network_data_xyz, inplace=False) + intrusion_network_data.loc[:, "model_values"] = self._network_model_values( + intrusion_network_data ) # -- check if step is within the same unit. If so, find clusters of data: diff --git a/ROADMAP.md b/ROADMAP.md index 088014500..1a24ccd34 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -126,6 +126,20 @@ The policy for a transform change, after constraints exist, is "reject at use" (decided 2026-11-05). The interpolator raises `StaleFrameError` at the next `set`, `evaluate` or `save` call. The interpolator changes no state. +### Deprecation of GeologicalModel.rescale and scale (spec 005, S4) +Decision date: 2026-11-05 (spec 005, decisions C-013 and C-016). + +`rescale` (local to world) and `scale` (world to local) keep their behaviour +and their signature. Each one emits a `DeprecationWarning`. The warning names +`bounding_box.reproject` and `bounding_box.project`. + +This replaces the status-log entry "2c-3". That entry said that both methods +stay as signature-stable pass-throughs. The tier stays "stable" until removal. + +Removal is not before 2 minor releases after the first release that warns. +Removal is also not before the known callers (the QGIS plugin) have moved. +The library no longer calls either method. + ## Stage sequence - [x] **Stage 0 — Planning infra.** This file, release/versioning/compat @@ -866,3 +880,4 @@ next `set`, `evaluate` or `save` call. The interpolator changes no state. release-please/PyPI/ruff wiring. `tests/unit`: 695 passed/4 skipped, zero regressions. - **2026-11-05:** Spec 005 stage S3 implemented: saved interpolator frame, legacy-load rule, frame-keeping copies, stale-frame check. Gate result: root pytest 1978 passed, 5 skipped. +- **2026-11-05:** Spec 005 stage S4 implemented: display, export and intrusion classification use stored world locations; rescale and scale deprecated. diff --git a/examples/3_fault/plot_1_faulted_intrusion.py b/examples/3_fault/plot_1_faulted_intrusion.py index 33c66e6a7..835755672 100644 --- a/examples/3_fault/plot_1_faulted_intrusion.py +++ b/examples/3_fault/plot_1_faulted_intrusion.py @@ -68,10 +68,7 @@ viewer.plot_surface(fault, value=0) viewer.plot_vector_field(fault) viewer.add_points( - model.rescale( - model.data[model.data["feature_name"] == "strati"][["X", "Y", "Z"]].values, - inplace=False, - ), + model.data[model.data["feature_name"] == "strati"][["X", "Y", "Z"]].values, name="prefault", ) viewer.display() @@ -97,10 +94,7 @@ viewer.plot_data(strati) viewer.plot_surface(fault, value=0.0) viewer.add_points( - model.rescale( - model.data[model.data["feature_name"] == "strati"][["X", "Y", "Z"]].values, - inplace=False, - ), + model.data[model.data["feature_name"] == "strati"][["X", "Y", "Z"]].values, name="prefault", ) viewer.display() diff --git a/specs/005-coordinate-frame-correctness/plan.md b/specs/005-coordinate-frame-correctness/plan.md index 2230cd45e..41b595617 100644 --- a/specs/005-coordinate-frame-correctness/plan.md +++ b/specs/005-coordinate-frame-correctness/plan.md @@ -7,17 +7,19 @@ The constitution (.specify/memory/constitution.md) has priority. --> ## Open points -- **OP1**: This plan gives the exact design only for stages S1, S2 and S3 (S1: +- **OP1**: This plan gives the exact design only for stages S1 to S4 (S1: shared transform, reference-constraint boundary and fault frame; S2: fold, P2, directional-regularisation and constant-norm cells; S3: saved frames, copies - and transform changes). The spec-planner limit is 10 code files, and the + and transform changes; S4: display, data export, fault-centre geometry and + intrusion classification). The spec-planner limit is 10 code files, and the maintainer selected a staged plan on 2026-11-04. S2 has its exact design since 2026-11-04 ("Stage S2 design"). S3 has its exact design since - 2026-11-05 ("Stage S3 design"). Stages S4 to S6 have a stage map, a file - list from a grep survey and a requirement allocation, but no exact - signatures. Owner: the spec-planner skill, one run for each stage. Blocks: - the tasks of S4 to S6. The spec-tasks skill makes the S3 tasks next (S1 is - merged; S2 is in pull request #396). + 2026-11-05 ("Stage S3 design"), and S4 since 2026-11-05 ("Stage S4 design"). + Stages S5 and S6 have a stage map, a file list from a grep survey and a + requirement allocation, but no exact signatures. Owner: the spec-planner + skill, one run for each stage. Blocks: the tasks of S5 and S6. The spec-tasks + skill makes the S4 tasks next (S1 is merged; S2 is in pull request #396; S3 + is built in pull request #397). - **OP2**: Run time of the mandatory R2 and R5 matrices. R2 needs 4 anchors x 3 rotations x 2 centre modes x 2 engines = 48 solves (FDI with 20000 requested cells). R5 needs 3 buffers x 3 rotations x 2 centre modes x 2 @@ -53,21 +55,49 @@ The constitution (.specify/memory/constitution.md) has priority. --> Decision needed: accept this error, which names the key, as the documented rollback behaviour. Default until an answer: accept it. Blocks: the S3 rollback check wording only. -- **OP7**: The P2 value-only fit of R1 (6 x 6 x 6 cells, ridge and gradient - regularisation, `cg` with the SC-003 settings) misses the SC-002 limits in every - frame, also the unrotated control at `ANCHORS[0]`: value error `1.8e-8` (limit - `1e-8*F`, F = 7.5) and gradient error `3.9e-7` (limit `4e-9`). The cause is solver - and regularisation accuracy, not a frame defect. FDI and PLI meet SC-002. Owner: - the maintainer or the solver owner. Decision needed: improve the P2 fit set-up - (production solver code, a separate task), or record P2 fitting as outside - SC-002. Default until an answer: the S3 P2 cells use the exact nodal control. - Blocks: nothing in S3. +- **OP7** (closed 2026-11-05, spec C-017, see "Answers"): The P2 value-only fit of R1 + (6 x 6 x 6 cells, ridge and gradient regularisation, `cg` with the SC-003 + settings) misses the SC-002 limits in every frame, also the unrotated control at + `ANCHORS[0]`. The value error is `7.8e-8` there (limit `1e-8*F`, F = 7.5, so `7.5e-8`) and `2e-7` + to `7.6e-7` at 30 degrees. The gradient error is `3.9e-7` + against the limit `1e-8` (the rule is `1e-8*max(1, |gradient|)`). The figure + `4e-9`, which earlier text gave for the gradient limit, was wrong. Also the + value error is `7.8e-8` against the limit `7.5e-8` in the unrotated control, so + it fails by 4 percent (an earlier figure of `1.8e-8` was a ratio to the scale of + the query values). At 30 degrees it is `2e-7` to `7.6e-7`. FDI and PLI stay + below `3.2e-9` (value) and `1.7e-9` (gradient). The cause is + solver and regularisation accuracy, not a frame defect. FDI and PLI meet SC-002. + Decision: SC-002 for P2 applies to the exact nodal field; the fit accuracy goes + to a separate issue. - **OP6**: The feature `copy()` methods under `LoopStructural/modelling/features` were not read. If one of them calls `interpolator.copy()`, the frame now stays on the result. Owner: the S3 copy test task (it runs `git grep -n 'interpolator.*copy('` first). Default until an answer: no change in S3. Blocks: nothing. +- **OP8** (closed for the calls on 2026-11-05, see "Answers"): The four calls + `model.rescale(...)` in `exporters.py` (lines 92, 366, 431 and 480) discard their + result, so they change nothing. S4 removes them, because the deprecated method + would warn on every export. The argument `real_coords=False` of the exporters is + ignored; it cannot give model coordinates. Owner: the maintainer. Decision + needed: document or deprecate `real_coords`. Default until an answer: leave it. + Blocks: nothing. +- **OP11** (closed 2026-11-05, spec C-016, see "Answers"): `GeologicalModel.scale` is + the inverse of `rescale` (world to local) and has the same "TODO move to the + bounding box" note. Decision: S4 deprecates `scale` together with `rescale`. +- **OP9**: `LoopStructural/utils/regions.py` was not read (10-file limit). The + abutting regions receive the world fault centre as `point`. The S1 contract + says that region callbacks take world points. Owner: the S4 test task for the + fault centre (it reads `regions.py` first). Default until an answer: the + regions treat `point` and `vector` as world values. If they convert, the task + reports it. Blocks: nothing. +- **OP10**: In `GeologicalFeature.get_data`, the `value_map` branch selects rows + with `value_constraints[value_constraints == v, :3]`. This compares the whole + array, not the value column. It is a pre-existing defect, not a coordinate + defect. Owner: the maintainer. Decision needed: file an issue, as for the + C-005 and C-006 defects. Default until an answer: S4 keeps the expression and + tests only the branch without `value_map`. Blocks: nothing. + ## Answers - 2026-11-04 (wave 3, user decision): the T006 resize repair changes the @@ -102,6 +132,33 @@ The constitution (.specify/memory/constitution.md) has priority. --> nodal control (`exact=True`). The spec allows it ("Exact evaluation control"). No tolerance changes. The P2 fit accuracy is open point OP7. `fdi`, `pli`, `cn_fdi` and `cn_pli` keep the fit (`exact=False`). +- 2026-11-05 (C-018, maintainer decision, later the same day): the maintainer will + release a major version but not yet. The legacy-load change of FR-019 stays + backward compatible on the 1.x line. A legacy payload with no choice loads with + the identity frame and emits one `FutureWarning`. The code carries a note to + remove the fallback at the next major release. This replaces the S3 answer + "no warning-only identity fallback" for 1.x and makes the C-015 entry below + moot for the loader (no break ships in 1.x). See "S3 amendment". +- 2026-11-05 (spec gap interview, C-015, maintainer decision): the legacy-load + break (FR-019) ships in a minor release, with a note. It does not ship as a major + release. `ROADMAP.md` records an explicit exception to the rule that 1.x stays + backward compatible, and `COMPAT.md` and the release notes state the break. This + replaces the earlier text "must be a major version" (S3 answer below). The + constitution asks for a "major-version note"; the note is the statement of this + decision. If the maintainer reads the constitution as a demand for a major + release, the maintainer amends it by pull request. The text of `ROADMAP.md` and + `COMPAT.md` that stage S3 added (pull request #397) says "must be a major + version" and needs the same change. +- 2026-11-05 (spec gap interview, C-016, maintainer decision, OP11): S4 deprecates + `GeologicalModel.scale` together with `rescale`. Its warning names + `model.bounding_box.project(points)`. Nothing in the library calls `scale`. +- 2026-11-05 (spec gap interview, C-017, maintainer decision, OP7): SC-002 for P2 + applies to the exact nodal field of R1. The P2 fit accuracy is a solver property. + Issue #399 records it. The backend table of the spec says so. +- 2026-11-05 (S4 plan, maintainer request): deprecate `GeologicalModel.rescale`. + The method keeps working and emits a `DeprecationWarning` that says it will be + removed. The four discarded calls in `exporters.py` go (OP8, calls). `scale` + follows in the same release (C-016). - 2026-11-05 (S3 end review, maintainer decision): a legacy payload needs both `local_origin` and `local_rotation`, or `declare_identity=True`. A partial input raises `InterpolatorFormatError` (`legacy_no_transform`) and names the @@ -114,7 +171,8 @@ The constitution (.specify/memory/constitution.md) has priority. --> - 2026-11-05 (S3 plan, maintainer decision): the legacy-load break (FR-019) gets a ROADMAP.md decision and a `COMPAT.md` row with a major-version note. No warning-only identity fallback exists. The release that contains S3 must - be a major version, or the maintainer records another decision. + be a major version, or the maintainer records another decision. (Replaced on + 2026-11-05 by C-015: a minor release with a note.) - 2026-11-04 (S2 wave 9, user decision): T022 found an existing defect. `ConstantNormFDIInterpolator` always fails with `IndexError` in @@ -147,9 +205,10 @@ S1 repairs the shared boundary and the fault frame: extent-direction row at the centre. S2 repairs the fold paths. S3 makes saved interpolators, copies and transform -changes keep the frame ("Stage S3 design"). S4 to S6 repair display -consumers, grid and diagnostics, and the auxiliary utilities and validation -matrix (see "Stages and rollback"). +changes keep the frame ("Stage S3 design"). S4 removes two extra conversions: the +display and export of observation data, and the intrusion classification. It also +deprecates `GeologicalModel.rescale` ("Stage S4 design"). S5 and S6 repair grid and diagnostics, and the auxiliary +utilities and validation matrix (see "Stages and rollback"). ## Technical context @@ -215,7 +274,7 @@ matrix (see "Stages and rollback"). ## Design -S1, S2 and S3 have the exact design below. S4 to S6 have scope only (OP1). +S1 to S4 have the exact design below. S5 and S6 have scope only (OP1). ### Types and data @@ -818,8 +877,10 @@ it merges, run this planner again on the merged result. 3. **Saved payload or creation call.** A dict is a saved payload if and only if `support` is a dict. Other dicts keep the creation behaviour. 4. **Legacy payload.** It has no recoverable transform (old `to_dict` never - wrote one). The caller must pass `local_origin` and `local_rotation`, or - `declare_identity=True`. The loader never guesses. + wrote one). The caller should pass `local_origin` and `local_rotation`, or + `declare_identity=True`. On the 1.x line, a call with no choice loads with the + identity frame and one `FutureWarning`; from the next major release it raises + (amendment C-018, see "S3 amendment"). 5. **Copy.** A copy shares the `BoundingBox` object of its source, as the factory does for the interpolators of one model. A loaded interpolator gets its own `BoundingBox`. @@ -1134,7 +1195,7 @@ under "S3 golden review" (C-004) before any change. the result. Else it records that no measure exists. - The legacy rule is a breaking change for callers that loaded old files with no choice (FR-019 exception, FR-028). It needs the ROADMAP.md decision and - the major-version note below. + the note on the version below (a minor release, C-015). #### S3 constitution check @@ -1163,7 +1224,7 @@ under "S3 golden review" (C-004) before any change. | `tests/unit/modelling/test_frame_persistence.py` | New | | `tests/unit/modelling/test_frame_copy_and_lifecycle.py` | New | | `tests/fixtures/api_surface_snapshot.json` | Add the new public names, only if the file lists `loop_interpolation` names (additive) | -| `COMPAT.md` | S3 migration notice with the major-version note | +| `COMPAT.md` | S3 migration notice with the note that the break ships in a minor release (C-015) | | `ROADMAP.md` | FR-019 decision under "Decisions"; status-log line when S3 completes | | `specs/005-coordinate-frame-correctness/verification.md` | S3 pre-fix, post-fix, golden review, backend matrix, rollback and run-time sections | @@ -1175,9 +1236,68 @@ The `COMPAT.md` row says: loading a legacy interpolator payload (no `local_origin` and `local_rotation`, or `declare_identity=True`, and raises `InterpolatorFormatError` otherwise. New payloads carry `format_version: 2`. `copy()` keeps the frame. A transform change after constraints exist raises -`StaleFrameError` at the next use. Major-version note: the release that -contains S3 must be a major version, or the maintainer records another -decision in `ROADMAP.md`. +`StaleFrameError` at the next use. Version note: by maintainer decision +(C-015) the release that contains S3 is a minor release; `ROADMAP.md` +records the exception to the 1.x rule. + +#### S3 amendment: backward-compatible legacy load (C-018, 2026-11-05) + +This amendment replaces the statements of "S3 decisions" item 4, of +`payload_kind` step 2, of `resolve_frame` step 3 and of the legacy tests that say +"a legacy payload with no choice raises". Everything else in S3 stays. + +- Behaviour on the 1.x line. A legacy payload (no `format_version`) with no + caller frame (`caller_frame` false) loads. `payload_kind` returns `"legacy"` + and does not warn; `resolve_frame` returns `None` for this case (the identity + frame, no bounding box). `InterpolatorFactory.from_dict` calls + `warn_legacy_identity()` once, as its last step, after the load succeeded, so a + failed load emits no warning. This is what older releases did. The + warning text starts with `legacy interpolator payload has no transform; assuming + the identity frame` and names `local_origin` and `local_rotation`, + `declare_identity=True`, and the next major release (which raises an error). +- Unchanged: a partial transform, a conflict, a malformed or an unsupported payload + still raise `InterpolatorFormatError`. These are new keyword arguments and new + checks, so no old call depends on them. +- One switch and one note. `_frame_persistence.py` gets a module constant + `_STRICT_LEGACY: bool = False` with the comment `REMOVE AT THE NEXT MAJOR RELEASE + (spec 005, C-018): delete the fallback, the constant, `warn_legacy_identity` and + `_external_stacklevel`; a legacy payload with no choice then always raises + InterpolatorFormatError("legacy_no_transform", ...)`. When the constant is + true, `payload_kind` raises as it did before this amendment. +- Warning category: `FutureWarning`, because the user must see it; a wrong + placement is silent. `warn_legacy_identity()` (in `_frame_persistence.py`) computes + the `stacklevel` so that the warning points at the first frame outside the + `loop_interpolation` package. This holds for `from_dict`, `from_json`, + `from_yaml` and a direct `InterpolatorFactory.from_dict` call. A fixed + `stacklevel` was wrong for three of the four (found in the review of the change). +- Tests. `test_legacy_payload_requires_choice` becomes + `test_legacy_payload_without_choice_warns_and_loads_identity` (3 engines): it + expects one `FutureWarning` with the message start above, a loaded interpolator + with `bounding_box is None`, and an unchanged source dict. New tests + `test_legacy_warning_points_at_caller` (4 entry points) and + `test_legacy_warning_not_emitted_when_load_fails` (3 engines) check the + attribution to the caller and that a failed load does not warn. For `ANCHORS[0]` with + rotation 0 it also checks the values against the source (SC-006). New test + `test_strict_legacy_switch_raises` (3 engines): with `_STRICT_LEGACY` set to + true through `monkeypatch`, the same call raises `InterpolatorFormatError` with + reason `legacy_no_transform` (the behaviour of the major release). The tests of + the partial transform, the supplied transform and the identity declaration do not + change. +- Documents (stage S3 branch, pull request #397): the `ROADMAP.md` decision + "Legacy interpolator payloads (spec 005, FR-019)" says: on 1.x the loader warns + and assumes identity; at the next major release it raises. The `COMPAT.md` S3 + row moves from the migration notices to a deprecation: old path "load of a legacy + payload with no choice", behaviour kept with a warning, earliest removal at the + next major release. The version note ("minor release", C-015) is removed from + both. `verification.md` records the new behaviour and corrects the P2 numbers + (value `7.8e-8`, gradient limit `1e-8`). The text of the pull request changes the + same way. +- Files: `packages/loop_interpolation/src/loop_interpolation/_frame_persistence.py`, + `tests/unit/modelling/test_frame_persistence.py`, `ROADMAP.md`, `COMPAT.md`, + `specs/005-coordinate-frame-correctness/verification.md`. +- The rollback check of S3 stays: a legacy file loads on the S3 code with the old + path and with a supplied transform; a file written by S3 fails on the old reader + (OP5). #### S3 task order (for the spec-tasks skill) @@ -1197,6 +1317,375 @@ with `declare_identity=True` (or the supplied transform) and compare with SC-006. On the S2 reader, load a file that S3 wrote; expect the `TypeError` that names `format_version` and no object. Record both in `verification.md`. +### Stage S4 design + +S4 makes the display, the data export, the fault-centre geometry and the +intrusion classification use the stored world locations (FR-022; U3.4 and +U3.5; "world-location comparison within `Tp`"). S4 removes two extra +conversions. It also deprecates `GeologicalModel.rescale`: the method keeps its +behaviour and warns that it will be removed (maintainer request, 2026-11-05). It +adds no new public name. Base: branch +`feat/005-s3-frame-persistence` (S3, pull request #397, stacked on #396). If S3 +changes before it merges, run this planner again on the merged result. + +#### S4 facts from the code (read on 2026-11-05) + +- The data of a feature builder is world data. `GeologicalFeatureBuilder.get_value_constraints`, + `get_gradient_constraints`, `get_tangent_constraints` and `get_norm_constraints` + (`_geological_feature_builder.py:395` to `466`) read `self.data`, the DataFrame + that the caller gave. The interpolator converts the rows once, when + `set_*_constraints` stores them. The old projection of the model data is a + comment (`geological_model.py:593`). +- `GeologicalFeature.get_data` (`_geological_feature.py:303` to `372`) takes + those world rows and calls `self.model.rescale(...)` on the location columns in + four places: gradient, norm, value with `value_map`, value without `value_map`. + `GeologicalModel.rescale` is `bounding_box.reproject` (local to world, + `geological_model.py:1103` to `1122`). So it adds the local origin, and it + applies the rotation, a second time. With a local origin of `(100,200,300)` and an + identity rotation, the point `(102,203,304)` is shown at `(202,403,604)`. This is + the display reproduction of the spec. The vectors are taken from the world rows + and are not converted, so they are already right. +- `get_data` has two callers: `Loop2DView.add_data` (`_2d_viewer.py`) and + `ModelExporter.save` (`_model_exporter.py:119` and `126`; it writes each object + with `save`). `StructuralFrame.get_data` (`_structural_frame.py:160`) delegates to + its three features. No other `get_data` converts a location: a grep for + `rescale` finds no other call in the feature classes, and + `IntrusionFeature.get_data` is `pass`. +- Pre-existing, outside S4: in the `value_map` branch the mask is + `value_constraints[value_constraints == v, :3]`. It compares the whole array, + not the value column (OP10). +- Intrusion classification. `IntrusionFrameBuilder.set_intrusion_steps_parameters` + (`intrusion_frame_builder.py:332` to `354`) takes the X, Y and Z columns of the + intrusion network data and passes `self.model.rescale(xyz, inplace=False)` to + `self.model.evaluate_model`. The network data is a copy of the user's rows + (`set_intrusion_frame_c0_data`, line 158), so it is world data. + `GeologicalModel.evaluate_model(xyz, *, scale=True)` takes world points; `scale` + is kept only for the signature (`geological_model.py:1212`). So the unit that + classifies each network point is read at a wrong location. The variable + `intrusion_network_data_xyz` is used only there. +- Fault centre. `FaultSegment.fault_centre` returns `builder.fault_centre`, a + world point since S1. `fault_ellipsoid` uses it directly (S1 fixed it). + `Loop2DView.add_fault_ellipse` (`_2d_viewer.py:211` to `224`) draws the ellipse at + `center[0], center[1]` with no conversion, which is right. The abutting regions + receive the world centre as `point` (`_fault_segment.py:463` and `469`); + `utils/regions.py` was not read (OP9). +- Exporters. The four calls `model.rescale(verts)`, `model.rescale(points)`, + `model.rescale(xyz)` and `model.rescale(bbox)` in `exporters.py` (lines 92, 366, + 431 and 480) discard the result, because `inplace` is `False` by default. They + change nothing. The exporters build world inputs (`regular_grid(local=False)` and + the world bounding box) and `evaluate_model` projects them internally, so the + written coordinates are world coordinates. The argument `real_coords=False` cannot + produce model coordinates; it is ignored (pre-existing, outside S4; OP8). + `ModelExporter.get_block_model` is right: `cell_centres()` is local, `rescale` + makes it world, and `evaluate_model` projects it again. +- Other `reproject` calls are right. `LoopStructural/interpolators/_api.py:151`, + `169` and `187` convert stored local interpolator data to world. + `_geological_feature.py:258`, `266` and `286` do the same for the misfit methods. + Those methods change the stored arrays in place (`grad /= norm`) and + `grad[:, :3:6]` selects the position columns; this belongs to S5 (FR-023). +- Callers of `rescale` (grep on 2026-11-05, the whole repository): `exporters.py` + (4 discarded calls), `_model_exporter.py:77` (`get_block_model`), + `_geological_feature.py` (4 live calls and 1 comment in `get_data`), + `intrusion_frame_builder.py:353`, and two calls in + `examples/3_fault/plot_1_faulted_intrusion.py` (lines 71 and 100). No other + package calls it. `GeologicalModel.scale` has no caller. +- The example has the same double conversion as `get_data`. It calls + `model.rescale(model.data[...][["X", "Y", "Z"]].values, inplace=False)` on world + data of the model, and draws the points with `viewer.add_points`. The prefault + points appear shifted by the local origin. This is a third display case of FR-022. +- `rescale` and `scale` are stable public API: `@public_api(tier="stable")` + (`geological_model.py:1104`, `1125`), listed in `API.md:122` and in + `tests/fixtures/api_surface_snapshot.json:30`. `ROADMAP.md:810` records that they + "stay as public, signature-stable pass-throughs" to the bounding box. The + contract test compares only the signature and the tier, so a warning in the body + does not change the snapshot. `public_api` wraps the function and passes every + call through. +- The repository has a deprecation pattern. `set_stratigraphic_column` + (`geological_model.py:649`) calls `warnings.warn(message, DeprecationWarning, + stacklevel=2)`. The versioning policy of `ROADMAP.md` ("Versioning policy") and the + "Deprecation lifecycle" of `COMPAT.md` require, for a deprecation on the 1.x line: + a runtime `DeprecationWarning`, an entry in the `COMPAT.md` table, and a removal + not before 2 minor releases after the first release that warns. pytest has no + `filterwarnings` option in `pyproject.toml`, so a warning does not fail a test. +- `tests/unit/io/test_exporters.py` has two stand-in models with a `rescale` + method (lines 53 and 67). They may be unused after the dead exporter calls go; + the S4 task checks that the file passes unchanged. +- Survey correction. `_3d_viewer.py`, `intrusion_builder.py` and `_fault_segment.py` + need no change in S4. `exporters.py` and `_model_exporter.py` change only to stop + calling the deprecated method (see the decisions). + +#### S4 decisions + +1. **Contract.** Every `get_data` returns world locations and world vectors. A + feature builder holds world data, so `get_data` copies it and converts nothing. +2. **Fix 1.** In `GeologicalFeature.get_data`, remove the four calls of + `self.model.rescale`. Keep every other expression as it is, also the + `value_map` mask (OP10). +3. **Fix 2.** In the intrusion frame builder, the classification evaluates the model + at the stored world points. A new private method holds the expression, so a test + can reach it. +4. **Deprecate `GeologicalModel.rescale`.** The method keeps its behaviour and its + signature (local to world, `bounding_box.reproject`). It emits one + `DeprecationWarning` for each call, with `stacklevel=2`, that names the + replacement `model.bounding_box.reproject(points)`. The tier stays `"stable"` + until the method is removed. `GeologicalModel.scale` (world to local) is + deprecated in the same release, with the replacement + `model.bounding_box.project(points)` (C-016). Removal: not + before 2 minor releases after the first release that warns, and not before the + known callers (the QGIS plugin) have moved. The `ROADMAP.md` decision of the + 2026-07-30 status-log entry "2c-3" (line 810: "`scale()`/`rescale()` stay as public," + "signature-stable pass-throughs") changes for both methods: S4 records the new decision. +5. **Callers move first.** Our own code must not call the deprecated method, or a + user would see our warning. The four live calls of `get_data` and the call of the + intrusion classification go with fixes 1 and 2. The four discarded calls in + `exporters.py` are removed (they do nothing). `ModelExporter.get_block_model` + uses `model.bounding_box.reproject(...)`. The two example calls are removed, + because they apply a second conversion to world data. +6. **No change** to the viewers (`_3d_viewer.py`, `_2d_viewer.py`), + `_fault_segment.py` and `intrusion_builder.py`. The argument + `real_coords` of the exporters stays as it is (OP8). + +#### S4 contracts + +| Boundary | Input | Output | Owner | +|---|---|---|---| +| `GeologicalFeature.get_data` | world rows of the builder | `ValuePoints.locations`, `VectorPoints.locations`: world points; `VectorPoints.vectors`: world directions | `_geological_feature.py` | +| Viewer and exporter of data | objects from `get_data` | drawn or written at their `locations` | `Loop2DView.add_data`, `ModelExporter.save` (unchanged) | +| Intrusion classification | world X, Y, Z of the network data | stratigraphic ids from `model.evaluate_model` at those points | `IntrusionFrameBuilder._network_model_values` | +| Fault-centre geometry | `FaultSegment.fault_centre` (world) | ellipsoid or ellipse at that point | `fault_ellipsoid`, `Loop2DView.add_fault_ellipse` (unchanged) | +| `GeologicalModel.rescale` (deprecated) | local points | world points, as before; one `DeprecationWarning` | `geological_model.py`; replacement `bounding_box.reproject` | +| `GeologicalModel.scale` (deprecated) | world points | local points, as before; one `DeprecationWarning` | `geological_model.py`; replacement `bounding_box.project` | + +#### S4 functions and signatures + +`GeologicalFeature.get_data(self, value_map: dict | None = None)` +(`_geological_feature.py`): the four location arguments become +`gradient_constraints[:, :3]`, `norm_constraints[:, :3]`, +`value_constraints[value_constraints == v, :3]` and `value_constraints[:, :3]`. +The docstring says: "The locations and the vectors are in world coordinates, as +the builder stores them." No other line changes. The commented-out block at the +end of the method stays as it is. + +New private method of `IntrusionFrameBuilder` (`intrusion_frame_builder.py`): + +```python +def _network_model_values(self, data: pd.DataFrame) -> np.ndarray: + """Return the stratigraphic id of each network point. + + Parameters + ---------- + data : pandas.DataFrame + Network data with the world columns ``X``, ``Y`` and ``Z``. + + Returns + ------- + numpy.ndarray + The id of the unit at each point. The model is evaluated at the world + points; no conversion is applied. + """ + xyz = data.loc[:, ["X", "Y", "Z"]].to_numpy(dtype=float) + return self.model.evaluate_model(xyz) +``` + +In `set_intrusion_steps_parameters`: delete the assignment of +`intrusion_network_data_xyz` (lines 333 to 335, three lines; the variable has no +other use), and replace the statement at line 352 with + +```python +intrusion_network_data.loc[:, "model_values"] = self._network_model_values( + intrusion_network_data +) +``` + +The name `_network_model_values` was free on 2026-11-05 (`git grep -w`: zero +matches). + +`GeologicalModel.rescale` (`geological_model.py`): the signature +`rescale(self, points: np.ndarray, *, inplace: bool = False) -> np.ndarray` and the +decorator `@public_api(tier="stable")` do not change. The body becomes: + +```python +warnings.warn( + "GeologicalModel.rescale is deprecated and will be removed in a future " + "release; use model.bounding_box.reproject(points) instead", + DeprecationWarning, + stacklevel=2, +) +return self.bounding_box.reproject(points, inplace=inplace) +``` + +The module already imports `warnings` (line 649). The docstring gets a +`.. deprecated::` note that names `bounding_box.reproject` and says "Deprecated by +spec 005, stage S4". `GeologicalModel.scale` (`geological_model.py`) gets the same change. The signature +`scale(self, points: np.ndarray, *, inplace: bool = False) -> np.ndarray` and the +decorator do not change. The body becomes: + +```python +warnings.warn( + "GeologicalModel.scale is deprecated and will be removed in a future " + "release; use model.bounding_box.project(points) instead", + DeprecationWarning, + stacklevel=2, +) +return self.bounding_box.project(np.array(points).astype(float), inplace=inplace) +``` + +The other callers change as follows: + +- `exporters.py`: delete the four statements `model.rescale(verts)`, + `model.rescale(points)`, `model.rescale(xyz)` and `model.rescale(bbox)`. If + `real_coords` has then no use in a function, keep the argument (OP8). +- `_model_exporter.py`, `get_block_model`: replace + `model.rescale(model.bounding_box.cell_centres())` by + `model.bounding_box.reproject(model.bounding_box.cell_centres())`. +- `examples/3_fault/plot_1_faulted_intrusion.py`: in the two `viewer.add_points` + calls, pass `model.data[model.data["feature_name"] == "strati"][["X", "Y", "Z"]].values` + directly (world points) and delete the `model.rescale(...)` wrapper and its + `inplace=False`. + +#### S4 tests + +`tests/fixtures/coordinate_frames.py` gets one helper: +`world_foliation_model(anchor, rotation_deg) -> tuple[GeologicalModel, GeologicalFeature, pd.DataFrame]`. +It makes `model_with_frame(anchor, anchor + (8, 10, 12), rotation_deg)`, a data +frame in world coordinates with the columns `X, Y, Z, feature_name, val, nx, ny, nz, gx, gy, gz, w` +(27 value rows from `r1_observations(anchor)`, 4 gradient rows and 4 normal rows at +`anchor + (2, 3, 4)`, `(5, 6, 7)`, `(3, 8, 5)` and `(6, 4, 9)` with the unit +direction `R1_GRADIENT / norm`), and the feature +`model.create_and_add_foliation("r1", ...)` built without a solve. It returns the +model, the feature and the data frame. The test task reads +`tests/integration/test_interpolator.py` and `tests/unit/interpolator/test_outside_box.py` +for the call pattern of `create_and_add_foliation` and copies it. + +`tests/unit/modelling/test_world_location_consumers.py` (new): + +| Test function | Cells | Checks | +|---|---|---| +| `test_feature_data_locations_are_world` | 4 anchors x 3 rotations = 12 | `feature.get_data()` gives a `ValuePoints` (value), and `VectorPoints` (gradient and norm). Each `locations` equals the input rows within `tp(L, W)`; each `vectors` equals the input vectors within `1e-12` (U3.4) | +| `test_spec_example_observation_is_not_shifted` | `ANCHORS[1]` x 3 rotations = 3 | the observation `(102,203,304)` is at `(102,203,304)` and not at `(202,403,604)` | +| `test_exported_data_keeps_world_locations` | 4 anchors, rotation 30 = 4 | each object of `get_data()` is saved with `save` to a CSV file (the task reads `_point.py` `save` first for the supported extension), and the `x`, `y`, `z` columns equal the input within `tp(L, W)` (U3.4, export) | +| `test_intrusion_classification_uses_world_points` | 4 anchors x 3 rotations = 12 | `IntrusionFrameBuilder._network_model_values(data)` is called on `object.__new__(IntrusionFrameBuilder)` with `.model` set to a real rotated `GeologicalModel` whose `evaluate_model` is replaced by a spy; the spy receives exactly the world X, Y, Z (`np.array_equal`) (U3.5) | +| `test_2d_viewer_fault_ellipse_centre_is_world` | 2 anchors x 2 rotations (0 and 30) = 4 | `Loop2DView.add_fault_ellipse` is called with a stand-in `self` that has `ax` (a Matplotlib `Agg` axis) and `model`; the patch centre equals `fault.fault_centre[:2]` within `tp(L, W)` (U3.5, coverage, no defect). The fault comes from `_fault(anchor, rotation, "PLI")` of `test_fault_frame_analytic.py` | +| `test_rescale_warns_and_keeps_behaviour` | 4 anchors x 3 rotations = 12 | `model.rescale(points)` raises one `DeprecationWarning` (`pytest.warns`, the message contains `bounding_box.reproject`), the warning is attributed to the test file (`stacklevel`), the result equals `model.bounding_box.reproject(points)` within `tp(L, W)`, and `inplace=True` changes the passed array like `reproject` does (SC-014, FR-028) | +| `test_scale_warns_and_keeps_behaviour` | 4 anchors x 3 rotations = 12 | `model.scale(points)` raises one `DeprecationWarning` (the message contains `bounding_box.project`), attributed to the test file, with a result equal to `model.bounding_box.project(points)` within `tp(L, W)` | +| `test_internal_callers_do_not_warn` | 1 | with the filter `warnings.filterwarnings("error", message=".*(rescale|scale) is deprecated.*")`: `feature.get_data()`, `ModelExporter.get_block_model(model)` and `IntrusionFrameBuilder._network_model_values(data)` run with no warning; the exporter functions that `tests/unit/io/test_exporters.py` already calls run with no warning (the task reads that file for the call pattern) | + +Pre-fix evidence (FR-031). The first four tests fail on the current tree because +of the extra conversion: the first, second and third by a location shifted by the +local origin (`(202,403,604)` for the example), and the fourth because the spy +receives the shifted points (the method does not exist yet, so the first failure is +`AttributeError`). Record also a behavioural measure with the production path: the +shift of the example point for `ANCHORS[1]` and each rotation. The fifth test +passes before the fix; record it as "coverage, no defect". + +The existing tests that must pass with no change: +`tests/unit/modelling/intrusions/test_intrusions.py`, +`tests/unit/io/test_exporters.py`, `tests/unit/test_public_api_contract.py` (the +signature and the tier of `rescale` do not change), and the S1 to S3 test files. The intrusion tests build a +model. If a translated frame changes an expected value, record the change under +"S4 golden review" (C-004) with the defect and its effect, before any change. +With an identity frame (origin 0, rotation 0) nothing changes. + +#### S4 backend and capability cells + +| Cell | Result in S4 | +|---|---| +| Python FDI foliation data (display and export of observations) | Mandatory. The tests above use `FDI`. | +| Fault centre in the 2D viewer | Coverage test above (PLI fault). | +| Intrusion classification | Mandatory. Spy test; the intrusion model tests stay unchanged. | +| Native, Surfe, browser | Not in S4. The viewer and exporters do not call them. | + +#### S4 risks + +- Intrusion models in a translated frame classified their network data at wrong + points. The corrected classification can change the result of such a model + (a golden result). The intrusion tests decide: record each change under C-004. +- Users who corrected the display by shifting their data now see a shifted + display. This is the corrected behaviour (`COMPAT.md` notice). +- `GeologicalModel.rescale` keeps its meaning (local to world) and now warns. The + S4 grep (`git grep -n "\.rescale("`) in the finish task must show only the + commented block of `get_data` and the tests of the deprecation. +- A downstream caller (the QGIS plugin, user scripts) gets a `DeprecationWarning`. + Python hides it outside `__main__` and test runs. The message names the + replacement. The removal waits for the lifecycle of `COMPAT.md`. + +#### S4 constitution check + +| Project rule | Result | How S4 agrees | +|---|---|---| +| Work stays within the existing packages | Pass | Two source files change, in `LoopStructural`. No new module. | +| Gates `pytest` and `ruff check LoopStructural packages` | Pass | The S4 row of "Stages and rollback" names both. | +| numpy docstrings, type hints, py39 | Pass | The new method has a numpy docstring and type hints. | +| The public `GeologicalModel` API stays compatible; a break needs a ROADMAP.md decision and a major-version note | Pass | No signature, tier or behaviour breaks. `rescale` is deprecated on the 1.x line with a runtime warning, as the versioning policy and the lifecycle of `COMPAT.md` require; `scale` and `evaluate_model` do not change. `get_data` keeps its signature; its locations become correct (known meaning). Removal is a later, separate change. | +| Stages can be tested and reversed independently | Pass | S4 reverts to the S3 merge. | +| `ROADMAP.md` owns decisions; update it when one changes | Pass | The decision of line 810 ("`scale()`/`rescale()` stay as public pass-throughs") changes for `rescale`. S4 adds the decision "Deprecation of GeologicalModel.rescale and scale (spec 005, S4)" under "Decisions". The "005 complete" line stays in S6. | +| Feature documents stay under `specs/NNN-name/` | Pass | Only `plan.md`, `tasks.md` and `verification.md` change there. | + +#### S4 files + +| File | Change | +|---|---| +| `LoopStructural/modelling/core/geological_model.py` | `rescale` and `scale`: warn with `DeprecationWarning`; docstrings | +| `LoopStructural/modelling/features/_geological_feature.py` | `get_data`: remove four `self.model.rescale` calls; docstring | +| `LoopStructural/modelling/intrusions/intrusion_frame_builder.py` | New `_network_model_values`; use it in `set_intrusion_steps_parameters`; delete `intrusion_network_data_xyz` | +| `LoopStructural/export/exporters.py` | Delete the four discarded `model.rescale` calls | +| `LoopStructural/modelling/core/_model_exporter.py` | `get_block_model`: use `bounding_box.reproject` | +| `examples/3_fault/plot_1_faulted_intrusion.py` | Pass the world points to `viewer.add_points`; remove two `model.rescale` wrappers | +| `tests/fixtures/coordinate_frames.py` | New helper `world_foliation_model` | +| `tests/unit/modelling/test_world_location_consumers.py` | New | +| `COMPAT.md` | S4 migration notice; one row in the deprecation table for each of `GeologicalModel.rescale` and `GeologicalModel.scale` | +| `ROADMAP.md` | Decision "Deprecation of GeologicalModel.rescale and scale (spec 005, S4)" under "Decisions" | +| `API.md` | Line 122: mark `rescale` as deprecated, with the replacement | +| `specs/005-coordinate-frame-correctness/verification.md` | S4 pre-fix, post-fix, golden review, backend matrix, rollback and run-time sections | + +Not changed (the survey in "Later stages" listed them): `_3d_viewer.py`, +`intrusion_builder.py` and `_fault_segment.py`. + +The deprecation rows of `COMPAT.md` (one for `rescale`, one for `scale`) follow +the columns of that table. The `rescale` row: old path +`GeologicalModel.rescale(points, inplace=False)`; new path +`GeologicalModel.bounding_box.reproject(points, inplace=False)`; shim added +`2026-11-05` (the date of the run); first released in "TBD (next release containing +this warning)"; introduced by the S4 commit; earliest removal "TBD (2 minor +releases after the first release that warns)"; removed in empty; status +"Active"; owner "Core maintainers". The `scale` row has the old path +`GeologicalModel.scale(points, inplace=False)` and the new path +`GeologicalModel.bounding_box.project(points, inplace=False)`; the other columns +are the same. The `API.md` line reads: "Geometry: `scale` (deprecated, use +`bounding_box.project`), `rescale` (deprecated, use `bounding_box.reproject`), +`regular_grid`, `bounding_box` (property)". + +The `COMPAT.md` row says: `GeologicalFeature.get_data()` (used by +`Loop2DView.add_data` and `ModelExporter.save`) returned locations moved by the +local origin and rotated a second time, and the intrusion classification evaluated +the model at such points. Both now use the stored world locations. With an +identity frame nothing changes. + +#### S4 task order (for the spec-tasks skill) + +1. The fixture helper (no dependency on code changes). +2. The test file, in three parts in one file, one task after the other: display + and export tests; the intrusion test; the 2D viewer test. +3. The pre-fix evidence in `verification.md` (after all tests, before any fix). +4. The tests of the deprecation (written before it; they must fail first), in the + same test file, one task after the others. +5. The fixes and the deprecation in parallel (they are in separate files): + `get_data`; the intrusion helper and its call site; the `rescale` warning; the + exporter files; the example. The tests of step 4 pass only when all callers + have moved, so the finish task of the group runs them together. +6. The golden review of the intrusion tests, `COMPAT.md`, `ROADMAP.md`, `API.md`, + the rollback check, and last the gates (the orchestrator runs the full suites + when memory is tight). + +S4 rollback check. Revert to the S3 merge. Then: (a) the import +`from loopstructuralvisualisation import Loop2DView, Loop3DView` works and +`inspect.signature(Loop2DView.add_data)` is equal before and after; (b) on the S3 +code, `get_data()` shows the spec example at `(202,403,604)`, on the S4 code at +`(102,203,304)` (recorded in `verification.md`); (c) with an identity frame the +two codes give the same `get_data()` output; (d) on the S3 code `model.rescale(p)` +and `model.scale(p)` give no warning, on the S4 code each gives one, and the values +are equal. + ### Stages and rollback | Stage | Files and behaviour | Acceptance check | Rollback target and compatibility check | @@ -1204,7 +1693,7 @@ that names `format_version` and no object. Record both in `verification.md`. | S1 | Shared transform boundary, reference relationships for FDI and PLI, fault frame, buffer and forced geometry, rotation check, resize stability (this design) | New S1 tests pass for FDI and PLI; `pytest`; `ruff check LoopStructural packages`; `pytest packages/loop_common/tests packages/loop_interpolation/tests` | Revert the S1 commits to master `ebc7c699`. Check: the existing `tests/unit/modelling/test__fault_builder.py` and `packages/loop_common/tests/test_bounding_box.py` pass on the reverted tree. No saved format changed. | | S2 | Fold geometry, directional regularisation, quadrature, masks, sign alignment for FDI-fold, PLI discrete fold, P2 and P2-fold; constant-norm variants (FR-008; R3 for P2; R4) ("Stage S2 design") | New S2 tests (SC-002 for constant norm; SC-007, SC-008 and SC-013 for every mandatory fold and P2 cell); the existing fold package tests pass with no change; `pytest`; `ruff check LoopStructural packages`; package tests | Revert to the S1 merge. Check: the S1 tests still pass. No saved format changed. | | S3 | Persistence format version, transform and coordinate-space metadata ("Stage S3 design"); no second projection at load; copies keep the frame; legacy load requires a transform or identity declaration; transform change policy and restored-point cache invalidation (FR-016 to FR-020); ROADMAP.md decision for FR-019; `COMPAT.md` migration note | SC-006, SC-012, SC-016, SC-017 | Revert to the S2 merge. Check: old-format R1 files load through their documented path; a new-format file fails with 1 actionable error and 0 state changes on the old reader (SC-017). | -| S4 | Display, export of observation geometry, fault-centre geometry and intrusion classification use stored world locations (FR-022) | World-location comparison within `Tp` (U3.4, U3.5) | Revert to the S3 merge. Check: viewer import paths unchanged. | +| S4 | Display, export of observation geometry, fault-centre geometry and intrusion classification use stored world locations (FR-022; "Stage S4 design") | World-location comparison within `Tp` (U3.4, U3.5) | Revert to the S3 merge. Check: viewer import paths unchanged. | | S5 | Cell-centre sampling, remaining resize paths, non-mutating diagnostics, residual and exclusion report (FR-014 remainder, FR-015, FR-023, FR-024) | SC-009, SC-011 | Revert to the S4 merge. Check: diagnostics report fields unchanged for callers. | | S6 | Auxiliary point transformations and duplicate entry points (FR-025); displacement validation R2 (FR-026, FR-027); backend matrix incl. native and Surfe (FR-030); golden review (FR-029); 005 completion line in ROADMAP.md | SC-004, SC-010, SC-014, SC-015, SC-018 | Revert to the S5 merge. Check: `LoopStructural.utils` and `loop_common.math` import paths unchanged. | @@ -1271,8 +1760,10 @@ current tree is recorded as "not reproduced"; its test stays as a guard. constant-norm variants ("Stage S2 design"). S6 covers native and Surfe (OP1). - **Cross-language fixtures**: Not applicable in S1. -- **Grid and display checks**: S1 checks support enclosure and resize. Cell - centres and display are S4 and S5. +- **Grid and display checks**: S1 checks support enclosure and resize. S4 checks + the display and export of data, the intrusion classification and the fault + centre in the 2D viewer (`test_world_location_consumers.py`). Cell centres are + S5. - **Performance check**: none required. Record the S1 test time (OP2). - **Merge gates**: `pytest` and `ruff check LoopStructural packages`. Also run `pytest packages/loop_common/tests packages/loop_interpolation/tests`, @@ -1363,16 +1854,18 @@ grep survey on 2026-11-04 and are not final. - **S3**: planned. See "Stage S3 design" and "S3 files". The survey listed `_support_factory.py` and `_fault_segment.py`; the code shows that S3 does not change them (no restoration cache exists). -- **S4**: `packages/loopstructural_visualisation/src/loopstructuralvisualisation/_3d_viewer.py`, - `LoopStructural/modelling/intrusions/intrusion_builder.py`, - `LoopStructural/modelling/intrusions/intrusion_frame_builder.py`, - `LoopStructural/modelling/features/fault/_fault_segment.py`, - `LoopStructural/export/exporters.py`. +- **S4**: planned. See "Stage S4 design" and "S4 files". The survey listed + `_3d_viewer.py`, `intrusion_builder.py`, `_fault_segment.py` and + `exporters.py`; the code shows that S4 does not change them. S4 changes + `_geological_feature.py` and `intrusion_frame_builder.py`. - **S5**: `packages/loop_common/src/loop_common/geometry/_bounding_box.py` (`cell_centres`), `_structured_grid.py`, `_structured_grid_2d.py`, `packages/loop_common/src/loop_common/supports/_3d_structured_grid.py`, `_3d_rectilinear_grid.py`, - `packages/loop_interpolation/src/loop_interpolation/_diagnostics.py`. + `packages/loop_interpolation/src/loop_interpolation/_diagnostics.py`, and + `LoopStructural/modelling/features/_geological_feature.py` (found in the S4 + survey: the misfit methods change the stored arrays in place with + `grad /= norm`, and `grad[:, :3:6]` selects the position columns). - **S6**: `packages/loop_common/src/loop_common/math/_transformation.py`, `LoopStructural/utils/_transformation.py`, `packages/loop_interpolation/src/loop_interpolation/_native.py`, @@ -1404,13 +1897,13 @@ grep survey on 2026-11-04 and are not final. | FR-019 | `payload_kind`, `resolve_frame`, `InterpolatorFormatError`; ROADMAP.md decision; `COMPAT.md` row | `test_legacy_*`, `test_conflicting_inputs`, `test_malformed_metadata`, `test_unsupported_future_version` | None | | FR-020 | `frame_signature`, `_check_frame`, `StaleFrameError` (reject at use) | `test_stale_frame_rejected_without_state_change`, `test_fault_restoration_uses_current_frame` | Policy choice OP4; no restoration cache exists | | FR-021 | `validate_rigid_rotation` in `set_local_transform` and `rotation_xy`; saved metadata checked in `resolve_frame` | `test_rigid_transform_contract.py`; `test_malformed_metadata` | None | -| FR-022 | S4 | S4 | OP1 | +| FR-022 | Remove the extra `rescale` in `GeologicalFeature.get_data` and in the intrusion classification (`_network_model_values`); the fault centre is a world point since S1 and its consumers are unchanged | `test_world_location_consumers.py` (display, export, classification, 2D ellipse); S1 `test_fault_ellipsoid_centre_is_world` | OP8 to OP10 | | FR-023 | S5 | S5 | OP1 | | FR-024 | `excluded_rows` (S1 part) | Restricted-support exclusion count | Report integration S5, OP1 | | FR-025 | S6 | S6 | OP1 | | FR-026 | S1: values, gradients, residuals (SC-003, SC-008); displacement S6 | `test_fault_frame_analytic.py` | Displacement OP1 | | FR-027 | S6 | S6 | OP1 | -| FR-028 | No signature change except new keyword arguments; `COMPAT.md` note; FR-019 exception recorded in ROADMAP.md | API contract tests; existing round-trip tests unchanged | None | +| FR-028 | No signature change except new keyword arguments; `COMPAT.md` note; FR-019 exception recorded in ROADMAP.md; S4: `rescale` deprecated with its behaviour kept (`COMPAT.md`, `ROADMAP.md`, `API.md`) | API contract tests; existing round-trip tests unchanged; `test_rescale_warns_and_keeps_behaviour` | None | | FR-029 | `verification.md` golden review section | Review record per changed result | None | | FR-030 | S1 FDI and PLI cells; S2 P2, fold and constant-norm cells | S1 and S2 tests | Native and Surfe cells S6, OP1 | | FR-031 | Pre-fix rule in "Test strategy" | `verification.md` | None | @@ -1467,6 +1960,16 @@ grep survey on 2026-11-04 and are not final. bytes]` is the key for any cache that depends on the frame; spec 003 keys its caches on it and clears them when it changes. `copy()` keeps the `bounding_box` object of its source. +- **S4 world data contract (owner: `LoopStructural`)**: every + `get_data` of a feature returns `ValuePoints` and `VectorPoints` with world + locations and world vectors; `Loop2DView.add_data` and `ModelExporter.save` + use them as they are. `GeologicalModel.rescale` (local to world) and `scale` (world to local) are + deprecated stable API. They warn (`DeprecationWarning`; use + `bounding_box.reproject` and `bounding_box.project`). A caller must not pass + `rescale` world points. + `IntrusionFrameBuilder._network_model_values(data)` evaluates the model + at the world X, Y and Z of the network data. Spec 004 and the QGIS plugin read + display data through `get_data` and get world locations. - **Exclusion record**: `GeologicalFeatureBuilder.excluded_rows: dict[str, dict[str, int]]`. S5 connects it to the diagnostics report. - **Fault geometry**: an explicit `fault_center` is a world point; the diff --git a/specs/005-coordinate-frame-correctness/run-state.json b/specs/005-coordinate-frame-correctness/run-state.json index c7d17a0f7..568a71c1d 100644 --- a/specs/005-coordinate-frame-correctness/run-state.json +++ b/specs/005-coordinate-frame-correctness/run-state.json @@ -1,10 +1,10 @@ { "spec": "005-coordinate-frame-correctness", - "stage": "S3", - "worktree": "C:/Users/lachl/dev/LoopStructural-005-s3", - "branch": "feat/005-s3-frame-persistence", + "stage": "S4", + "worktree": "C:/Users/lachl/dev/LoopStructural-005-s4", + "branch": "feat/005-s4-world-locations", "review_mode": "wave", - "start_commit": "91ffca4a269252c32cefe077261edd27f10a4b14", + "start_commit": "9b3507a9a4d4c7d7a903a08c7d33619a7546bf61", "runtime": { "runtime": "claude", "max_workers": 5, @@ -14,278 +14,242 @@ "review_spec": "opus", "review_verifier": "opus" }, - "env": "PYTHONPATH=;/packages/loop_common/src;/packages/loop_interpolation/src; python = main checkout .venv (C:/Users/lachl/dev/LoopStructural/.venv/Scripts/python.exe); run the root pytest and the package tests in separate calls, one heavy run at a time (low memory)", - "baseline": { - "lint": "pass: ruff check LoopStructural packages (S3 worktree, start commit)", - "pytest": "1760 passed, 5 skipped, 13 warnings in 1052.40 s at 662d2713 (S2 code; commits after it change only specs/README/docs). Not run again at S3 start because of low memory.", - "packages": "912 passed, 47 skipped, 2 xfailed in 126.56 s at 662d2713" - }, - "base_note": "Built from docs/005-s3-plan (S2 code from feat/005-s2-fold-frames, PR #396 not merged). Rebase onto master after #396 merges.", + "env": "PYTHONPATH=C:/Users/lachl/dev/LoopStructural-005-s4;C:/Users/lachl/dev/LoopStructural-005-s4/packages/loop_common/src;C:/Users/lachl/dev/LoopStructural-005-s4/packages/loop_interpolation/src; python = main checkout .venv (C:/Users/lachl/dev/LoopStructural/.venv/Scripts/python.exe); run the root pytest and the package tests in separate calls, one heavy run at a time (low memory); detached script + foreground poll for long runs; logs with Out-File -Encoding utf8; for the viewer test (T054) add /packages/loopstructural_visualisation/src to PYTHONPATH (else it skips); use PowerShell for pytest (Bash resolves loop_interpolation from the main checkout); pyevtk for test_exporters: pip install --target /pyevtk_pkg pyevtk (system python), add the dir to PYTHONPATH", + "base_note": "Built from docs/005-s4-plan (9b3507a9) on origin/master (S1 to S3 and PR #400 merged). Not pushed; no PR.", "waves": [ { - "wave": 14, - "base": "b32707fd62957ccb5d4c606c2bbe5bff22a6b7a9", + "wave": 23, + "base": "9b3507a9a4d4c7d7a903a08c7d33619a7546bf61", "tasks": [ - "T033", - "T034" + "T051" ], - "check": "scope pass (VERIFY); ruff pass; black pass; fixture collects 0 tests; 5 engines build (exact: errors <= 2e-10 at anchor 3 rot 30; cn fit errors 4e-9 and 1.3e-11 at anchor 0); imports from the S3 worktree", - "commit": "c7a6802dd750bcfb0417828fe9b37a1269605f70", + "check": "scope pass; ruff pass; payload_declares 36 passed", + "commit": "7b4596046b230fb5f8cd5f55c40e6e277d85f932", "review": "pass: sonnet, no findings" }, { - "wave": 15, - "base": "5cd6013450bf1aecc419f29cf0443fa879b71e20", + "wave": 24, + "base": "7b4596046b230fb5f8cd5f55c40e6e277d85f932", "tasks": [ - "T035", - "T037" + "T052" ], - "check": "scope pass (VERIFY); ruff and black pass. Pre-fix: test_frame_persistence.py 108 failed 3 passed (identity cells); test_frame_copy_and_lifecycle.py 41 failed 6 passed (copy 36/36, stale 5/5 fail; restored 1 and rebuild 5 pass = coverage, no defect). First errors: KeyError format_version/transform; nan after rotated round trip; copy().bounding_box is None; DID NOT RAISE StaleFrameError", - "commit": "87928866225cc1f56b1634d791fa75ceec2b72da", - "review": "pass after rejecting 1 major: sonnet, 1 major rejected (F in spec SC definition is the max abs reference scalar value = observations; S2 test_constant_norm_frames uses the same scale), 2 minor open" - }, - { - "wave": 16, - "base": "acef571f5ed93c72ccf7b00b4161865e348213e7", - "tasks": [ - "T036", - "T038" - ], - "check": "scope pass (VERIFY); ruff and black pass. Pre-fix: legacy/conflicting/malformed/unsupported/creation_call/old_reader 58 failed (TypeError unexpected keyword declare_identity or format_version; DID NOT RAISE); T035 file 166 failed 3 passed (identity cells); fault_restoration 2 failed (DID NOT RAISE StaleFrameError). apply_to_points reaches the interpolators through evaluate_value/evaluate_gradient of the frame features. Later part of the fault test (rebuild vs control) ran clean with the raise check off (FDI 18.6 s, PLI 4.2 s)", - "commit": "d3813ce2a22be699a3ef768a904b25a9dc19c883", - "review": "pass after fixes: sonnet first review 1 major + 1 minor fixed (2a4a6636); recheck found 1 new major (T036 conflict test demanded the word supplied for declare_identity against a saved transform; my plan wording was ambiguous). User decision 2026-11-05: fix it and verify without a third review round.", - "fix_commit": "2a4a66368d6afee03f639bc73a06ddb8caeaa493" + "check": "scope pass; ruff pass; 18 failed 1 passed pre-fix (shift by local origin; rotation)", + "commit": "448aba2f29d1dfef4d1a9060335a6e94a010768d", + "review": "pass: sonnet, no findings" }, { - "wave": 17, - "base": "caf0a0e73e568355dbad84c11ec7e7b207e7788f", + "wave": 25, + "base": "448aba2f29d1dfef4d1a9060335a6e94a010768d", "tasks": [ - "T039" + "T053" ], - "check": "scope pass (VERIFY: only verification.md). Pre-fix totals 209 failed 9 passed over 218 cells (persistence 166/3, lifecycle 43/6). Behavioural: rotated fdi round trip gives NaN (2 of 2); copy().bounding_box is None = True; stale evaluation after set_local_transform returns NaN with no error", - "commit": "b139482282c2c49755115e145db8a8519fefd439", + "check": "scope pass; ruff pass; 12 failed AttributeError pre-fix", + "commit": "1b4abbb0020cce326ef56c7452de704af79513ad", "review": "pass: sonnet, no findings" }, { - "wave": 18, - "base": "11911b5b0cf0abad7365d7a2a5ad0565d9e1c38a", + "wave": 26, + "base": "1b4abbb0020cce326ef56c7452de704af79513ad", "tasks": [ - "T040" + "T054" ], - "check": "scope pass (VERIFY); ruff and black pass; Done-when script ok (frame_payload(None,3) order and identity; payload_kind({},False) legacy_no_transform); worker scripts: payload_kind 8 branches, bounding_box_from_support round trip error 3.6e-15; persistence file still 166 failed 3 passed (loader and to_dict not done)", - "commit": "8c9b63592058748169aae8b6afd1a888cc0da63c", - "review": "pass: sonnet, 2 minor" + "check": "scope pass; ruff pass; fault_ellipse 4 passed (coverage); file collects without viewer", + "commit": "121b75c6a62c88188de06f6869cd132b35a712db", + "review": "pass: sonnet, 1 minor open" }, { - "wave": 19, - "base": "66f446abc0e8040a5a4133010fa95bfc7698bd20", + "wave": 27, + "base": "27cd35bfc8692315a79ad6a06f019de6fc721ff8", "tasks": [ - "T041", - "T042" + "T055", + "T056" ], - "check": "scope pass (VERIFY); ruff and black pass. T042: lifecycle -k stale/restored/rebuild/fault_restoration 13 passed (27 s); tests/unit/interpolator 346 passed (worker); package test_geological_interpolator.py 34 passed 6 failed = test_interpolator_json_yaml_round_trip until T043 (expected, documented). T041 done-when script ok. Persistence file 37 passed 132 failed (payload_declares 36 + old_reader 1 now pass; loader tests wait for T043)", - "commit": "5413ec2a83867003835ce00b1ca126d02c699358", - "review": "pass: sonnet, 3 minor" + "check": "scope pass; ruff pass; T056 25 failed (24 DID NOT WARN, 1 AttributeError); T052-T054 30 failed 5 passed", + "commit": "769562fe2ab0cb9caf32c3fc5c6653e5718d88cb", + "review": "pass: sonnet, 2 minor open" }, { - "wave": 20, - "base": "347ed03d2f458840d03f406c10ec621926bf2279", + "wave": 28, + "base": "d8ed7d3eba533567046ca057963c7d98bf65b399", "tasks": [ - "T043", - "T044", - "T045", - "T046" + "T057", + "T058", + "T059", + "T060", + "T061", + "T062" ], - "check": "scope pass (VERIFY); ruff pass. Root call: test_frame_persistence.py + test_interpolator_factory.py 181 passed; test_frame_copy_and_lifecycle.py 49 passed (27.7 s); package call: test_p2_fold_registration.py + test_geological_interpolator.py 43 passed; worker: tests/unit/interpolator 346 passed. The S3 persistence file passes 169 of 169. Root and package tests run in separate calls (conftest clash). Orchestrator reverted a Black blank line that T046 added outside copy; _p2interpolator.py stays Black-unclean as on master (not touched).", - "commit": "4e51f4cb85fa6fa07804ff1c23bc34d177cf295d", - "review": "pass: sonnet, 3 minor (1 fixed by the orchestrator: stray Black blank line)" + "check": "scope pass except approved T056 test fix; ruff pass; 62 passed (consumers + public api); test_exporters 12 passed 1 pre-existing fail (gocad)", + "commit": "7d5959e7f806ef8ff571406b0045be3c17301000", + "review": "pass: sonnet, no findings" }, { - "wave": 21, - "base": "185333acfe9a4d69edffafbec96915002bd02a68", + "wave": 29, + "base": "2e84bfcc443c6f60c9d2d55412f508ea0f0d227b", "tasks": [ - "T047", - "T048", - "T049" + "T063", + "T064", + "T065", + "T066" ], - "check": "scope pass (VERIFY); COMPAT row at the end of the Migration notices table (grep 1); ROADMAP decision under Decisions (grep 1); rollback checks: 9 of 9 legacy loads exact (errors 0.0), no-choice load raises legacy_no_transform, old reader raises TypeError naming format_version and returns no object; no throw-away worktree left", - "commit": "4cd0fd4fb5f935ceb517379776d3b1b8cdf00cda", + "check": "scope pass; greps ok; consumers 60 passed; intrusions 9; api contract 2; exporters 12 + 1 pre-existing fail", + "commit": "75c2cb3517b600036f847fbd7bf9af6e316ba583", "review": "pass: sonnet, no findings" }, { - "wave": 22, - "base": "67b1e48ab4fe344ef6b7f2f11b1b78af7658542f", + "wave": 30, + "base": "686322e9927b146c390947e562fe4388ab0549fa", "tasks": [ - "T050" + "T067" ], - "check": "Gates run by the orchestrator at 67b1e48a (code unchanged since wave 20): ruff pass; packages 912 passed 47 skipped 2 xfailed in 108.69 s; root 1978 passed 5 skipped 13 warnings in 797.81 s (= 1760 + 218 new S3 cells). Focused: test_frame_persistence.py 169 passed 5.2 s; test_frame_copy_and_lifecycle.py 49 passed 31.3 s. Frame check cost 6.0 us (4 percent) per evaluate_value call and 3.1 us (1 percent) per evaluate_gradient call on 100 points. Orchestrator fixed 3 facts in the T050 text (branch name, p2_fold lifecycle cell, time comparison).", - "gates": { - "lint": "pass", - "packages": "912 passed, 47 skipped, 2 xfailed", - "root": "1978 passed, 5 skipped" - }, - "commit": "db3e8a580934fe6077418ef1ce2602a023df9718", - "review": "pass: sonnet, 2 minor" + "check": "scope pass; (a)-(d) recorded; no rollback worktree left", + "commit": "0871a904d372076bb1e1ff5f62644598c44b8ba6", + "review": "pass: sonnet, no findings" } ], "runs": { - "T033": [ + "T051": [ { "model": "sonnet", - "result": "pass: r1_interpolator, strip_frame_keys, engine tuples, QUERY_OFFSETS" + "result": "pass: world_foliation_model added" } ], - "T034": [ + "T052": [ { - "model": "haiku", - "result": "pass: stubs and exports; import placed in sorted order (ruff I001), not directly after ._diagnostics" + "model": "sonnet", + "result": "pass: 19 cells, 18 fail (shift by local origin), identity cell passes" } ], - "T035": [ + "T053": [ { "model": "sonnet", - "result": "pass: 4 test functions, 111 cells, fail for the right reason" + "result": "pass: 12 cells fail with AttributeError" }, { "model": "sonnet", - "result": "fix pass (same fix): test_round_trip_world_values compares loaded with source" + "result": "review fix pass: test_intrusion_steps_parameters_evaluate_model_at_world_points covers the production call site; mutation check fails as expected" } ], - "T037": [ + "T054": [ { "model": "sonnet", - "result": "blocked: P2 fit misses SC-002 in every frame (value 1.8e-8, gradient 3.9e-7 at the unrotated control); decision OP7" + "result": "pass: 4 cells coverage" }, { "model": "sonnet", - "result": "fix pass: p2 rebuild cell uses exact=True; 5 rebuild cells pass" + "result": "fix pass: viewer import moved into the test with importorskip" } ], - "T036": [ + "T055": [ { "model": "sonnet", - "result": "pass with 2 test bugs found by the orchestrator" - }, - { - "model": "sonnet", - "result": "fix pass: loaded compared with the source (not the analytic field, which differs by 2e-10 at anchor 3 rot 30); old-reader test uses the real to_dict" + "result": "pass: S4 pre-fix section in verification.md" } ], - "T038": [ + "T056": [ { "model": "sonnet", - "result": "pass: test_fault_restoration_uses_current_frame (FDI, PLI)" - } - ], - "T039": [ + "result": "pass: 25 failed (24 DID NOT WARN, 1 AttributeError _network_model_values)" + }, { "model": "sonnet", - "result": "pass: S3 pre-fix section with 17-row table and 3 behavioural records" + "result": "fix pass: scale inplace assertion records the existing behaviour (copy; caller array unchanged)" } ], - "T040": [ + "T057": [ { "model": "sonnet", - "result": "pass: frame_payload, payload_kind, bounding_box_from_support; docstrings of T034 fixed" + "result": "pass: 19 cells" } ], - "T041": [ + "T058": [ { - "model": "sonnet", - "result": "pass: resolve_frame with _malformed/_parse_transform helpers; catches both LoopValueError classes of loop_common" + "model": "haiku", + "result": "pass: 12 cells" } ], - "T042": [ + "T059": [ { - "model": "sonnet", - "result": "pass: frame_signature, _note_frame, _check_frame, _inherit_frame, _restore_local_constraints, 7 setter guards, evaluate_* and to_dict guards, from_* keywords" + "model": "haiku", + "result": "fail: stacklevel=2 attributes the warning to the public_api wrapper; asked a question" + }, + { + "model": "haiku", + "result": "pass after stacklevel=3 (fix); scale inplace test cell failed (test error, see answers)" } ], - "T043": [ + "T060": [ { - "model": "sonnet", - "result": "pass: from_dict saved-payload path, frame args, malformed wrapping" + "model": "haiku", + "result": "pass: 4 rescale calls removed; 3 now-empty if real_coords blocks removed too; test_exporters ran only with pyevtk from a scratch --target dir" } ], - "T044": [ + "T061": [ { "model": "haiku", "result": "pass" } ], - "T045": [ + "T062": [ { "model": "haiku", "result": "pass" } ], - "T046": [ + "T063": [ { - "model": "haiku", - "result": "pass (orchestrator reverted a stray blank line from black)" + "model": "sonnet", + "result": "pass" } ], - "T047": [ + "T064": [ + { + "model": "sonnet", + "result": "pass; Active written in the Removed-in column like the existing rows" + } + ], + "T065": [ { "model": "haiku", "result": "pass" } ], - "T048": [ + "T066": [ { "model": "haiku", "result": "pass" } ], - "T049": [ + "T067": [ { "model": "sonnet", - "result": "pass: S3 rollback checks section; throw-away worktree removed" + "result": "pass; Loop3DView.add_data does not exist: checked Loop2DView.add_data (signature equal) and Loop3DView.add_mesh" } ], - "T050": [ + "T068": [ { "model": "sonnet", - "result": "pass: 4 S3 sections in verification.md and the ROADMAP status line; orchestrator ran the heavy gates (memory) and corrected 3 facts" + "result": "pass; gates run by the orchestrator (detached script); Done-when root pytest has 1 pre-existing failure (gocad, not S4)" } ] }, "findings": { - "found": 28, - "rejected": 1, + "found": 7, + "rejected": 0, "dropped": 0, - "fixed": 7, + "fixed": 1, "open_minor": [ - "T037 test_stale_frame_rejected_without_state_change: origin and rotation change together; a signature that ignores one of them still passes. Add origin-only and rotation-only cells (not in the plan; can be done in T042 or later).", - "T035 test_round_trip_world_values: compares the loaded values with the analytic field only; add a comparison with the source evaluation (optional).", - "T035 and T037 define F differently in the task text (query points in T035, observations in T037). The spec definition is the observations; T035 is stricter.", - "T036: no positive cases: v2 payload with a caller transform equal to the saved one must load; legacy payload with only local_origin or only local_rotation (other defaults to identity).", - "T036 test_creation_call_unchanged: also check local_origin/local_rotation arguments raise malformed, and that the input dict is unchanged.", - "T038: the R2 fixture fault displaces none of its probes (0 of 6859 moved); the test compares restored points (trivially equal) and the fault-frame coordinates. A fixture with real displacement would test restoration itself (S6).", - "T040 payload_kind: a payload that is not a dict raises AttributeError, not InterpolatorFormatError (the loader gates on a dict first; add a malformed check if a caller can reach it).", - "T040 bounding_box_from_support does not check shapes; T041 resolve_frame must (added to the T041 context).", - "T042 frame_signature compares raw bytes: -0.0 and +0.0 origins give different signatures, so an equal box with -0.0 raises StaleFrameError. Fix: add 0.0 to the arrays before tobytes().", - "T042 _restore_local_constraints is not atomic: a failure in a later family leaves the earlier family stored and _frame_at_data None. The loader drops the half-built interpolator, so no published state is affected (T043 wraps the error as malformed).", - "T042 _restore_local_constraints: ragged arrays give a raw numpy ValueError (wrapped by T043); unknown keys in data are ignored without a message.", - "T042 __init__ sets data without _note_frame: a non-empty initial data argument is not tracked (no known caller).", - "T043 from_dict saved path: a missing or unknown `type` raises a plain ValueError, not InterpolatorFormatError(\"malformed\") (nothing is returned, so no partial result).", - "T043 from_dict saved path does not validate the shape of `data` (a non-dict can raise TypeError) or `c` against the support, and up_to_date True with no c loads as solved without coefficients (same as the old creation path).", - "T041 equal-frame check uses atol=1e-12 with no relative part: at UTM coordinates (1 ULP about 9e-10) a caller origin that differs by 1 ULP gives conflict. Plan value kept; consider a scale-aware tolerance.", - "T037 test_rebuild_in_new_frame_preserves_world_evaluation does not change a frame; it is an SC-002 check at the fixture frame. verification.md maps SC-016 to it.", - "T041 BoundingBox.from_dict does not pass dimensions, so a 2D box in a v2 payload is malformed. Not reachable now (2D supports have no to_dict).", - "T050 ROADMAP.md S3 status line is one long line; wrap at about 80 columns.", - "T041 for a v2 payload with a rotated saved transform and only local_origin given, the missing rotation is compared as the identity, so the result is conflict. Unchanged v2 behaviour; documented in plan.md step 2.", - "Outside S3 (found by the end review in a scratch probe): model.evaluate_feature_gradient(\"strati\", pts) raised ValueError shape mismatch (20,3) vs (20,4) at LoopStructural/modelling/features/_geological_feature.py:229 in a small rotated model. Not in the S3 range; check separately.", - "plan.md \"Messages\" list shows only the base legacy_no_transform text; add the suffix ({name} is missing) for a partial legacy frame.", - "A pre-S3 pickled object that already holds constraints gets _frame_at_data None from the class default, so a later transform change is not detected until clean() or a new constraint set (same as before S3; optional: record the frame on the first _check_frame call; document in COMPAT.md).", - "C-018 review, minor: _external_stacklevel compares directory strings; a path spelling difference (symlink or drive case) could misattribute the warning. Tests pass on this machine." + "T054 test_2d_viewer_fault_ellipse_centre_is_world: compares the patch centre to fault.fault_centre[:2] only; add a check that fault_centre[:2] is close to anchor[:2] (optional).", + "T056 test_internal_callers_do_not_warn: importorskip(pyevtk) sits after the first checks; move it to the start or split the exporter part.", + "T056 failing ids before the fixes (25: 24 DID NOT WARN, 1 AttributeError _network_model_values) are not in verification.md; record them in T063.", + "T067/T064: the plan names Loop3DView.add_data, which does not exist (Loop2DView.add_data does). COMPAT.md S4 notice repeats the name; correct in a later pass.", + "T054 fault ellipse test: add check against anchor[:2] (end review, minor).", + "T056 test_internal_callers_do_not_warn: importorskip(pyevtk) mid-test hides the checks that ran; move the exporter part to its own test (end review, minor).", + "T068 verification.md S4 deprecated-call grep: line numbers of verification.md hits are stale (wave 31, minor)." ] }, "answers": [ - "2026-11-05: OP4 reject at use; OP5 accept TypeError naming format_version; major-version note in COMPAT/ROADMAP (plan.md Answers)", - "2026-11-05 (wave 15, OP7): p2 rebuild cell uses the exact nodal control; P2 fit accuracy is open point OP7 (plan.md)", - "2026-11-05 (wave 16 recheck, user decision): fix the conflict-test wording branch and verify it by the orchestrator, no third review round. plan.md conflict message rule reworded.", - "2026-11-05 (wave 19 planning, orchestrator): T042 Done-when excepts test_interpolator_json_yaml_round_trip (loader is T043); the intermediate commit after wave 19 leaves 1 to 3 existing round-trip tests failing until wave 20." + "2026-11-05 (wave 28, orchestrator): (1) T059 stacklevel is 3, not 2, because @public_api adds one frame; the test requires the warning to name the caller file. (2) GeologicalModel.scale has always copied its input (np.array(points).astype(float)), so inplace=True never changed the caller array; FR-033 says keep behaviour, so the T056 scale test now checks the existing behaviour (result equals project(points); caller array unchanged) instead of mutation. (3) pyevtk is not in the venv: tests/unit/io/test_exporters.py and the exporter part of test_internal_callers_do_not_warn skip without it. pyevtk 1.7.0 was installed with pip --target into the scratch dir and put on PYTHONPATH for the checks; the venv is unchanged. (4) test_write_vol_gocad_is_broken_for_real_bounding_box fails before and after S4 (pre-existing: BoundingBox.__getitem__ raises LoopValueError for a slice; the test expects pytest.raises(LoopValueError) but the failure is at the base too) - to check." ], "history": { "S1": { @@ -889,42 +853,345 @@ "head": "1a67ba7f", "status": "S2 implemented; PR #396 open; end spec review pass (opus); gates at 662d2713: ruff pass, packages 912 passed 47 skipped 2 xfailed, root 1760 passed 5 skipped" } + }, + "S3": { + "spec": "005-coordinate-frame-correctness", + "stage": "S3", + "worktree": "C:/Users/lachl/dev/LoopStructural-005-s3", + "branch": "feat/005-s3-frame-persistence", + "review_mode": "wave", + "start_commit": "91ffca4a269252c32cefe077261edd27f10a4b14", + "runtime": { + "runtime": "claude", + "max_workers": 5, + "worker_S": "haiku", + "worker_M": "sonnet", + "review_wave": "sonnet", + "review_spec": "opus", + "review_verifier": "opus" + }, + "env": "PYTHONPATH=;/packages/loop_common/src;/packages/loop_interpolation/src; python = main checkout .venv (C:/Users/lachl/dev/LoopStructural/.venv/Scripts/python.exe); run the root pytest and the package tests in separate calls, one heavy run at a time (low memory)", + "baseline": { + "lint": "pass: ruff check LoopStructural packages (S3 worktree, start commit)", + "pytest": "1760 passed, 5 skipped, 13 warnings in 1052.40 s at 662d2713 (S2 code; commits after it change only specs/README/docs). Not run again at S3 start because of low memory.", + "packages": "912 passed, 47 skipped, 2 xfailed in 126.56 s at 662d2713" + }, + "base_note": "Built from docs/005-s3-plan (S2 code from feat/005-s2-fold-frames, PR #396 not merged). Rebase onto master after #396 merges.", + "waves": [ + { + "wave": 14, + "base": "b32707fd62957ccb5d4c606c2bbe5bff22a6b7a9", + "tasks": [ + "T033", + "T034" + ], + "check": "scope pass (VERIFY); ruff pass; black pass; fixture collects 0 tests; 5 engines build (exact: errors <= 2e-10 at anchor 3 rot 30; cn fit errors 4e-9 and 1.3e-11 at anchor 0); imports from the S3 worktree", + "commit": "c7a6802dd750bcfb0417828fe9b37a1269605f70", + "review": "pass: sonnet, no findings" + }, + { + "wave": 15, + "base": "5cd6013450bf1aecc419f29cf0443fa879b71e20", + "tasks": [ + "T035", + "T037" + ], + "check": "scope pass (VERIFY); ruff and black pass. Pre-fix: test_frame_persistence.py 108 failed 3 passed (identity cells); test_frame_copy_and_lifecycle.py 41 failed 6 passed (copy 36/36, stale 5/5 fail; restored 1 and rebuild 5 pass = coverage, no defect). First errors: KeyError format_version/transform; nan after rotated round trip; copy().bounding_box is None; DID NOT RAISE StaleFrameError", + "commit": "87928866225cc1f56b1634d791fa75ceec2b72da", + "review": "pass after rejecting 1 major: sonnet, 1 major rejected (F in spec SC definition is the max abs reference scalar value = observations; S2 test_constant_norm_frames uses the same scale), 2 minor open" + }, + { + "wave": 16, + "base": "acef571f5ed93c72ccf7b00b4161865e348213e7", + "tasks": [ + "T036", + "T038" + ], + "check": "scope pass (VERIFY); ruff and black pass. Pre-fix: legacy/conflicting/malformed/unsupported/creation_call/old_reader 58 failed (TypeError unexpected keyword declare_identity or format_version; DID NOT RAISE); T035 file 166 failed 3 passed (identity cells); fault_restoration 2 failed (DID NOT RAISE StaleFrameError). apply_to_points reaches the interpolators through evaluate_value/evaluate_gradient of the frame features. Later part of the fault test (rebuild vs control) ran clean with the raise check off (FDI 18.6 s, PLI 4.2 s)", + "commit": "d3813ce2a22be699a3ef768a904b25a9dc19c883", + "review": "pass after fixes: sonnet first review 1 major + 1 minor fixed (2a4a6636); recheck found 1 new major (T036 conflict test demanded the word supplied for declare_identity against a saved transform; my plan wording was ambiguous). User decision 2026-11-05: fix it and verify without a third review round.", + "fix_commit": "2a4a66368d6afee03f639bc73a06ddb8caeaa493" + }, + { + "wave": 17, + "base": "caf0a0e73e568355dbad84c11ec7e7b207e7788f", + "tasks": [ + "T039" + ], + "check": "scope pass (VERIFY: only verification.md). Pre-fix totals 209 failed 9 passed over 218 cells (persistence 166/3, lifecycle 43/6). Behavioural: rotated fdi round trip gives NaN (2 of 2); copy().bounding_box is None = True; stale evaluation after set_local_transform returns NaN with no error", + "commit": "b139482282c2c49755115e145db8a8519fefd439", + "review": "pass: sonnet, no findings" + }, + { + "wave": 18, + "base": "11911b5b0cf0abad7365d7a2a5ad0565d9e1c38a", + "tasks": [ + "T040" + ], + "check": "scope pass (VERIFY); ruff and black pass; Done-when script ok (frame_payload(None,3) order and identity; payload_kind({},False) legacy_no_transform); worker scripts: payload_kind 8 branches, bounding_box_from_support round trip error 3.6e-15; persistence file still 166 failed 3 passed (loader and to_dict not done)", + "commit": "8c9b63592058748169aae8b6afd1a888cc0da63c", + "review": "pass: sonnet, 2 minor" + }, + { + "wave": 19, + "base": "66f446abc0e8040a5a4133010fa95bfc7698bd20", + "tasks": [ + "T041", + "T042" + ], + "check": "scope pass (VERIFY); ruff and black pass. T042: lifecycle -k stale/restored/rebuild/fault_restoration 13 passed (27 s); tests/unit/interpolator 346 passed (worker); package test_geological_interpolator.py 34 passed 6 failed = test_interpolator_json_yaml_round_trip until T043 (expected, documented). T041 done-when script ok. Persistence file 37 passed 132 failed (payload_declares 36 + old_reader 1 now pass; loader tests wait for T043)", + "commit": "5413ec2a83867003835ce00b1ca126d02c699358", + "review": "pass: sonnet, 3 minor" + }, + { + "wave": 20, + "base": "347ed03d2f458840d03f406c10ec621926bf2279", + "tasks": [ + "T043", + "T044", + "T045", + "T046" + ], + "check": "scope pass (VERIFY); ruff pass. Root call: test_frame_persistence.py + test_interpolator_factory.py 181 passed; test_frame_copy_and_lifecycle.py 49 passed (27.7 s); package call: test_p2_fold_registration.py + test_geological_interpolator.py 43 passed; worker: tests/unit/interpolator 346 passed. The S3 persistence file passes 169 of 169. Root and package tests run in separate calls (conftest clash). Orchestrator reverted a Black blank line that T046 added outside copy; _p2interpolator.py stays Black-unclean as on master (not touched).", + "commit": "4e51f4cb85fa6fa07804ff1c23bc34d177cf295d", + "review": "pass: sonnet, 3 minor (1 fixed by the orchestrator: stray Black blank line)" + }, + { + "wave": 21, + "base": "185333acfe9a4d69edffafbec96915002bd02a68", + "tasks": [ + "T047", + "T048", + "T049" + ], + "check": "scope pass (VERIFY); COMPAT row at the end of the Migration notices table (grep 1); ROADMAP decision under Decisions (grep 1); rollback checks: 9 of 9 legacy loads exact (errors 0.0), no-choice load raises legacy_no_transform, old reader raises TypeError naming format_version and returns no object; no throw-away worktree left", + "commit": "4cd0fd4fb5f935ceb517379776d3b1b8cdf00cda", + "review": "pass: sonnet, no findings" + }, + { + "wave": 22, + "base": "67b1e48ab4fe344ef6b7f2f11b1b78af7658542f", + "tasks": [ + "T050" + ], + "check": "Gates run by the orchestrator at 67b1e48a (code unchanged since wave 20): ruff pass; packages 912 passed 47 skipped 2 xfailed in 108.69 s; root 1978 passed 5 skipped 13 warnings in 797.81 s (= 1760 + 218 new S3 cells). Focused: test_frame_persistence.py 169 passed 5.2 s; test_frame_copy_and_lifecycle.py 49 passed 31.3 s. Frame check cost 6.0 us (4 percent) per evaluate_value call and 3.1 us (1 percent) per evaluate_gradient call on 100 points. Orchestrator fixed 3 facts in the T050 text (branch name, p2_fold lifecycle cell, time comparison).", + "gates": { + "lint": "pass", + "packages": "912 passed, 47 skipped, 2 xfailed", + "root": "1978 passed, 5 skipped" + }, + "commit": "db3e8a580934fe6077418ef1ce2602a023df9718", + "review": "pass: sonnet, 2 minor" + } + ], + "runs": { + "T033": [ + { + "model": "sonnet", + "result": "pass: r1_interpolator, strip_frame_keys, engine tuples, QUERY_OFFSETS" + } + ], + "T034": [ + { + "model": "haiku", + "result": "pass: stubs and exports; import placed in sorted order (ruff I001), not directly after ._diagnostics" + } + ], + "T035": [ + { + "model": "sonnet", + "result": "pass: 4 test functions, 111 cells, fail for the right reason" + }, + { + "model": "sonnet", + "result": "fix pass (same fix): test_round_trip_world_values compares loaded with source" + } + ], + "T037": [ + { + "model": "sonnet", + "result": "blocked: P2 fit misses SC-002 in every frame (value 1.8e-8, gradient 3.9e-7 at the unrotated control); decision OP7" + }, + { + "model": "sonnet", + "result": "fix pass: p2 rebuild cell uses exact=True; 5 rebuild cells pass" + } + ], + "T036": [ + { + "model": "sonnet", + "result": "pass with 2 test bugs found by the orchestrator" + }, + { + "model": "sonnet", + "result": "fix pass: loaded compared with the source (not the analytic field, which differs by 2e-10 at anchor 3 rot 30); old-reader test uses the real to_dict" + } + ], + "T038": [ + { + "model": "sonnet", + "result": "pass: test_fault_restoration_uses_current_frame (FDI, PLI)" + } + ], + "T039": [ + { + "model": "sonnet", + "result": "pass: S3 pre-fix section with 17-row table and 3 behavioural records" + } + ], + "T040": [ + { + "model": "sonnet", + "result": "pass: frame_payload, payload_kind, bounding_box_from_support; docstrings of T034 fixed" + } + ], + "T041": [ + { + "model": "sonnet", + "result": "pass: resolve_frame with _malformed/_parse_transform helpers; catches both LoopValueError classes of loop_common" + } + ], + "T042": [ + { + "model": "sonnet", + "result": "pass: frame_signature, _note_frame, _check_frame, _inherit_frame, _restore_local_constraints, 7 setter guards, evaluate_* and to_dict guards, from_* keywords" + } + ], + "T043": [ + { + "model": "sonnet", + "result": "pass: from_dict saved-payload path, frame args, malformed wrapping" + } + ], + "T044": [ + { + "model": "haiku", + "result": "pass" + } + ], + "T045": [ + { + "model": "haiku", + "result": "pass" + } + ], + "T046": [ + { + "model": "haiku", + "result": "pass (orchestrator reverted a stray blank line from black)" + } + ], + "T047": [ + { + "model": "haiku", + "result": "pass" + } + ], + "T048": [ + { + "model": "haiku", + "result": "pass" + } + ], + "T049": [ + { + "model": "sonnet", + "result": "pass: S3 rollback checks section; throw-away worktree removed" + } + ], + "T050": [ + { + "model": "sonnet", + "result": "pass: 4 S3 sections in verification.md and the ROADMAP status line; orchestrator ran the heavy gates (memory) and corrected 3 facts" + } + ] + }, + "findings": { + "found": 28, + "rejected": 1, + "dropped": 0, + "fixed": 7, + "open_minor": [ + "T037 test_stale_frame_rejected_without_state_change: origin and rotation change together; a signature that ignores one of them still passes. Add origin-only and rotation-only cells (not in the plan; can be done in T042 or later).", + "T035 test_round_trip_world_values: compares the loaded values with the analytic field only; add a comparison with the source evaluation (optional).", + "T035 and T037 define F differently in the task text (query points in T035, observations in T037). The spec definition is the observations; T035 is stricter.", + "T036: no positive cases: v2 payload with a caller transform equal to the saved one must load; legacy payload with only local_origin or only local_rotation (other defaults to identity).", + "T036 test_creation_call_unchanged: also check local_origin/local_rotation arguments raise malformed, and that the input dict is unchanged.", + "T038: the R2 fixture fault displaces none of its probes (0 of 6859 moved); the test compares restored points (trivially equal) and the fault-frame coordinates. A fixture with real displacement would test restoration itself (S6).", + "T040 payload_kind: a payload that is not a dict raises AttributeError, not InterpolatorFormatError (the loader gates on a dict first; add a malformed check if a caller can reach it).", + "T040 bounding_box_from_support does not check shapes; T041 resolve_frame must (added to the T041 context).", + "T042 frame_signature compares raw bytes: -0.0 and +0.0 origins give different signatures, so an equal box with -0.0 raises StaleFrameError. Fix: add 0.0 to the arrays before tobytes().", + "T042 _restore_local_constraints is not atomic: a failure in a later family leaves the earlier family stored and _frame_at_data None. The loader drops the half-built interpolator, so no published state is affected (T043 wraps the error as malformed).", + "T042 _restore_local_constraints: ragged arrays give a raw numpy ValueError (wrapped by T043); unknown keys in data are ignored without a message.", + "T042 __init__ sets data without _note_frame: a non-empty initial data argument is not tracked (no known caller).", + "T043 from_dict saved path: a missing or unknown `type` raises a plain ValueError, not InterpolatorFormatError(\"malformed\") (nothing is returned, so no partial result).", + "T043 from_dict saved path does not validate the shape of `data` (a non-dict can raise TypeError) or `c` against the support, and up_to_date True with no c loads as solved without coefficients (same as the old creation path).", + "T041 equal-frame check uses atol=1e-12 with no relative part: at UTM coordinates (1 ULP about 9e-10) a caller origin that differs by 1 ULP gives conflict. Plan value kept; consider a scale-aware tolerance.", + "T037 test_rebuild_in_new_frame_preserves_world_evaluation does not change a frame; it is an SC-002 check at the fixture frame. verification.md maps SC-016 to it.", + "T041 BoundingBox.from_dict does not pass dimensions, so a 2D box in a v2 payload is malformed. Not reachable now (2D supports have no to_dict).", + "T050 ROADMAP.md S3 status line is one long line; wrap at about 80 columns.", + "T041 for a v2 payload with a rotated saved transform and only local_origin given, the missing rotation is compared as the identity, so the result is conflict. Unchanged v2 behaviour; documented in plan.md step 2.", + "Outside S3 (found by the end review in a scratch probe): model.evaluate_feature_gradient(\"strati\", pts) raised ValueError shape mismatch (20,3) vs (20,4) at LoopStructural/modelling/features/_geological_feature.py:229 in a small rotated model. Not in the S3 range; check separately.", + "plan.md \"Messages\" list shows only the base legacy_no_transform text; add the suffix ({name} is missing) for a partial legacy frame.", + "A pre-S3 pickled object that already holds constraints gets _frame_at_data None from the class default, so a later transform change is not detected until clean() or a new constraint set (same as before S3; optional: record the frame on the first _check_frame call; document in COMPAT.md).", + "C-018 review, minor: _external_stacklevel compares directory strings; a path spelling difference (symlink or drive case) could misattribute the warning. Tests pass on this machine." + ] + }, + "answers": [ + "2026-11-05: OP4 reject at use; OP5 accept TypeError naming format_version; major-version note in COMPAT/ROADMAP (plan.md Answers)", + "2026-11-05 (wave 15, OP7): p2 rebuild cell uses the exact nodal control; P2 fit accuracy is open point OP7 (plan.md)", + "2026-11-05 (wave 16 recheck, user decision): fix the conflict-test wording branch and verify it by the orchestrator, no third review round. plan.md conflict message rule reworded.", + "2026-11-05 (wave 19 planning, orchestrator): T042 Done-when excepts test_interpolator_json_yaml_round_trip (loader is T043); the intermediate commit after wave 19 leaves 1 to 3 existing round-trip tests failing until wave 20." + ], + "status": "S3 implemented and gated; end review pass after one fix round", + "spec_review": { + "stage": "S3", + "model": "opus", + "range": "91ffca4a..db3e8a58", + "verdict": "changes needed", + "major": 2, + "minor": 3, + "fixed": [ + "major: pre-S3 pickled interpolators had no _frame_at_data: class-level default added (T042 file) + test_pre_s3_object_state_still_evaluates", + "major: a legacy payload with only local_origin or only local_rotation loaded silently with identity defaults: now legacy_no_transform naming the missing argument (user decision 2026-11-05); plan.md step 2 changed; 2 negative tests" + ], + "recheck": "pass: opus, 2 minor (fix commit c9ca3260)", + "fix_commit": "c9ca3260a29b01ec4906497b8e2f2955016f2867" + }, + "final": { + "head_gated": "e49f3b2a", + "lint": "ruff check LoopStructural packages: pass", + "format": "black --check on the 10 changed .py files: new module and the 3 test files clean; 4 library files dirty and already dirty at the S3 start (_finite_difference_interpolator, _interpolator_factory, _p2_fold_interpolator, _p2interpolator); only task hunks changed", + "test_packages": "912 passed, 47 skipped, 2 xfailed, 7 warnings in 103.85 s (after C-018 and its review fix)", + "test_root": "1994 passed, 5 skipped, 13 warnings in 817.74 s (after C-018 and its review fix)", + "status": "S3 implemented: 50 of 50 tasks done; not pushed; no pull request" + }, + "post_review_changes": [ + { + "date": "2026-11-05", + "id": "C-018", + "what": "Backward-compatible legacy load: a legacy payload with no choice loads with the identity frame and emits one FutureWarning; _STRICT_LEGACY switch with a remove-at-major note; partial transform, conflict, malformed and unsupported still raise. Docs (ROADMAP, COMPAT, verification) updated; P2 figures corrected.", + "gates": { + "lint": "pass", + "packages": "912 passed, 47 skipped, 2 xfailed", + "root": "1994 passed, 5 skipped" + }, + "review": "first review: 1 major (warning location and timing) + 2 minor; fixed with warn_legacy_identity() and 7 new test cells (commit 169ae619); fix checked by the orchestrator, no second reviewer round" + } + ] } }, - "status": "S3 implemented and gated; end review pass after one fix round", - "spec_review": { - "stage": "S3", - "model": "opus", - "range": "91ffca4a..db3e8a58", - "verdict": "changes needed", - "major": 2, - "minor": 3, - "fixed": [ - "major: pre-S3 pickled interpolators had no _frame_at_data: class-level default added (T042 file) + test_pre_s3_object_state_still_evaluates", - "major: a legacy payload with only local_origin or only local_rotation loaded silently with identity defaults: now legacy_no_transform naming the missing argument (user decision 2026-11-05); plan.md step 2 changed; 2 negative tests" - ], - "recheck": "pass: opus, 2 minor (fix commit c9ca3260)", - "fix_commit": "c9ca3260a29b01ec4906497b8e2f2955016f2867" - }, "final": { - "head_gated": "e49f3b2a", + "head_gated": "1ba71f17", "lint": "ruff check LoopStructural packages: pass", - "format": "black --check on the 10 changed .py files: new module and the 3 test files clean; 4 library files dirty and already dirty at the S3 start (_finite_difference_interpolator, _interpolator_factory, _p2_fold_interpolator, _p2interpolator); only task hunks changed", - "test_packages": "912 passed, 47 skipped, 2 xfailed, 7 warnings in 103.85 s (after C-018 and its review fix)", - "test_root": "1994 passed, 5 skipped, 13 warnings in 817.74 s (after C-018 and its review fix)", - "status": "S3 implemented: 50 of 50 tasks done; not pushed; no pull request" + "test_packages": "912 passed, 47 skipped, 2 xfailed in 150.24 s", + "test_root": "1 failed, 2066 passed, 4 skipped in 805.50 s; the failure test_write_vol_gocad_is_broken_for_real_bounding_box is pre-existing (confirmed with the S4 changes stashed; two LoopValueError classes); it was skipped before because pyevtk is missing from the venv (pyevtk from a scratch dir on PYTHONPATH)", + "format": "black --check: 3 library files dirty before S4 (exporters.py, intrusion_frame_builder.py, geological_model.py); only task hunks changed" }, - "post_review_changes": [ - { - "date": "2026-11-05", - "id": "C-018", - "what": "Backward-compatible legacy load: a legacy payload with no choice loads with the identity frame and emits one FutureWarning; _STRICT_LEGACY switch with a remove-at-major note; partial transform, conflict, malformed and unsupported still raise. Docs (ROADMAP, COMPAT, verification) updated; P2 figures corrected.", - "gates": { - "lint": "pass", - "packages": "912 passed, 47 skipped, 2 xfailed", - "root": "1994 passed, 5 skipped" - }, - "review": "first review: 1 major (warning location and timing) + 2 minor; fixed with warn_legacy_identity() and 7 new test cells (commit 169ae619); fix checked by the orchestrator, no second reviewer round" - } - ] -} + "spec_review": { + "range": "9b3507a9..e0b52cc5", + "reviewer": "opus", + "verdict": "changes needed: 1 major (call site of _network_model_values not covered), 2 minor", + "fix": "1 major fixed with a new test (mutation proved)" + } +} \ No newline at end of file diff --git a/specs/005-coordinate-frame-correctness/spec.md b/specs/005-coordinate-frame-correctness/spec.md index 40bbe4adf..7892f8edb 100644 --- a/specs/005-coordinate-frame-correctness/spec.md +++ b/specs/005-coordinate-frame-correctness/spec.md @@ -1,6 +1,6 @@ # Feature spec: Coordinate and fault-frame correctness -**Feature**: `005-coordinate-frame-correctness` | **Created**: 2026-11-03 | **Status**: Stage S1 implemented (pull request #392); stages S2 to S6 need planning +**Feature**: `005-coordinate-frame-correctness` | **Created**: 2026-11-03 | **Status**: S1 merged (pull request #392); S2 in #396; S3 in #397; S4 planned; S5 and S6 need planning **Input**: Fix the shared world-to-local coordinate defects and the fault-frame interpolation defects found in the two investigations. @@ -96,11 +96,12 @@ A modeller saves an interpolator, reloads it, copies its configuration, or displ 3. **Given** an API that returns an empty copy, **when** the copy is created, **then** it retains the coordinate frame. It need not contain the source observations or coefficients. 4. **Given** an observation already stored in world coordinates, **when** it is displayed or exported as observation geometry, **then** its physical location is unchanged. 5. **Given** a world-coordinate fault centre or intrusion observation, **when** a downstream consumer uses it, **then** no extra inverse projection changes its location. -6. **Given** a legacy file with no recoverable transform or explicit caller declaration, **when** loading is requested, **then** loading fails. The error identifies the missing transform and the required caller action. No partial field is published. -7. **Given** that legacy file and a caller-supplied transform or explicit identity declaration, **when** loading is retried, **then** its world evaluations satisfy SC-006. The loader does not silently guess identity. +6. **Given** a legacy file with no recoverable transform or explicit caller declaration, **when** loading is requested on the 1.x line, **then** the file loads with the identity frame that older releases assumed, and one warning that end users see by default names the choices and says that the next major release raises an error. From the next major release, loading fails instead. The error identifies the missing transform and the required caller action. No partial field is published. +7. **Given** that legacy file and a caller-supplied transform or explicit identity declaration, **when** loading is retried, **then** its world evaluations satisfy SC-006. The loader does not guess identity without a warning. 8. **Given** a newly saved interpolator, **when** its payload is inspected, **then** it declares its format version, transform, and constraint coordinate space. These declarations survive 2 save/load cycles. -9. **Given** a populated interpolator, **when** its transform is changed, **then** the documented FR-020 policy applies. Rejection preserves prior state. Acceptance preserves world evaluation after rebuilding. Existing restoration caches are populated before a separate check for stale use. +9. **Given** a populated interpolator, **when** its transform is changed, **then** the documented FR-020 policy applies. Rejection preserves prior state; so does the rejection of each later use of the stale state. Acceptance preserves world evaluation after rebuilding. Restoration caches that exist (spec 003 adds them) are populated before a separate check for stale use. 10. **Given** a completed stage and its documented rollback target, **when** the stage is reversed, **then** old-format R1 fields remain usable. An unsupported new-format field fails without changing valid state. +11. **Given** a stable method that this work deprecates, **when** a caller uses it, **then** it returns the same result as before and emits one `DeprecationWarning` that names its replacement. No call inside the library emits that warning. ### User story 4: Trust support geometry and diagnostics (priority: P2) @@ -148,7 +149,7 @@ A developer compares frame interpolation, displacement, and coordinate utilities - Explicit and inferred fault centres; nonorthogonal input fault directions; curved observations; conflicting extent observations. - Buffers of 0, 0.2, and 0.5; forced mesh geometry; constraints on and outside support boundaries. - A transform changed after data, coefficients, or restored-point caches already exist. -- Legacy files without transform metadata fail until the caller supplies a transform or explicitly declares identity. Recoverable metadata is used without guessing. +- Legacy files without transform metadata load with the identity frame and one warning on the 1.x line, unless the caller supplies a transform or explicitly declares identity. From the next major release they fail until the caller does so. Recoverable metadata is used without guessing. - Malformed metadata and unsupported future format versions fail without publishing a partial field or changing prior valid state. - Conflicting caller transform information and saved metadata fail with an error naming the conflict. An explicit identity declaration is not an implicit default. - A diagnostic with no valid vectors; a constant scalar field; residual normalisation with zero field range. @@ -176,9 +177,9 @@ A developer compares frame interpolation, displacement, and coordinate utilities - **FR-015**: Cell-centre sampling MUST agree with the world geometry and cell association of the returned grid. No centre may lie beyond its associated cell because of mixed bases or endpoint spacing. - **FR-016**: Interpolator persistence MUST retain its transform and the declared coordinate space of saved constraints. A format version MUST identify the new metadata. - **FR-017**: Loading saved local arrays MUST NOT apply the public world-input conversion to them a second time. -- **FR-018**: APIs that copy an interpolator configuration MUST retain its coordinate frame, including APIs whose documented result is empty. -- **FR-019**: A legacy file with no recoverable transform MUST require a caller-supplied transform or an explicit identity declaration. Missing, malformed, unsupported, or conflicting transform information MUST fail with an actionable error before a partial field is published. The loader MUST NOT silently assume identity. -- **FR-020**: Changes to a transform MUST NOT leave dependent constraints, coefficients, or caches reusable in their previous coordinate frame. Rebuilding or rejecting the change before mutation are acceptable documented policies. +- **FR-018**: APIs that copy an interpolator configuration MUST retain its coordinate frame, including APIs whose documented result is empty. An API that has no such copy (the piecewise linear interpolator has no `copy`) stays unsupported and keeps its documented rejection. +- **FR-019**: A legacy file with no recoverable transform MUST require a caller-supplied transform (the local origin and the local rotation, both) or an explicit identity declaration. A partial transform is not enough. Missing, malformed, unsupported, or conflicting transform information MUST fail with an actionable error before a partial field is published. The loader MUST NOT silently assume identity. On the 1.x line, a legacy file with no choice at all is the one exception to the error: it loads with the identity frame that older releases assumed, and it emits one `FutureWarning` that names the choices and says that the next major release raises an error. The code that allows this MUST carry a note to remove it at the next major release (C-018). +- **FR-020**: Changes to a transform MUST NOT leave dependent constraints, coefficients, or caches reusable in their previous coordinate frame. Rebuilding the dependent state, rejecting the change before mutation, or rejecting every later use of the stale state are acceptable documented policies. In each policy the stored constraints, coefficients, and caches stay unchanged until the caller rebuilds them. - **FR-021**: A matrix accepted as a rotation MUST be finite, have the correct dimensions, and represent a proper rigid rotation. Scale and shear MUST NOT silently use rigid gradient-transformation rules. - **FR-022**: Displayed observations, fault-centre geometry, and intrusion classification MUST use their stored world locations without an extra inverse projection. - **FR-023**: Gradient and value diagnostics MUST compare quantities in one coordinate basis and MUST NOT mutate the state they inspect. @@ -186,11 +187,12 @@ A developer compares frame interpolation, displacement, and coordinate utilities - **FR-025**: The audited auxiliary point-transformation utilities MUST support forward/inverse round trips for their documented point-cloud shapes. Duplicate public entry points MUST retain their compatible import paths. - **FR-026**: Fault-frame validation MUST check scalar values, gradient magnitudes, gradient directions, constraint residuals, and displacement against analytic references. - **FR-027**: Integration error, fault discontinuities, and outside-support behaviour MUST be assessed separately from smooth frame-interpolation error. Existing zero-gradient protection MUST remain effective. -- **FR-028**: The public model API, return shapes, supported import paths, and inputs with known coordinate meaning MUST be preserved. The approved exception for ambiguous legacy files is FR-019 and requires a migration note. Any required public API breaking change MUST follow the constitution and roadmap decision process. +- **FR-028**: The public model API, return shapes, supported import paths, and inputs with known coordinate meaning MUST be preserved. On the 1.x line, a legacy file keeps its old load path (FR-019, C-018), so no stage breaks the public API there. The change of that path to an error happens at the next major release, which the maintainer plans. `ROADMAP.md` and `COMPAT.md` record the plan and the removal of the fallback. Any required public API breaking change MUST follow the constitution and roadmap decision process. - **FR-029**: Analytic evidence MAY justify an explicitly reviewed correction to an affected golden result. Each change MUST identify the defect and its expected physical effect. Unaffected baselines and acceptance tolerances MUST remain unchanged. Tests MUST NOT preserve a demonstrated defect merely because an old result contains it. - **FR-030**: Each required backend and capability in the coverage matrix MUST satisfy the coordinate contract through its integration boundary. Optional exclusions MUST identify the unavailable dependency or unsupported capability. A new native, browser, or RBF solver MUST NOT become a prerequisite for existing Python fixes. - **FR-031**: Production-path regression tests MUST reproduce each defect before its fix and verify it after the fix. Replacement investigation samplers MUST NOT be the acceptance path. - **FR-032**: The work MUST ship in independently testable stages. Each stage MUST have a documented rollback and MUST preserve compatibility entry points. +- **FR-033**: A stable public method that this work stops using inside the library MAY be deprecated if its behaviour and signature stay unchanged. A deprecation MUST emit a runtime `DeprecationWarning` that names the replacement. It MUST be logged in `COMPAT.md` with its removal rule. No call inside the library MAY trigger it. The first deprecations are `GeologicalModel.rescale` (local to world; the replacement is the bounding-box reprojection) and `GeologicalModel.scale` (world to local; the replacement is the bounding-box projection). Both go in the same release, with one lifecycle. ### Ownership and boundaries @@ -324,7 +326,7 @@ R4 checks geometric compatibility and sampling. It does not require an exact non | Backend or capability | Required fixtures and operations | Gate | |---|---|---| | Python FDI and PLI/P1 | R1 evaluation and fitting; R2 fault frame and displacement; R3 relationships; R5 bounds; supported persistence and copies; transform lifecycle | Mandatory for both engines | -| Python P2 | R1 evaluation and supported fitting; R3 reference and row assembly; bounds, persistence, and documented copy operations; transform lifecycle | Mandatory | +| Python P2 | R1 evaluation with the exact nodal field (C-017); R3 reference and row assembly; bounds, persistence, and documented copy operations; transform lifecycle | Mandatory | | Python FDI-fold, PLI discrete-fold, and P2-fold | R3 point and direction sampling, masks, sign alignment, nonzero targets, and each supported regularisation family; R4 geometry | Mandatory for every existing family; preserve documented unsupported cases | | Python FDI and PLI constant-norm variants | R1 with its analytic norm target; world queries and gradients; transform lifecycle; inherited affected relationships | Mandatory | | Common 2D and 3D geometry | Point and direction round trips, corner enclosures, resize stability, cell centres, and auxiliary transformations | Mandatory | @@ -334,7 +336,7 @@ R4 checks geometric compatibility and sampling. It does not require an exact non An unsupported capability must retain its documented rejection. Missing optional dependencies must not skip mandatory Python checks. A future native or RBF engine is not a prerequisite. - **SC-001**: Rigid point round trips differ by at most `Tp` for 2 and 3 dimensions. Direction round trips have maximum component error at most `1e-12` for unit inputs. -- **SC-002**: R1 value error is at most `1e-8*F`. Its gradient component error is at most `1e-8*max(1,|gradient|)`. The same limits apply after translation and computational or physical rotation. +- **SC-002**: R1 value error is at most `1e-8*F`. Its gradient component error is at most `1e-8*max(1,|gradient|)`. The same limits apply after translation and computational or physical rotation. For P2, these limits apply to the exact nodal field of R1 (C-017). - **SC-003**: R2 has maximum scalar-coordinate error at most `1e-6`, relative gradient-magnitude error at most `1e-4`, and maximum direction error at most 0.01 degree. Sample its 6859 specified probes with both FDI and PLI. Use CG relative tolerance `1e-10`, absolute tolerance `1e-12`, and at most 20000 iterations. Explicit and inferred centres satisfy the same limits. - **SC-004**: On R2's 1332 off-fault section points, each displacement comparison differs from its analytic-frame result by at most 0.03 world-length units. This is 1 percent of slip 3. Use the same displacement algorithm for both frames. Count fault-plane and invalid-support samples separately. - **SC-005**: Run R5 for 3 buffers, 3 rotations, and explicit and inferred centres with both FDI and PLI. All requested corners and generated observations are enclosed within `Tp`. Each face differs from its expected bound by at most 1 cell plus `Tp`, outward only. All 3 buffer requests produce distinct bounds. Zero valid observations are lost because of a space mismatch. @@ -344,11 +346,11 @@ An unsupported capability must retain its documented rejection. Missing optional - **SC-009**: Diagnostics cause 0 changes to stored positions, vectors, weights, coefficients, and cache-validity state. A matching analytic unit direction gives cosine agreement within `1e-10`. - **SC-010**: Test each documented auxiliary forward/inverse transformation with 1 point and point clouds of 4 and 50 points. Maximum round-trip position error is at most `Tp`. Untransformed columns remain exactly unchanged. - **SC-011**: Reapplying unchanged support bounds 10 times causes 0 changes to origin, maximum, spacing, and counts. Every grid cell has exactly 1 centre; all centres match their exported physical cells within `Tp`. -- **SC-012**: Invalid rotations and invalid saved metadata produce 1 actionable failure per attempted operation and 0 changes to valid prior state. An ambiguous legacy file fails without an explicit caller choice. With a supplied transform or identity declaration, its values and gradients satisfy SC-006. +- **SC-012**: Invalid rotations and invalid saved metadata produce 1 actionable failure per attempted operation and 0 changes to valid prior state. From the next major release, an ambiguous legacy file fails without an explicit caller choice; on the 1.x line it loads with the identity frame and one warning. With a supplied transform or identity declaration, its values and gradients satisfy SC-006. - **SC-013**: R4 reference samples and the new automatic direction targets agree with their analytic unit gradients within `1e-10` per component. The centre extent is `(0,1,0)`. The two off-centre sampled extent directions differ by at least 15 degrees. A constant centre direction applied at those probes fails acceptance. - **SC-014**: 100 percent of affected production paths have regression coverage. Before merge, `pytest` and `ruff check LoopStructural packages` must both pass. Affected package tests and API/import compatibility checks must also pass. Pre-change comparisons diagnose existing failures; they do not waive a failing required gate. - **SC-015**: 100 percent of changed golden results have analytic evidence and an explicit review record. Exactly 0 unaffected baseline files change. Acceptance tolerances are relaxed in exactly 0 cases to make a correction pass. -- **SC-016**: For populated R1, change the local origin to `O+(3,-2,1)` and rotate 30 degrees about z. Rejection causes 0 state changes. Acceptance causes 0 stale constraint or coefficient uses and preserves world evaluation within SC-002 after rebuilding. If restoration caches exist, repeat on R2 after populating them. After an accepted change, require 0 stale cache hits and world restored points within `Tp` of an uncached control. +- **SC-016**: For populated R1, change the local origin to `O+(3,-2,1)` and rotate 30 degrees about z. Rejection causes 0 state changes. If the policy rejects each later use of the stale state, each use of the populated interpolator raises 1 error. It causes 0 state changes. A rebuild in the new frame preserves world evaluation within SC-002. Acceptance causes 0 stale constraint or coefficient uses and preserves world evaluation within SC-002 after rebuilding. If restoration caches exist (spec 003 adds them), repeat on R2 after populating them. After an accepted change, require 0 stale cache hits and world restored points within `Tp` of an uncached control. - **SC-017**: Each stage records 1 rollback target and passes 1 rollback compatibility check. Old-format R1 inputs retain their documented load path and SC-006 accuracy. An unsupported new-format input causes 1 actionable failure and 0 mutations of valid state. Test an unknown future format version as a separate failure case. - **SC-018**: Every mandatory backend-matrix cell has a passing test result and 0 skips. Optional exclusions identify their exact dependency or unsupported capability. A missing optional backend is never reported as having passed. - **SC-019**: Before planning starts, both affected specs 003 and 004 contain the accepted scoped amendments. There are 0 remaining conflicts about corrected baselines, fingerprints, cache invalidation, or the coordinate-conversion boundary. This gate does not require either solver feature to be implemented. @@ -391,6 +393,7 @@ An unsupported capability must retain its documented rejection. Missing optional | FR-030 | U5.6; SC-018 | | FR-031 | SC-014; pre-fix and post-fix regression evidence | | FR-032 | U3.10; SC-017 | +| FR-033 | U3.11; SC-014 | ## Out of scope @@ -404,12 +407,13 @@ An unsupported capability must retain its documented rejection. Missing optional - Changes to other spec documents, branches, worktrees, commits, or release publication during this interview. - The trace and dip anisotropy settings of a fault after a property change. A rebuild after a property change uses anisotropy weights of 1.0, also when the caller set 0. FR-012 covers only the fault buffer and forced mesh geometry. Issue #394 records this defect (C-005). - Solver settings that `build` ignores. Top-level `rtol`, `atol` and `maxiter` have no effect, and no warning occurs; only `solver_kwargs` applies them. This is not a coordinate-frame defect. Issue #393 records it (C-006). The tests of this spec give the SC-003 solve settings through `solver_kwargs`. +- The accuracy of the P2 value-only fit of R1. In every frame it misses the SC-002 limits, also in the unrotated control. It depends on the solver and the regularisation, not on a coordinate space. Issue #399 records it (C-017). ## Risks and rollback Coordinate fixes intentionally change outputs that encoded the defect. This can affect stored golden results and user workarounds that supplied extra observations. The user approved reviewed changes supported by analytic evidence; unaffected baselines remain fixed. Migration notes must explain the corrected world-coordinate behaviour and the effect on such workarounds. -A saved solved field may lack the information needed to recover its physical placement. Loading then requires a caller-supplied transform or an explicit identity declaration. The error must explain that requirement. No warning-only identity fallback is allowed. +A saved solved field may lack the information needed to recover its physical placement. From the next major release, loading requires a caller-supplied transform or an explicit identity declaration, and the error explains that requirement. On the 1.x line, a legacy file with no choice loads with the identity frame and one warning (C-018). Stages must retain compatible entry points. A stage can be reverted independently if its acceptance checks fail. Files written in a new format must retain their declared version. A rollback must reject unsupported new files instead of silently treating them as legacy identity files. @@ -428,7 +432,7 @@ Each stage must demonstrate its rollback against the documented reader version a - **C-001, name**. Question: which name should this spec use? Answer: keep `005-coordinate-frame-correctness`. - **C-002, extent geometry**. Question: how should supplied centre, normal, and slip directions constrain generated extent geometry? Answer: use those directions at the centre and allow curvature away from it. An explicit planar mode is not required. -- **C-003, legacy persistence**. Question: how should an old interpolator load when its transform cannot be recovered? Answer: require the caller to supply the transform or explicitly declare identity. Do not assume identity with a warning. +- **C-003, legacy persistence**. Question: how should an old interpolator load when its transform cannot be recovered? Answer: require the caller to supply the transform or explicitly declare identity. Do not assume identity with a warning. (C-018 allows the warning-only fallback on the 1.x line until the next major release.) - **C-004, known-defect baselines**. Question: may analytic evidence justify a change to a golden result containing a coordinate defect? Answer: allow reviewed corrections to affected baselines and preserve unaffected results. All four interview decisions are resolved. The requirements, acceptance scenarios, edge cases, and success criteria above incorporate the answers. @@ -449,6 +453,22 @@ The S1 build (pull request #392) found two defects that the spec did not decide, - **C-006, ignored solver settings**. Question: must this spec make `build` apply or reject top-level solver settings? Answer: no. It is not a coordinate-frame defect. Issue #393 records it. - **C-007, R2 and R5 run time (plan OP2)**. Question: where does the mandatory R2 and R5 matrix run? Answer: in the default `pytest` run, with no marker. In S1, the four new test files take about 5 minutes. The analytic file takes 264 s. The full root suite takes about 14.5 minutes. +### Decisions after stages S2 to S4, answered on 2026-11-05 + +The maintainer decided these points during the S2 to S4 builds and plans. The plan holds their detail. + +- **C-008, constant-norm FDI defect (S2)**. `ConstantNormFDIInterpolator` raised `IndexError` for every frame, because `StructuredGrid.element_size` is one scalar. The defect does not depend on the frame. A separate task fixed it. No requirement changed. +- **C-009, policy for a transform change (S3, plan OP4)**. The interpolator rejects each later use of its stored data in another frame, and changes no state. S3 neither rejects the change itself nor rebuilds automatically. FR-020, U3.9 and SC-016 name this policy. +- **C-010, old reader (S3, plan OP5)**. A reader from before S3 fails on a new-format file with a `TypeError` that names `format_version`, and makes no object. The maintainer accepts this as the failure that SC-017 asks for at the rollback target. +- **C-011, partial legacy transform (S3 end review)**. A legacy file needs the local origin and the local rotation, both, or an identity declaration. A partial input raises an error that names the missing part. FR-019 states this. +- **C-012, P2 fit accuracy (S3, plan OP7)**. The P2 value-only fit of R1 misses SC-002 in every frame. The S3 rebuild test uses the exact nodal control for P2. C-017 decided the scope of SC-002 for P2. +- **C-013, deprecation of `rescale` (S4 plan)**. `GeologicalModel.rescale` keeps its behaviour and warns. FR-033 and U3.11 state this. +- **C-014, version of the legacy-load break**. The first draft of FR-028 asked for a major version with a `BREAKING CHANGE` commit footer. C-015 replaced it. +- **C-015, release of the legacy-load break (2026-11-05, maintainer)**. Question: how does the break of FR-019 ship? Options were a major release with a `BREAKING CHANGE` footer on the squash commit of pull request #397, a hold of stage S3 for a 2.0 line, or a minor release with a note. Answer: a minor release with a note. (C-018 removes the break from 1.x, so no exception is needed there; the rest of this entry applies to the major release.) The rule of `ROADMAP.md` ("1.x stays truly backward compatible") gets an explicit exception. The constitution asks for a "major-version note" for a breaking change. The note is the statement of this decision in `ROADMAP.md`, `COMPAT.md` and the release notes. If the maintainer reads the constitution as a demand for a major release, the maintainer amends it by pull request. +- **C-016, deprecation of `scale` (2026-11-05, maintainer)**. Question: deprecate `GeologicalModel.scale` with `rescale`? Options were: both together, `rescale` only, or `scale` one minor release later. Answer: both together. Nothing in the library calls `scale`, so no library code changes for it. The warning names the bounding-box projection as the replacement. `COMPAT.md` gets one row for each method. +- **C-017, SC-002 for the P2 fit (2026-11-05, maintainer)**. Question: does SC-002 apply to the P2 value-only fit of R1? The fit misses SC-002 in every frame: the gradient error is 3.9e-7 against a limit of 1e-8, also in the unrotated control; the value error is 7.8e-8 against a limit of 7.5e-8 unrotated, and 2e-7 to 7.6e-7 at 30 degrees (the earlier figure of 1.8e-8 was a ratio, not the error). Options were: the exact nodal field only, a frame-invariance bound for the fit, or a fix of the P2 fit in this spec. Answer: the exact nodal field only. SC-002 for P2 checks the coordinate conversion. The fit accuracy is a solver property and goes to a separate issue (#399), as C-005 and C-006 did. The backend table and the out-of-scope list say so. +- **C-018, backward-compatible legacy load (2026-11-05, maintainer)**. The maintainer will release a major version, but not yet. Question: can the change of FR-019 stay backward compatible until then? Answer: yes. On the 1.x line, a legacy file with no choice loads with the identity frame that older releases assumed, and it emits one `FutureWarning` (visible to end users by default, because a wrong placement can be silent). The new keyword arguments (a supplied transform, an identity declaration) work now, and a partial transform is refused. The code carries a note to remove the fallback at the next major release, when the loader raises the error of FR-019. This replaces the sentence "do not assume identity with a warning" of C-003 for the 1.x line only. From the major release, C-003 applies in full. No break ships in 1.x, so C-015 needs no exception to the 1.x rule. + ## Next step -Stage S1 is implemented in pull request #392. After it merges, run the spec-planner skill for stage S2 (plan OP1). No clarification marks remain. +S1 is merged (#392). S2 is in pull request #396, and S3 in #397 (stacked on #396). The S4 plan is in `plan.md`. No clarification mark is open. Run the spec-tasks skill for stage S4. Stages S5 and S6 need the spec-planner skill (plan OP1). diff --git a/specs/005-coordinate-frame-correctness/tasks.md b/specs/005-coordinate-frame-correctness/tasks.md index bd8ffaf99..7b85698e4 100644 --- a/specs/005-coordinate-frame-correctness/tasks.md +++ b/specs/005-coordinate-frame-correctness/tasks.md @@ -1,4 +1,4 @@ -# Tasks: Coordinate and fault-frame correctness (stages S1, S2 and S3) +# Tasks: Coordinate and fault-frame correctness (stages S1 to S4) **Feature**: `005-coordinate-frame-correctness` | **Plan**: [plan.md](plan.md) @@ -9,16 +9,16 @@ Rules: Run the test and see it fail first. - [P] = can be done at the same time as the other [P] tasks of the same wave. - Written in ASD-STE100 Simplified Technical English. -- This file covers stages S1 (T001 to T017, done), S2 (T018 to T032) and S3 - (T033 to T050) of plan.md. Stages S4 to S6 wait for OP1 (a later - spec-planner run for each stage, then a new spec-tasks run). +- This file covers stages S1 (T001 to T017, done), S2 (T018 to T032), S3 + (T033 to T050, done) and S4 (T051 to T068) of plan.md. Stages S5 and S6 wait + for OP1 (a later spec-planner run for each stage, then a new spec-tasks run). --> ## Orchestrator notes - Run this file with the spec-orchestrator skill. It has the rules for the worktree, the sub agents, the checks and the commits. -- Waves: 22 (S1: 1 to 7, done; S2: 8 to 13; S3: 14 to 22). Tiers: [S] = simple model, [M] = standard model. +- Waves: 31 (S1: 1 to 7, done; S2: 8 to 13; S3: 14 to 22, done; S4: 23 to 31). Tiers: [S] = simple model, [M] = standard model. - Do not start a task with "Blocked by". ## Phase 1: Setup @@ -966,6 +966,206 @@ frame. A legacy file needs an explicit choice. A frame change never gives a stal path now raises `StaleFrameError` in `pytest`, report it; do not weaken the check. Add to the status log of `ROADMAP.md` a line dated today: "Spec 005 stage S3 implemented: saved interpolator frame, legacy-load rule, frame-keeping copies, stale-frame check." Write in ASD-STE100 Simplified Technical English. +## Phase 14: Stage S4 foundation + +- [x] T051 [Found] [M] Add the S4 helper `world_foliation_model` to the shared fixtures. + - Files: `tests/fixtures/coordinate_frames.py` + - Needs: T050 + - Wave: 23 + - Done when: `python -c "from tests.fixtures.coordinate_frames import world_foliation_model"` prints no error, and `pytest tests/unit/modelling/test_frame_persistence.py -k payload_declares` still passes. + - Context: Add `world_foliation_model(anchor, rotation_deg) -> tuple[GeologicalModel, GeologicalFeature, pd.DataFrame]`. Build the model with the existing + `model_with_frame(anchor, anchor + (8, 10, 12), rotation_deg)`. The data frame is in WORLD coordinates with the columns + `X, Y, Z, feature_name, val, nx, ny, nz, gx, gy, gz, w`: 27 value rows from `r1_observations(anchor)`; 4 gradient rows and 4 normal rows at + `anchor + (2, 3, 4)`, `(5, 6, 7)`, `(3, 8, 5)` and `(6, 4, 9)` with the unit direction `R1_GRADIENT / norm(R1_GRADIENT)`. + `feature_name` is `"r1"` for every row. Set `model.data` to the frame and make the feature with `model.create_and_add_foliation("r1", ...)`. Do NOT solve + (no call that runs the interpolation). Copy the call pattern of `create_and_add_foliation` from `tests/integration/test_interpolator.py` and + `tests/unit/interpolator/test_outside_box.py` (read both). Return the model, the feature and the data frame. Do not change existing helpers. + Rule: input data is world data; the interpolator converts it once. + +## Phase 15: User story 3 - display, export and classification use world locations (S4, FR-022) + +**Goal**: `get_data`, the data export and the intrusion classification use the stored world locations, with no second conversion. `rescale` and `scale` warn. +**Independent test**: `pytest tests/unit/modelling/test_world_location_consumers.py`. + +- [x] T052 [US3] [M] Test: display and export locations are world (U3.4; FR-022). + - Files: `tests/unit/modelling/test_world_location_consumers.py` + - Needs: T051 + - Wave: 24 + - Done when: `pytest tests/unit/modelling/test_world_location_consumers.py -k "feature_data_locations or example_observation or exported_data"` fails before T057 (record the failing ids and the first error of each) and passes after T057. + - Context: New file. Use `world_foliation_model` (T051), `ANCHORS`, `ROTATIONS_DEG` and `tp(L, W)` from `tests.fixtures.coordinate_frames` (read the file for the exact names). + `test_feature_data_locations_are_world`: 4 anchors x 3 rotations = 12 cells. `feature.get_data()` gives a `ValuePoints` (value) and `VectorPoints` (gradient and norm). + Each `locations` equals the input rows within `tp(L, W)`; each `vectors` equals the input vectors within `1e-12`. + `test_spec_example_observation_is_not_shifted`: `ANCHORS[1]` x 3 rotations = 3 cells. The observation at `anchor + (2, 3, 4)` is at that point, and not at the point shifted by the local origin + (for origin `(100,200,300)` and identity rotation: `(102,203,304)` not `(202,403,604)`). + `test_exported_data_keeps_world_locations`: 4 anchors, rotation 30 = 4 cells. Save each object of `get_data()` with its `save` method to a CSV file in `tmp_path` + (read `LoopStructural/datatypes/_point.py` `save` first for the extension it supports; read only). The `x`, `y`, `z` columns equal the input within `tp(L, W)`. + The test file must not call `model.rescale`. Pre-fix expectation: the three tests fail by a location shifted by the local origin. + +- [x] T053 [US3] [M] Test: intrusion classification evaluates the model at world points (U3.5; FR-022). + - Files: `tests/unit/modelling/test_world_location_consumers.py` + - Needs: T052 + - Wave: 25 + - Done when: `pytest tests/unit/modelling/test_world_location_consumers.py -k intrusion_classification` fails before T058 with `AttributeError` (the method does not exist yet), then passes after T058. + - Context: Append to the file of T052. `test_intrusion_classification_uses_world_points`: 4 anchors x 3 rotations = 12 cells. Make a real rotated `GeologicalModel` (use `model_with_frame`). + Make the builder with `object.__new__(IntrusionFrameBuilder)` (import from `LoopStructural.modelling.intrusions.intrusion_frame_builder`) and set `builder.model = model`. + Replace `model.evaluate_model` with a spy that stores its argument and returns `np.zeros(len(xyz))`. Call `builder._network_model_values(data)` on a data frame with the world + columns `X, Y, Z` (at least 5 rows, taken from the anchor). The spy gets exactly the world X, Y, Z: `np.array_equal(spy_arg, data[["X","Y","Z"]].to_numpy(float))`. + The signature of the method under test is `_network_model_values(self, data: pd.DataFrame) -> np.ndarray`. + +- [x] T054 [US3] [M] Test: the 2D viewer draws the fault ellipse at the world centre (U3.5, coverage; no defect). + - Files: `tests/unit/modelling/test_world_location_consumers.py` + - Needs: T053 + - Wave: 26 + - Done when: `pytest tests/unit/modelling/test_world_location_consumers.py -k fault_ellipse` passes before and after the fixes (record it as "coverage, no defect"). + - Context: Append to the file of T053. `test_2d_viewer_fault_ellipse_centre_is_world`: 2 anchors x 2 rotations (0 and 30) = 4 cells. Call `Loop2DView.add_fault_ellipse` + (`packages/loopstructural_visualisation/src/loopstructuralvisualisation/_2d_viewer.py`, lines 211 to 224; read it) as an unbound function with a stand-in `self` that has + `ax` (a Matplotlib axis from the `Agg` backend) and `model`. The patch centre equals `fault.fault_centre[:2]` within `tp(L, W)`. + Build the fault with `_fault(anchor, rotation, "PLI")` from `tests/unit/modelling/test_fault_frame_analytic.py` (import it; read the file for the signature). + Use `matplotlib.use("Agg")`. Do not change the viewer. + +- [x] T055 [US3] [M] Record the S4 pre-fix evidence in `verification.md`. + - Files: `specs/005-coordinate-frame-correctness/verification.md` + - Needs: T054 + - Wave: 27 + - Done when: `verification.md` has the heading `## S4 pre-fix failures` with the failing test ids, the first error of each, and the measured shift; no fix commit exists yet. + - Context: Run `pytest tests/unit/modelling/test_world_location_consumers.py` on the unfixed code (the root suite only; one call). Record: the tests that fail and their first error; the fault ellipse test + as "coverage, no defect". Add a behavioural measure with the production path: for `ANCHORS[1]` and each of the 3 rotations, the distance between the observation + `anchor + (2, 3, 4)` and the location in `feature.get_data()` (use `world_foliation_model`). Use the layout of the S3 pre-fix section of the same file. FR-031. + Write in ASD-STE100 Simplified Technical English. + +- [x] T056 [US3] [M] Test: `rescale` and `scale` warn, keep behaviour, and no library caller warns (U3.11; FR-033). + - Files: `tests/unit/modelling/test_world_location_consumers.py` + - Needs: T054 + - Wave: 27 + - Done when: `pytest tests/unit/modelling/test_world_location_consumers.py -k "rescale_warns or scale_warns or internal_callers"` fails before T057 to T062 (record the failing ids) and passes after T062. + - Context: Append to the file of T054. `test_rescale_warns_and_keeps_behaviour`: 4 anchors x 3 rotations = 12 cells. `with pytest.warns(DeprecationWarning, match="bounding_box.reproject")`: + `model.rescale(points)` raises one warning; the warning is attributed to the test file (`record[0].filename == __file__`); the result equals `model.bounding_box.reproject(points)` within `tp(L, W)`; + `inplace=True` changes the passed array like `reproject(points, inplace=True)` does. `test_scale_warns_and_keeps_behaviour`: same 12 cells with `match="bounding_box.project"` and `model.scale` + against `model.bounding_box.project`. `test_internal_callers_do_not_warn`: 1 cell. Use `warnings.filterwarnings("error", message=".*(rescale|scale) is deprecated.*")`; run `feature.get_data()`, + `ModelExporter.get_block_model(model)` and `IntrusionFrameBuilder._network_model_values(data)` with no warning. Also call the exporter functions that + `tests/unit/io/test_exporters.py` already calls (read it for the call pattern) with no warning. `rescale(self, points, *, inplace=False)` and `scale(self, points, *, inplace=False)` keep their signatures. + +## Phase 16: Stage S4 implementation + +- [x] T057 [P] [US3] [M] Remove the extra conversion from `GeologicalFeature.get_data`. + - Files: `LoopStructural/modelling/features/_geological_feature.py` + - Needs: T052, T055 + - Wave: 28 + - Done when: `pytest tests/unit/modelling/test_world_location_consumers.py -k "feature_data_locations or example_observation or exported_data"` passes. + - Context: In `get_data(self, value_map: dict | None = None)` (lines 303 to 372) remove the four `self.model.rescale(...)` calls. The four location arguments become + `gradient_constraints[:, :3]`, `norm_constraints[:, :3]`, `value_constraints[value_constraints == v, :3]` (keep this mask as it is; pre-existing defect OP10) and `value_constraints[:, :3]`. + Add to the docstring: "The locations and the vectors are in world coordinates, as the builder stores them." Change no other line. The commented-out block at the end of the method stays. + Do not touch the misfit methods (S5). + +- [x] T058 [P] [US3] [S] Evaluate the intrusion network at the stored world points. + - Files: `LoopStructural/modelling/intrusions/intrusion_frame_builder.py` + - Needs: T053, T055 + - Wave: 28 + - Done when: `pytest tests/unit/modelling/test_world_location_consumers.py -k intrusion_classification` passes. + - Context: Add the private method to `IntrusionFrameBuilder`, with a numpy docstring ("The id of the unit at each point. The model is evaluated at the world points; no conversion is applied."): + `def _network_model_values(self, data: pd.DataFrame) -> np.ndarray:` with the body `xyz = data.loc[:, ["X", "Y", "Z"]].to_numpy(dtype=float)` then `return self.model.evaluate_model(xyz)`. + In `set_intrusion_steps_parameters`: delete the assignment of `intrusion_network_data_xyz` (lines 333 to 335; no other use), and replace the statement at line 352 (the one with + `self.model.rescale(...)`) by `intrusion_network_data.loc[:, "model_values"] = self._network_model_values(intrusion_network_data)`. Keep `pd` and `np` imports as they are. + +- [x] T059 [P] [US3] [S] Deprecate `GeologicalModel.rescale` and `GeologicalModel.scale`. + - Files: `LoopStructural/modelling/core/geological_model.py` + - Needs: T056 + - Wave: 28 + - Done when: `pytest tests/unit/modelling/test_world_location_consumers.py -k "rescale_warns or scale_warns"` passes, and `pytest tests/unit/test_public_api_contract.py` passes. + - Context: Keep both signatures and the decorator `@public_api(tier="stable")`. `warnings` is already imported (line 649). New body of `rescale(self, points: np.ndarray, *, inplace: bool = False) -> np.ndarray`: + `warnings.warn("GeologicalModel.rescale is deprecated and will be removed in a future release; use model.bounding_box.reproject(points) instead", DeprecationWarning, stacklevel=2)` then + `return self.bounding_box.reproject(points, inplace=inplace)`. New body of `scale(...)`: the same warning with `scale` and `model.bounding_box.project(points)`, then + `return self.bounding_box.project(np.array(points).astype(float), inplace=inplace)`. Add to each docstring a `.. deprecated::` note that names the replacement and says + "Deprecated by spec 005, stage S4". Change no other line. + +- [x] T060 [P] [US3] [S] Delete the four discarded `model.rescale` calls in the exporters. + - Files: `LoopStructural/export/exporters.py` + - Needs: T056 + - Wave: 28 + - Done when: `grep -c "model.rescale" LoopStructural/export/exporters.py` prints 0, and `pytest tests/unit/io/test_exporters.py` passes. + - Context: The four statements are `model.rescale(verts)`, `model.rescale(points)`, `model.rescale(xyz)` and `model.rescale(bbox)` (near lines 92, 366, 431 and 480). Each discards its result, + so each does nothing. Delete only these statements. Keep the argument `real_coords` in every signature (it is ignored; pre-existing). Remove a local name only if the deleted line was its last use and ruff reports it unused. + +- [x] T061 [P] [US3] [S] Use the bounding box in `ModelExporter.get_block_model`. + - Files: `LoopStructural/modelling/core/_model_exporter.py` + - Needs: T056 + - Wave: 28 + - Done when: `grep -c "model.rescale" LoopStructural/modelling/core/_model_exporter.py` prints 0, and `pytest tests/unit/io/test_exporters.py` passes. + - Context: At line 77 replace `model.rescale(model.bounding_box.cell_centres())` by `model.bounding_box.reproject(model.bounding_box.cell_centres())`. The result is the same (local to world). + Change no other line. + +- [x] T062 [P] [US3] [S] Fix the double conversion in the faulted intrusion example. + - Files: `examples/3_fault/plot_1_faulted_intrusion.py` + - Needs: T056 + - Wave: 28 + - Done when: `grep -c "rescale" examples/3_fault/plot_1_faulted_intrusion.py` prints 0, and `python -m py_compile examples/3_fault/plot_1_faulted_intrusion.py` passes. + - Context: Two `viewer.add_points` calls (near lines 71 and 100) wrap the world data of the model in `model.rescale(..., inplace=False)`. Pass + `model.data[model.data["feature_name"] == "strati"][["X", "Y", "Z"]].values` directly (world points) and delete the `model.rescale(` wrapper with its `inplace=False`. Keep the other arguments of each call. + Do not run the example (it needs the viewer). + +## Phase 17: Stage S4 finish + +- [x] T063 [Finish] [M] Review the existing tests for golden changes and record the S4 post-fix results. + - Files: `specs/005-coordinate-frame-correctness/verification.md` + - Needs: T057, T058, T059, T060, T061, T062 + - Wave: 29 + - Done when: `verification.md` has the headings `## S4 post-fix results` and `## S4 golden review (C-004)`, and `pytest tests/unit/modelling/test_world_location_consumers.py` passes (all cells). + - Context: Run, in separate calls: the new test file; `tests/unit/modelling/intrusions/test_intrusions.py`; `tests/unit/io/test_exporters.py`; `tests/unit/test_public_api_contract.py`. + Record commands, counts and times. Under the golden review list each existing test whose expected value must change, with the defect and its effect (C-004), or write + "No golden change in S4." Do not change an existing test; report one that fails. Record the new cells (count) and that the fault ellipse test is "coverage, no defect". Write in ASD-STE100 Simplified Technical English. + +- [x] T064 [P] [Finish] [M] Record the S4 notices in `COMPAT.md`. + - Files: `COMPAT.md` + - Needs: T057, T059 + - Wave: 29 + - Done when: `grep -c "GeologicalModel.rescale(points, inplace=False)" COMPAT.md` prints at least 1 and `grep -c "GeologicalModel.scale(points, inplace=False)" COMPAT.md` prints at least 1. + - Context: Read `COMPAT.md` first, the S1 to S3 notices and the deprecation table, and copy their layout. (1) Add a migration notice: `GeologicalFeature.get_data()` (used by `Loop2DView.add_data` and + `ModelExporter.save`) returned locations moved by the local origin and rotated a second time, and the intrusion classification evaluated the model at such points. Both now use the stored world locations. + With an identity frame nothing changes. (2) Add two rows to the deprecation table. `rescale`: old path `GeologicalModel.rescale(points, inplace=False)`; new path + `GeologicalModel.bounding_box.reproject(points, inplace=False)`. `scale`: old path `GeologicalModel.scale(points, inplace=False)`; new path `GeologicalModel.bounding_box.project(points, inplace=False)`. + Both: shim added `2026-11-05`; first released "TBD (next release containing this warning)"; introduced by the S4 commit; earliest removal "TBD (2 minor releases after the first release that warns)"; removed empty; status "Active"; owner "Core maintainers". + (3) Raise the count of "Active shims" by 2. Do not change other text. + +- [x] T065 [P] [Finish] [S] Record the S4 deprecation decision in `ROADMAP.md`. + - Files: `ROADMAP.md` + - Needs: T059 + - Wave: 29 + - Done when: `grep -c "Deprecation of GeologicalModel.rescale and scale (spec 005, S4)" ROADMAP.md` prints 1. + - Context: Add a new `###` subsection as the last one of `## Decisions`, directly before `## Stage sequence`. Heading: `### Deprecation of GeologicalModel.rescale and scale (spec 005, S4)`. + Body (ASD-STE100): the decision (2026-11-05, C-013 and C-016); `rescale` (local to world) and `scale` (world to local) keep their behaviour and signature and emit a `DeprecationWarning` + that names `bounding_box.reproject` and `bounding_box.project`; this replaces the status-log entry "2c-3" that said both stay as signature-stable pass-throughs; the tier stays "stable" until removal; + removal not before 2 minor releases after the first release that warns, and not before known callers (the QGIS plugin) have moved; the library no longer calls them. Keep the style of the neighbouring subsections. + Do not change other text. + +- [x] T066 [P] [Finish] [S] Mark the two methods as deprecated in `API.md`. + - Files: `API.md` + - Needs: T059 + - Wave: 29 + - Done when: `grep -c "deprecated, use .bounding_box.reproject" API.md` prints 1. + - Context: Line 122 lists the geometry methods of `GeologicalModel`. Change that entry to read: "Geometry: `scale` (deprecated, use `bounding_box.project`), `rescale` (deprecated, use `bounding_box.reproject`), + `regular_grid`, `bounding_box` (property)". Keep the line format of its neighbours. Do not change other text. + +- [x] T067 [Finish] [M] Record the S4 rollback check in `verification.md`. + - Files: `specs/005-coordinate-frame-correctness/verification.md` + - Needs: T063 + - Wave: 30 + - Done when: `verification.md` has the heading `## S4 rollback checks` with the results of the 4 checks below, and `git worktree list` shows no throw-away worktree. + - Context: Use a throw-away worktree at the S4 start commit (the `start_commit` of stage S4 in `run-state.json`; if the key is missing, use the parent of the first S4 code commit): + `git worktree add --detach ../LoopStructural-005-s4-rollback `, with its own `PYTHONPATH` (check `LoopStructural.__file__`). Checks: (a) `from loopstructuralvisualisation import Loop2DView, Loop3DView` works and + `inspect.signature(Loop2DView.add_data)` is equal before and after; (b) on the old code `get_data()` shows the spec example at `(202,403,604)`, on the S4 code at `(102,203,304)`; (c) with an identity frame the two codes + give the same `get_data()` output; (d) on the old code `model.rescale(p)` and `model.scale(p)` give no warning, on the S4 code each gives one, and the values are equal. + Then `git worktree remove --force ` and `git worktree prune`. + +- [x] T068 [Finish] [M] Run the S4 gates and complete `verification.md` and the `ROADMAP.md` status log. + - Files: `specs/005-coordinate-frame-correctness/verification.md`, `ROADMAP.md` + - Needs: T064, T065, T066, T067 + - Wave: 31 + - Done when: `ruff check LoopStructural packages && pytest` and `pytest packages/loop_common/tests packages/loop_interpolation/tests` pass, `verification.md` has the S4 sections named below, and `ROADMAP.md` has one new status-log line for S4. + - Context: Run the root `pytest` and the package tests in separate calls (the conftest plugins clash) with the worktree `PYTHONPATH`. Add to `verification.md`: `## S4 backend matrix` (Python FDI foliation data: mandatory; fault centre in the 2D viewer: coverage; + intrusion classification: mandatory; native, Surfe, browser: not in S4), `## S4 test run time (OP2)`, and the result of `git grep -n "\.rescale("` (it must show only the commented block of `get_data` and the tests of the deprecation; report any other hit). + Add the S4 gate counts to the post-fix section. Add to the status log of `ROADMAP.md` a line dated today: "Spec 005 stage S4 implemented: display, export and intrusion classification use stored world locations; rescale and scale deprecated." + Report any library warning with the text "is deprecated" in the pytest summary. Write in ASD-STE100 Simplified Technical English. + ## Wave table | Wave | Tasks | Tier [S] | Tier [M] | @@ -992,10 +1192,19 @@ frame. A legacy file needs an explicit choice. A frame change never gives a stal | 20 | T043, T044, T045, T046 | 3 | 1 | | 21 | T047, T048, T049 | 2 | 1 | | 22 | T050 | 0 | 1 | +| 23 | T051 | 0 | 1 | +| 24 | T052 | 0 | 1 | +| 25 | T053 | 0 | 1 | +| 26 | T054 | 0 | 1 | +| 27 | T055, T056 | 0 | 2 | +| 28 | T057, T058, T059, T060, T061, T062 | 5 | 1 | +| 29 | T063, T064, T065, T066 | 2 | 2 | +| 30 | T067 | 0 | 1 | +| 31 | T068 | 0 | 1 | ## Critical path -T003 -> T004 -> T005 -> T010 -> T013 -> T015 -> T017 -> T018 -> T020 -> T023 -> T025 -> T030 -> T031 -> T033 -> T035 -> T036 -> T039 -> T040 -> T041 -> T043 -> T047 -> T050 +T003 -> T004 -> T005 -> T010 -> T013 -> T015 -> T017 -> T018 -> T020 -> T023 -> T025 -> T030 -> T031 -> T033 -> T035 -> T036 -> T039 -> T040 -> T041 -> T043 -> T047 -> T050 -> T051 -> T052 -> T053 -> T054 -> T055 -> T057 -> T063 -> T067 -> T068 ## Dependencies @@ -1023,4 +1232,11 @@ T003 -> T004 -> T005 -> T010 -> T013 -> T015 -> T017 -> T018 -> T020 -> T023 -> T043 (factory loader) needs T041 and T042; T044, T045, T046 (copies) need T042. - Finish: T047 (`COMPAT.md`), T048 (`ROADMAP.md`) and T049 (rollback check) after the code; T050 (gates, `verification.md`, ROADMAP status) last. -- Stages S4 to S6 have no tasks yet (OP1 in plan.md). +- S4 starts after T050. Foundation: T051 (fixture helper). Tests, one file in a chain: + T052 (display and export), T053 (intrusion), T054 (2D viewer); T055 (pre-fix + evidence) and T056 (deprecation tests) after T054. +- S4 code, wave 28, separate files: T057 (`get_data`), T058 (intrusion helper), T059 + (`rescale` and `scale` warn), T060 and T061 (exporters), T062 (example). +- Finish: T063 (golden review) after the code; T064 to T066 (`COMPAT.md`, + `ROADMAP.md`, `API.md`); T067 (rollback check); T068 (gates) last. +- Stages S5 and S6 have no tasks yet (OP1 in plan.md). diff --git a/specs/005-coordinate-frame-correctness/verification.md b/specs/005-coordinate-frame-correctness/verification.md index 23a4c0ec6..481d3bfc7 100644 --- a/specs/005-coordinate-frame-correctness/verification.md +++ b/specs/005-coordinate-frame-correctness/verification.md @@ -484,3 +484,216 @@ Before C-018 the root suite gave 1981 passed. The difference of 13 comes from `t Review fix. The first review of the C-018 change found that the warning pointed at a library file for `from_json` and `from_yaml`, and one frame too high for a direct `InterpolatorFactory.from_dict` call. It also found that the warning came before a later failure of the same call. The fix moves the warning to `warn_legacy_identity()`, called as the last step of a successful load. The function computes the `stacklevel` as the first frame outside the `loop_interpolation` package. The tests `test_legacy_warning_points_at_caller` (4 entry points) and `test_legacy_warning_not_emitted_when_load_fails` (3 engines) failed before the fix (6 of 7 cells) and pass now. The gates above ran after the fix. No `StaleFrameError` and no `FutureWarning` from library code appeared in the full run. The only `FutureWarning` for a legacy load comes from the tests that load a legacy payload with no choice. + +## S4 pre-fix failures + +Method. The S4 consumer test file ran on the unfixed library code. No S4 library change exists at this commit. Another worker adds the deprecation tests (T056) to the same file at the same time, so the run used a filter and not the whole file. The exact command was `python -m pytest -p no:cacheprovider tests/unit/modelling/test_world_location_consumers.py -k "feature_data_locations or example_observation or exported_data or intrusion_classification or fault_ellipse" -q --tb=line`, with the worktree `PYTHONPATH` (`LoopStructural` came from `LoopStructural-005-s4`). The result is 30 failed and 5 passed. The `rescale` and `scale` tests did not run. + +| Test function | Failed / total | Representative id and first error | +|---|---|---| +| `test_feature_data_locations_are_world` | 11 / 12 | `[0-30.0]`: values differ from the input rows. `[0, 0]` is 1.3660 against 1.0 (limit 4.7e-08). The 11 failing cells share this error type (the locations are converted a second time). `[0-0.0]` passes (anchor 0, no rotation: the extra conversion is the identity; coverage, no defect) | +| `test_spec_example_observation_is_not_shifted` | 3 / 3 | `[0.0]`: `assert 374.1657 <= 4.7e-08`. The cells `[30.0]` and `[90.0]` fail in the same way (374.32 and 376.07) | +| `test_exported_data_keeps_world_locations` | 4 / 4 | `[0]`: values differ from the input rows. `[0, 0]` is 3.2321 against 2.0. The cells `[1]`, `[2]` and `[3]` fail in the same way (the CSV files hold converted locations) | +| `test_intrusion_classification_uses_world_points` | 12 / 12 | `[0-0.0]`: `AttributeError: 'IntrusionFrameBuilder' object has no attribute '_network_model_values'` (missing name). All 12 cells share this error | +| `test_2d_viewer_fault_ellipse_centre_is_world` | 0 / 4 | Passes now (coverage, no defect), 4 cells | + +Totals by group: the first three tests give 18 failed and 1 passed. The intrusion test gives 12 failed. The fault ellipse test gives 4 passed. + +Behavioural measure from the production path (a short script, not saved in the repository). It used `world_foliation_model` with `ANCHORS[1]` = (100, 200, 300). It measured the distance between the observation `anchor + (2, 3, 4)` = (102, 203, 304) and the nearest location in the gradient object of `feature.get_data()`. + +| Rotation (deg) | Distance | Nearest location | +|---|---|---| +| 0 | 374.17 | (202, 403, 604) | +| 30 | 374.32 | (289.83, 324.80, 604) | +| 90 | 376.07 | (303, 98, 604) | + +At rotation 0, the nearest location is the observation plus the anchor (102 + 100, 203 + 200, 304 + 300). The expected distance is 0 (limit 4.7e-08). The shift is about 374 length units in each cell. + +## S4 post-fix results + +Spec worktree `LoopStructural-005-s4`, HEAD `2e84bfcc` plus the S4 working tree +changes. The worktree `PYTHONPATH` was set. A check showed that +`LoopStructural.__file__` comes from `LoopStructural-005-s4`. The memory on the +machine was low, so each command ran alone and no full suite ran in this task. +The orchestrator runs the full gates in T068, and T068 adds the gate counts to +this section. + +Pre-fix result of T056. Before the fixes, 25 cells of the deprecation tests +failed. `test_rescale_warns_and_keeps_behaviour` (12 cells) and +`test_scale_warns_and_keeps_behaviour` (12 cells) failed with "DID NOT WARN". +`test_internal_callers_do_not_warn` failed with `AttributeError: +'IntrusionFrameBuilder' object has no attribute '_network_model_values'`. + +| Command | Result | +|---|---| +| `pytest -p no:cacheprovider tests/unit/modelling/test_world_location_consumers.py -q` | 59 passed, 1 skipped in 2.55 s. The skip is `test_internal_callers_do_not_warn`: `pyevtk` is not in the venv. | +| Same file, with `pyevtk` on `PYTHONPATH` (see below) | 60 passed in 2.28 s | +| `pytest -p no:cacheprovider tests/unit/modelling/intrusions/test_intrusions.py -q` | 9 passed, 2 warnings in 19.81 s (the warnings are an old `setup()` deprecation in the test, not from S4) | +| `pytest -p no:cacheprovider tests/unit/io/test_exporters.py -q`, with `pyevtk` on `PYTHONPATH` | 12 passed, 1 failed in 0.12 s (pre-existing, see below) | +| `pytest -p no:cacheprovider tests/unit/test_public_api_contract.py -q` | 2 passed in 0.02 s | + +The new test file has 60 cells: `test_feature_data_locations_are_world` 12, +`test_spec_example_observation_is_not_shifted` 3, +`test_exported_data_keeps_world_locations` 4, +`test_intrusion_classification_uses_world_points` 12, +`test_2d_viewer_fault_ellipse_centre_is_world` 4 (coverage, no defect), +`test_rescale_warns_and_keeps_behaviour` 12, `test_scale_warns_and_keeps_behaviour` 12, +`test_internal_callers_do_not_warn` 1. The fault ellipse test passed before and +after the fixes: coverage, no defect. + +`pyevtk` fact. `pyevtk` is not in the venv. For `test_exporters.py` and for +`test_internal_callers_do_not_warn`, the run added +`C:/Users/lachl/AppData/Local/Temp/claude/C--Users-lachl-dev-dev-tools/f206cbcd-e5d0-45be-a4c8-5f5c89bb26f1/scratchpad/pyevtk_pkg` +to `PYTHONPATH`. Without it, the file skips the `pyevtk` tests. + +Failure in `test_exporters.py`: `test_write_vol_gocad_is_broken_for_real_bounding_box`. +Pre-existing, not S4. The test expects `pytest.raises(LoopValueError)` with the +class from `LoopStructural.utils.exceptions`. The error comes from +`loop_common.geometry._bounding_box`, which raises its own, different +`LoopValueError` class (it derives from `ValueError`, not from the +`LoopStructural` class). The classes differ, so `pytest.raises` does not match. +The message is "slice(None, None, None) is not a valid bounding box name". The +S4 changes do not touch this path. The run with the S4 changes stashed was not +done (stash is not allowed in this task); the reason comes from reading the +test and the failure text. No existing test was changed. + +Deviations. +- T059 uses `stacklevel=3`, because the `public_api` wrapper adds one frame. +- The T056 `scale` check for `inplace` records the existing behaviour: `scale` + copies its input, and the array of the caller does not change. FR-033 says + keep the behaviour. + +S4 gate counts (T068). At HEAD `1ba71f17` (S4 code final), the root `pytest` +gives 2066 passed, 1 failed, 4 skipped. The one failure is the pre-existing +`test_write_vol_gocad_is_broken_for_real_bounding_box` above. The package tests +give 912 passed, 47 skipped, 2 xfailed. See `## S4 final gates`. + +## S4 golden review (C-004) + +Command: `git grep -l "get_data\|\.rescale(\|\.scale(" -- 'tests/*.py' 'packages/*.py'`. +The test files in the result were run one at a time with the worktree +`PYTHONPATH`: + +| File | Result | +|---|---| +| `tests/unit/modelling/test_world_location_consumers.py` | 60 passed (new file) | +| `tests/unit/interpolator/test_geological_interpolator.py` | 24 passed in 1.08 s | +| `tests/unit/io/test_geoh5.py` | 1 skipped in 0.05 s | +| `tests/unit/utils/test_helper.py` | 9 passed in 0.08 s | +| `tests/unit/modelling/test_geological_feature_builder.py` | 18 passed in 0.14 s | +| `packages/loop_interpolation/tests/test_geological_interpolator.py` | 40 passed in 6.27 s | +| `packages/loopstructural_visualisation/tests/test_plotting.py` | 16 passed in 5.07 s | +| `tests/unit/modelling/intrusions/test_intrusions.py` | 9 passed in 19.81 s | +| `tests/unit/test_public_api_contract.py` | 2 passed in 0.02 s | +| `tests/unit/io/test_exporters.py` (with `pyevtk`) | 12 passed, 1 failed (pre-existing, not S4) | + +No existing test needs a new expected value because of S4. The only failure, +`test_write_vol_gocad_is_broken_for_real_bounding_box`, has a cause that S4 does +not touch (two `LoopValueError` classes). + +No golden change in S4. + +## S4 rollback checks + +Method. The rollback target is the S4 start commit `9b3507a9` (`start_commit` of stage S4 in `run-state.json`; old code, no S4 code). A throw-away worktree was made at this commit with `git worktree add --detach ../LoopStructural-005-s4-rollback 9b3507a9a4d4c7d7a903a08c7d33619a7546bf61`. Nothing was copied into it. The S4 version of `tests/fixtures/coordinate_frames.py` was copied to a scratch directory outside the repository and imported from there (the old commit has no `world_foliation_model`). Each tree had its own `PYTHONPATH` (tree, `packages/loop_common/src`, `packages/loop_interpolation/src`, `packages/loopstructural_visualisation/src`). Before each run the script printed `LoopStructural.__file__` and checked that it is inside the tree. For the old code the path was in `LoopStructural-005-s4-rollback`. For the S4 code (HEAD `686322e9`) the path was in `LoopStructural-005-s4`. The script printed `loopstructuralvisualisation.__file__` as well; it was inside the same tree in each run. + +| Check | Old code (`9b3507a9`) | S4 code | Result | +|---|---|---|---| +| (a) `from loopstructuralvisualisation import Loop2DView, Loop3DView` | works | works | pass | +| (a) `inspect.signature(Loop2DView.add_data)` | `(self, feature, val=True, grad=True, unfault=False, dip=True, **kwargs)` | same text | pass (equal) | +| (a) `Loop3DView.add_data` | does not exist | does not exist | see note | +| (b) first gradient location of `world_foliation_model(ANCHORS[1], 0)`, `get_data()` | `(202, 403, 604)` | `(102, 203, 304)` | pass (spec example) | +| (c) `get_data()` with identity frame (`ANCHORS[0]`, rotation 0): 3 objects, locations and vectors | reference | max abs difference 0.0 for `r1+_gradient` (4 rows), `r1_norm` (4 rows), `r1_value` (27 rows) | pass (equal) | +| (d) `model.rescale(p)` warnings | 0 | 1: `GeologicalModel.rescale is deprecated ...` | pass | +| (d) `model.scale(p)` warnings | 0 | 1: `GeologicalModel.scale is deprecated ...` | pass | +| (d) values of `rescale` and `scale` | `rescale(p)` and `scale(p)` for `p = [[102, 203, 304], [110, 215, 320]]` | equal to the old values (exact) | pass | + +Note on (a). The task text names `Loop3DView.add_data`. This method does not exist in either tree (`hasattr` is false). `add_data` exists only in `Loop2DView`. In `Loop3DView` the only `add_*` method defined in the class is `add_mesh(self, *args, **kwargs)`, and it is equal in both trees. The check used the signatures of `Loop2DView.add_data` and `Loop3DView.add_mesh`. The task text must name `Loop2DView.add_data`. + +Check (b) used `ANCHORS[1]` = (100, 200, 300), rotation 0, and the gradient constraint at `anchor + (2, 3, 4)`. The old code shows the point shifted by the origin; the S4 code shows it at the world location. + +Cleanup. The throw-away worktree was removed with `git worktree remove --force C:/Users/lachl/dev/LoopStructural-005-s4-rollback`, then `git worktree prune` ran. `git worktree list` shows no `-rollback` entry. + +## S4 backend matrix + +Engines and consumers that S4 touches. Native engines, Surfe and the browser +viewer are not in S4. + +| Item | Level | Evidence | +|---|---|---| +| Python FDI foliation data (`get_data` locations, exported data) | mandatory | `test_feature_data_locations_are_world`, `test_spec_example_observation_is_not_shifted`, `test_exported_data_keeps_world_locations` | +| Fault centre in the 2D viewer | coverage (no defect found) | `test_2d_viewer_fault_ellipse_centre_is_world` passed before and after the fixes | +| Intrusion classification | mandatory | `test_intrusion_classification_uses_world_points` (12 cells) | +| Native engines | not in S4 | S6 | +| Surfe | not in S4 | S6 | +| Browser viewer | not in S4 | S6 | + +## S4 test run time (OP2) + +| Item | Time | +|---|---| +| Full root suite (orchestrator, HEAD `1ba71f17`) | 805.50 s (13 min 25 s). The S3 end run took 817.74 s. | +| Package suites (orchestrator) | 150.24 s. The S3 end run took 103.85 s; the two runs had different machine load, so the times do not compare. | + +The root time is about the same as the S3 time. The root suite has 72 more +passed tests than at the S3 end: the 60 cells of the consumer file and the +exporter tests that now run, because `pyevtk` is present. + +## S4 final gates + +The orchestrator ran the full gates at HEAD `1ba71f17` (S4 code final). The +`PYTHONPATH` held the worktree, `packages/loop_common/src`, +`packages/loop_interpolation/src`, `packages/loopstructural_visualisation/src` +and a `pyevtk` 1.7.0 directory (the venv has no `pyevtk`). + +| Command | Result | +|---|---| +| `ruff check LoopStructural packages` | All checks passed | +| `pytest packages/loop_common/tests packages/loop_interpolation/tests` | 912 passed, 47 skipped, 2 xfailed, 7 warnings in 150.24 s (same counts as the S3 end) | +| Root `pytest` | 1 failed, 2066 passed, 4 skipped, 12 warnings in 805.50 s | + +The one failure is `tests/unit/io/test_exporters.py::test_write_vol_gocad_is_broken_for_real_bounding_box`. +It is pre-existing and not from S4 (two different `LoopValueError` classes; see +`## S4 post-fix results`). S3 skipped it because `pyevtk` was missing. The S3 +end had 1994 passed and 5 skipped. The 72 new passes are the 60 consumer cells +and the exporter tests that now run. + +Because of this one failure, the "Done when" line of T068 (`pytest` passes) is +not fully met. The failure is not in the scope of S4. + +## S4 deprecated-call grep (T068) + +`git grep -n "\.rescale("` shows these hits: + +- `LoopStructural/modelling/features/_geological_feature.py:366`: the commented block of `get_data`. +- `tests/unit/modelling/test_world_location_consumers.py:243` and `:247`: the deprecation tests. +- `COMPAT.md:17`: the deprecation table row (text only, not a call). +- `specs/005-coordinate-frame-correctness/plan.md` and `tasks.md`: spec text, not code. +- `specs/005-coordinate-frame-correctness/verification.md:568` and `:602`: this record. + +`git grep -n "\.scale("` shows these hits: + +- `COMPAT.md:18`: the deprecation table row (text only). +- `specs/005-coordinate-frame-correctness/plan.md`, `tasks.md` and `verification.md` (lines 568 and 603): spec text and this record. + +No code outside the commented block of `get_data` and the deprecation tests +calls `.rescale(` or `.scale(`. No other hit exists. + +## S4 library deprecation-warning check (T068) + +Command (with the S4 `PYTHONPATH` and the `pyevtk` directory): +`python -m pytest tests/unit/io/test_exporters.py tests/unit/modelling/intrusions tests/unit/modelling/test_world_location_consumers.py -q -p no:cacheprovider -W "error:.*is deprecated.*:DeprecationWarning"` +with the two warn-tests of `rescale` and `scale` deselected (they expect the warning). + +Result: 1 failed, 57 passed, 24 deselected, 2 warnings in 23.43 s. The failure +is the pre-existing `test_write_vol_gocad_is_broken_for_real_bounding_box`. The +summary has two warnings with the text "is deprecated": +`StructuralFrameBuilder.setup() is deprecated, use .build() instead`, from +`tests/unit/modelling/intrusions/test_intrusions.py` lines 55 and 100. The tests +call the old `setup()` method. This warning is old and not from S4. No warning +of `rescale` or `scale` appears, so no S4 code path calls them. + +## S4 follow-up: gocad test and viewer name + +- `tests/unit/io/test_exporters.py::test_write_vol_gocad_is_broken_for_real_bounding_box` failed only because two classes are named `LoopValueError`. `BoundingBox.__getitem__` raises the class of `loop_common.geometry._bounding_box` (a `ValueError`), and the test expected the class of `LoopStructural.utils.exceptions`. The test now accepts `(LoopValueError, ValueError)`. The file passes: 13 passed (run with `pyevtk` on `PYTHONPATH`). The full root suite was not run again after this change; the failure was the only one in the S4 root run. +- The task text and the S4 notice in `COMPAT.md` named `Loop3DView.add_data`. The method is `Loop2DView.add_data`. `plan.md`, `tasks.md` and `COMPAT.md` now say so. diff --git a/specs/README.md b/specs/README.md index c5c37b428..b8d314243 100644 --- a/specs/README.md +++ b/specs/README.md @@ -9,7 +9,7 @@ not certify that an implementation or a numerical acceptance gate passed. | [002-rbf-interpolator](002-rbf-interpolator/spec.md) | Draft, not reviewed | Continue its existing review and clarification workflow. This cascade did not change it. | | [003-fault-evaluation-performance](003-fault-evaluation-performance/spec.md) | Implementation in progress; 005 contract cascade applied | Its active plan and tasks remain in the existing LoopStructural-native worktree. OP3 and OP4 remain open; completed tasks, including concurrent T043, and measured failures are preserved. | | [004-matrix-free-directional-constraints](004-matrix-free-directional-constraints/spec.md) | Draft; 005 contract cascade applied; task graph checked | Review the updated plan and tasks. OP1 still requires the quadrature measurements before a default is chosen. | -| [005-coordinate-frame-correctness](005-coordinate-frame-correctness/spec.md) | S1 implemented 2026-11-04 on `feat/005-coordinate-frame-correctness` (17 of 17 tasks; evidence in [verification.md](005-coordinate-frame-correctness/verification.md)); S4 to S6 not planned | Review and merge the S1 branch. S2 planned and tasked 2026-11-04 (T018 to T031, waves 8 to 13); run spec-orchestrator for S2. S3 planned and tasked 2026-11-05 ("Stage S3 design" in plan.md; OP4 and OP5 closed; T033 to T050, waves 14 to 22; build after S2 merges). OP1: plan S4 to S6 in later planner runs. OP3: constant-norm accuracy check. OP2 closed 2026-11-04: the R2/R5 matrix stays in the default pytest run. 7 minor review findings are open (run-state.json). | +| [005-coordinate-frame-correctness](005-coordinate-frame-correctness/spec.md) | S1 implemented 2026-11-04 on `feat/005-coordinate-frame-correctness` (17 of 17 tasks; evidence in [verification.md](005-coordinate-frame-correctness/verification.md)); S5 and S6 not planned | Review and merge the S1 branch. S2 planned and tasked 2026-11-04 (T018 to T031, waves 8 to 13); run spec-orchestrator for S2. S3 planned and tasked 2026-11-05 ("Stage S3 design" in plan.md; OP4 and OP5 closed; T033 to T050, waves 14 to 22; build after S2 merges). S3 is built on `feat/005-s3-frame-persistence` (pull request #397, stacked on #396; 18 of 18 tasks, evidence in verification.md). S3 merged (#397); a follow-up keeps the legacy load backward compatible (pull request #400). S4 planned and tasked 2026-11-05 ("Stage S4 design" in plan.md; OP8 to OP10 have defaults and block nothing; T051 to T068, waves 23 to 31); run spec-orchestrator for S4. OP1: plan S5 and S6 in later planner runs. OP3: constant-norm accuracy check. OP2 closed 2026-11-04: the R2/R5 matrix stays in the default pytest run. 7 minor review findings are open (run-state.json). | ## Cascade of 005 diff --git a/tests/fixtures/coordinate_frames.py b/tests/fixtures/coordinate_frames.py index 0c4b6df98..f6415d576 100644 --- a/tests/fixtures/coordinate_frames.py +++ b/tests/fixtures/coordinate_frames.py @@ -251,6 +251,37 @@ def r1_interpolator( return interp, observations +def world_foliation_model( + anchor: np.ndarray, rotation_deg: float +) -> tuple[GeologicalModel, object, pd.DataFrame]: + """Return a model, an unsolved foliation and its world data in a rotated frame. + + The data frame holds 27 value rows, 4 gradient rows and 4 normal rows. + All locations are world coordinates. The function does not run the interpolation. + """ + model = model_with_frame(anchor, anchor + (8, 10, 12), rotation_deg) + columns = ["X", "Y", "Z", "feature_name", "val", "nx", "ny", "nz", "gx", "gy", "gz", "w"] + values = r1_observations(anchor) + offsets = np.array([(2, 3, 4), (5, 6, 7), (3, 8, 5), (6, 4, 9)], dtype=float) + points = anchor + offsets + unit = R1_GRADIENT / np.linalg.norm(R1_GRADIENT) + + def _direction_rows(prefix: str) -> pd.DataFrame: + rows = pd.DataFrame(points, columns=["X", "Y", "Z"]) + rows["feature_name"] = "r1" + rows["w"] = 1.0 + for axis, component in zip("xyz", unit): + rows[f"{prefix}{axis}"] = component + return rows + + data = pd.concat( + [values, _direction_rows("g"), _direction_rows("n")], ignore_index=True + ).reindex(columns=columns) + model.data = data + feature = model.create_and_add_foliation("r1", interpolatortype="FDI", nelements=1000) + return model, feature, data + + def strip_frame_keys(payload: dict) -> dict: """Return a deep copy of a payload without the frame keys (a legacy payload).""" legacy = copy.deepcopy(payload) diff --git a/tests/unit/io/test_exporters.py b/tests/unit/io/test_exporters.py index c85499b87..c47a2395b 100644 --- a/tests/unit/io/test_exporters.py +++ b/tests/unit/io/test_exporters.py @@ -235,7 +235,10 @@ def test_write_vol_gocad_is_broken_for_real_bounding_box(tmp_path): # raises instead of writing a VOXET file. model = _FakeModelForVolume() file_name = tmp_path / "vol_gocad" - with pytest.raises(LoopValueError): + # `BoundingBox.__getitem__` raises its own `LoopValueError` (defined in + # `loop_common.geometry._bounding_box`, a `ValueError`). It is not the class + # of `LoopStructural.utils.exceptions`, so match both. + with pytest.raises((LoopValueError, ValueError)): exporters.write_vol( model, str(file_name), "label", np.array([5, 5, 5]), FileFormat.GOCAD ) diff --git a/tests/unit/modelling/test_world_location_consumers.py b/tests/unit/modelling/test_world_location_consumers.py new file mode 100644 index 000000000..231c09cca --- /dev/null +++ b/tests/unit/modelling/test_world_location_consumers.py @@ -0,0 +1,309 @@ +"""Tests that consumers of model data show and export world locations.""" + +import itertools + +import numpy as np +import pandas as pd +import pytest + +from tests.fixtures.coordinate_frames import ( + ANCHORS, + ROTATIONS_DEG, + model_with_frame, + tp, + world_foliation_model, +) +from tests.unit.modelling.test_fault_frame_analytic import _fault + +LENGTH = 12.0 +WIDTH = 6500000.0 + 12.0 + + +def _input_rows(data: pd.DataFrame): + """Return the world locations of the value, gradient and norm rows.""" + values = data[data["val"].notna()] + gradients = data[data["gx"].notna()] + norms = data[data["nx"].notna()] + return values, gradients, norms + + +def _split_objects(objects): + """Return the ValuePoints, the gradient VectorPoints and the norm VectorPoints.""" + by_name = {obj.name: obj for obj in objects} + value = next(o for o in objects if hasattr(o, "values")) + gradient = next(o for n, o in by_name.items() if n.endswith("_gradient")) + norm = next(o for n, o in by_name.items() if n.endswith("_norm")) + return value, gradient, norm + + +@pytest.mark.parametrize( + "anchor_index, rotation", + list(itertools.product(range(len(ANCHORS)), ROTATIONS_DEG)), +) +def test_feature_data_locations_are_world(anchor_index, rotation): + """The data of a feature has the same world locations as the input rows.""" + anchor = ANCHORS[anchor_index] + _, feature, data = world_foliation_model(anchor, rotation) + values, gradients, norms = _input_rows(data) + value_obj, gradient_obj, norm_obj = _split_objects(feature.get_data()) + tol = tp(LENGTH, max(WIDTH, np.abs(anchor).max())) + np.testing.assert_allclose( + value_obj.locations, values[["X", "Y", "Z"]].to_numpy(float), rtol=0, atol=tol + ) + np.testing.assert_allclose( + gradient_obj.locations, gradients[["X", "Y", "Z"]].to_numpy(float), rtol=0, atol=tol + ) + np.testing.assert_allclose( + norm_obj.locations, norms[["X", "Y", "Z"]].to_numpy(float), rtol=0, atol=tol + ) + np.testing.assert_allclose( + gradient_obj.vectors, gradients[["gx", "gy", "gz"]].to_numpy(float), rtol=0, atol=1e-12 + ) + np.testing.assert_allclose( + norm_obj.vectors, norms[["nx", "ny", "nz"]].to_numpy(float), rtol=0, atol=1e-12 + ) + + +@pytest.mark.parametrize("rotation", ROTATIONS_DEG) +def test_spec_example_observation_is_not_shifted(rotation): + """The observation at anchor + (2, 3, 4) is at that point, not shifted by the origin.""" + anchor = ANCHORS[1] + _, feature, _ = world_foliation_model(anchor, rotation) + _, gradient_obj, _ = _split_objects(feature.get_data()) + expected = anchor + np.array([2.0, 3.0, 4.0]) + shifted = expected + anchor + tol = tp(LENGTH, WIDTH) + distance = np.linalg.norm(gradient_obj.locations - expected, axis=1) + shifted_distance = np.linalg.norm(gradient_obj.locations - shifted, axis=1) + assert distance.min() <= tol + assert shifted_distance.min() > 1.0 + + +@pytest.mark.parametrize("anchor_index", range(len(ANCHORS))) +def test_exported_data_keeps_world_locations(anchor_index, tmp_path): + """The CSV files of the feature data hold the world locations.""" + anchor = ANCHORS[anchor_index] + _, feature, data = world_foliation_model(anchor, 30.0) + values, gradients, norms = _input_rows(data) + expected = { + "value": values[["X", "Y", "Z"]].to_numpy(float), + "gradient": gradients[["X", "Y", "Z"]].to_numpy(float), + "norm": norms[["X", "Y", "Z"]].to_numpy(float), + } + tol = tp(LENGTH, max(WIDTH, np.abs(anchor).max())) + objects = feature.get_data() + assert len(objects) == 3 + for index, obj in enumerate(objects): + path = tmp_path / f"object_{index}.csv" + obj.save(str(path)) + frame = pd.read_csv(path) + kind = "value" if hasattr(obj, "values") else obj.name.rsplit("_", 1)[-1] + np.testing.assert_allclose( + frame[["x", "y", "z"]].to_numpy(float), expected[kind], rtol=0, atol=tol + ) + + +@pytest.mark.parametrize( + "anchor_index, rotation", + list(itertools.product(range(len(ANCHORS)), ROTATIONS_DEG)), +) +def test_intrusion_classification_uses_world_points(anchor_index, rotation): + """The intrusion builder evaluates the model at the world points of the data.""" + from LoopStructural.modelling.intrusions.intrusion_frame_builder import ( + IntrusionFrameBuilder, + ) + from tests.fixtures.coordinate_frames import model_with_frame + + anchor = ANCHORS[anchor_index] + model = model_with_frame(anchor, anchor + np.array([20.0, 20.0, 20.0]), rotation) + received = [] + + def spy(xyz): + received.append(np.array(xyz, copy=True)) + return np.zeros(len(xyz)) + + model.evaluate_model = spy + builder = object.__new__(IntrusionFrameBuilder) + builder.model = model + offsets = np.array( + [[1.0, 2.0, 3.0], [4.0, 5.0, 6.0], [7.0, 8.0, 9.0], [2.0, 9.0, 4.0], [8.0, 3.0, 5.0]] + ) + points = anchor + offsets + data = pd.DataFrame({"X": points[:, 0], "Y": points[:, 1], "Z": points[:, 2]}) + builder._network_model_values(data) + assert len(received) == 1 + assert np.array_equal(received[0], data[["X", "Y", "Z"]].to_numpy(float)) + + +@pytest.mark.parametrize( + "anchor_index, rotation", + list(itertools.product((0, 3), (0, 30))), +) +def test_2d_viewer_fault_ellipse_centre_is_world(anchor_index, rotation): + """The 2D viewer draws the fault ellipse at the world centre of the fault.""" + anchor = ANCHORS[anchor_index] + # Skip the test if the viewer package is not installed. + viewer = pytest.importorskip("loopstructuralvisualisation._2d_viewer") + matplotlib = pytest.importorskip("matplotlib") + matplotlib.use("Agg") + import matplotlib.pyplot as plt + + fault = _fault(anchor, rotation, "PLI") + fig, ax = plt.subplots() + try: + view = type("View", (), {})() + view.ax = ax + view.model = None + viewer.Loop2DView.add_fault_ellipse(view, faults=[fault]) + assert len(ax.patches) == 1 + tol = tp(LENGTH, max(WIDTH, np.abs(anchor).max())) + np.testing.assert_allclose(ax.patches[0].center, fault.fault_centre[:2], rtol=0, atol=tol) + finally: + plt.close(fig) + + +_OFFSETS = np.array([[1.0, 2.0, 3.0], [4.0, 5.0, 6.0], [7.0, 8.0, 9.0], [2.0, 9.0, 4.0]]) + + +def _deprecated_call_cell(anchor_index, rotation, method_name, target_name, match): + """Check that a deprecated model method warns and keeps its result.""" + anchor = ANCHORS[anchor_index] + model = model_with_frame(anchor, anchor + np.array([20.0, 20.0, 20.0]), rotation) + tol = tp(LENGTH, max(WIDTH, np.abs(anchor).max())) + points = anchor + _OFFSETS + expected = getattr(model.bounding_box, target_name)(points.copy()) + with pytest.warns(DeprecationWarning, match=match) as record: + result = getattr(model, method_name)(points.copy()) + assert len(record) == 1 + assert record[0].filename == __file__ + np.testing.assert_allclose(result, expected, rtol=0, atol=tol) + passed = points.copy() + with pytest.warns(DeprecationWarning, match=match): + result_inplace = getattr(model, method_name)(passed, inplace=True) + if method_name == "scale": + # scale copies the input, as before the deprecation; inplace does not change the + # caller array. + np.testing.assert_allclose(result_inplace, expected, rtol=0, atol=tol) + np.testing.assert_array_equal(passed, points) + else: + # With inplace=True the method changes the array that the caller passes. + expected_inplace = points.copy() + getattr(model.bounding_box, target_name)(expected_inplace, inplace=True) + np.testing.assert_allclose(passed, expected_inplace, rtol=0, atol=tol) + + +@pytest.mark.parametrize( + "anchor_index, rotation", + list(itertools.product(range(len(ANCHORS)), ROTATIONS_DEG)), +) +def test_rescale_warns_and_keeps_behaviour(anchor_index, rotation): + """model.rescale gives a DeprecationWarning and the result of reproject.""" + _deprecated_call_cell(anchor_index, rotation, "rescale", "reproject", "bounding_box.reproject") + + +@pytest.mark.parametrize( + "anchor_index, rotation", + list(itertools.product(range(len(ANCHORS)), ROTATIONS_DEG)), +) +def test_scale_warns_and_keeps_behaviour(anchor_index, rotation): + """model.scale gives a DeprecationWarning and the result of project.""" + _deprecated_call_cell(anchor_index, rotation, "scale", "project", "bounding_box.project") + + +def test_internal_callers_do_not_warn(tmp_path): + """No library caller uses the deprecated rescale or scale methods.""" + import warnings + + from LoopStructural.modelling.core._model_exporter import ModelExporter + from LoopStructural.modelling.intrusions.intrusion_frame_builder import ( + IntrusionFrameBuilder, + ) + + anchor = ANCHORS[1] + model, feature, _ = world_foliation_model(anchor, 30.0) + # The model is not solved. Use a stub for the model evaluation. + model.evaluate_model = lambda xyz: np.zeros(len(xyz)) + with warnings.catch_warnings(): + warnings.filterwarnings("error", message=".*(rescale|scale) is deprecated.*") + feature.get_data() + ModelExporter.get_block_model(model) + builder = object.__new__(IntrusionFrameBuilder) + builder.model = model + points = anchor + _OFFSETS + data = pd.DataFrame({"X": points[:, 0], "Y": points[:, 1], "Z": points[:, 2]}) + builder._network_model_values(data) + # Call the exporter functions in the way that tests/unit/io/test_exporters.py does. + pytest.importorskip("pyevtk") + from LoopStructural.export import exporters + from LoopStructural.export.file_formats import FileFormat + from tests.unit.io import test_exporters as fakes + + class _Surfaces(fakes._FakeModelForSurfaces): + def rescale(self, points, *, inplace=False): + return model.rescale(points, inplace=inplace) + + class _Volume(fakes._FakeModelForVolume): + def rescale(self, points, *, inplace=False): + return model.rescale(points, inplace=inplace) + + exporters.write_feat_surfs(_Surfaces(), "strati", file_format=FileFormat.NUMPY) + exporters.write_cubeface( + _Volume(), str(tmp_path / "cube"), "label", np.array([5, 5, 5]), FileFormat.VTK + ) + exporters.write_vol( + _Volume(), str(tmp_path / "vol"), "label", np.array([5, 5, 5]), FileFormat.VTK + ) + + +@pytest.mark.parametrize("anchor_index", (1, 3)) +def test_intrusion_steps_parameters_evaluate_model_at_world_points(anchor_index): + """The step parameters call evaluate_model with the world points, without conversion.""" + from types import SimpleNamespace + + from LoopStructural.modelling.intrusions.intrusion_frame_builder import ( + IntrusionFrameBuilder, + ) + + anchor = ANCHORS[anchor_index] + model = model_with_frame(anchor, anchor + np.array([20.0, 20.0, 20.0]), 30) + received = [] + + def spy(xyz): + received.append(np.array(xyz, copy=True)) + return np.zeros(len(xyz)) + + # Stand-in model. It keeps the real bounding box, so a conversion would change the points. + stand_in = SimpleNamespace( + evaluate_model=spy, + bounding_box=model.bounding_box, + stratigraphic_column={ + "s_from": {"u_from": {"id": 5, "min": 0.0, "max": 10.0}}, + "s_to": {"u_to": {"id": 6, "min": 10.0, "max": 20.0}}, + }, + ) + # Stand-ins for the series and the step structure. No point matches a unit id, + # so the method uses only the min and max of the unit. + series_from = SimpleNamespace(name="s_from", evaluate_value=lambda xyz: np.zeros(len(xyz))) + series_to = SimpleNamespace(name="s_to", evaluate_value=lambda xyz: np.zeros(len(xyz))) + structure = SimpleNamespace(evaluate_value=lambda xyz: np.zeros(len(xyz))) + builder = object.__new__(IntrusionFrameBuilder) + builder.model = stand_in + offsets = np.array( + [[1.0, 2.0, 3.0], [4.0, 5.0, 6.0], [7.0, 8.0, 9.0], [2.0, 9.0, 4.0], [8.0, 3.0, 5.0]] + ) + points = anchor + offsets + builder.intrusion_network_data = pd.DataFrame( + {"X": points[:, 0], "Y": points[:, 1], "Z": points[:, 2]} + ) + builder.intrusion_steps = { + 0: { + "structure": [structure], + "unit_from": "u_from", + "series_from": series_from, + "unit_to": "u_to", + "series_to": series_to, + } + } + builder.set_intrusion_steps_parameters() + assert len(received) == 1 + assert np.array_equal(received[0], points)