From c61175ebb187e96a8c28a9c72c68cc1c0c099639 Mon Sep 17 00:00:00 2001 From: lachlangrose Date: Thu, 5 Nov 2026 13:54:36 +1030 Subject: [PATCH 1/3] feat(interpolation): keep the legacy load backward compatible until the next major release A legacy interpolator payload with no choice of the caller loads with the identity frame, as older releases did, and emits one FutureWarning. A module switch, _STRICT_LEGACY, carries a note to remove the fallback at the next major release, when the loader raises an error. A partial transform, a conflict, a malformed payload and an unsupported version still raise. Update the ROADMAP.md decision, the COMPAT.md rows and verification.md, and correct the P2 fit figures (issue #399). Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01XADRvJ98bJ8Ey8Rw98F3j1 --- COMPAT.md | 5 +- ROADMAP.md | 31 +++++----- .../loop_interpolation/_frame_persistence.py | 62 ++++++++++++++----- .../run-state.json | 18 +++++- .../verification.md | 47 +++++++++++++- .../unit/modelling/test_frame_persistence.py | 39 +++++++++++- 6 files changed, 163 insertions(+), 39 deletions(-) diff --git a/COMPAT.md b/COMPAT.md index c4c1f71e8..5c9e56b74 100644 --- a/COMPAT.md +++ b/COMPAT.md @@ -13,6 +13,7 @@ any module-path move/rename on the 1.x line gets a re-export shim with a | `LoopStructural.interpolators._finite_difference_interpolator` | `loop_interpolation._finite_difference_interpolator` | 2026-07-29 | 2026-07-29 | extracted-package integration | 2 minor releases after first shim release | Active | Core maintainers | | `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 | ## Deprecation lifecycle @@ -49,12 +50,12 @@ unchanged, but the new path is preferred going forward. | `GeologicalModel.from_file(file)` (always loads via `dill`/`pickle`, no opt-out) | `GeologicalModel.from_file(file, allow_pickle=True)` — same default behavior (still unpickles trusted files with no code change required), but `allow_pickle=False` now refuses to unpickle and raises `LoopValueError` instead, since deserialising an untrusted pickle/dill file can execute arbitrary code. A runtime warning is also now logged whenever pickle-based loading is used. For untrusted/JSON-based input, use `GeologicalModel.from_recipe_dict`/`to_recipe_dict` instead. | 2026-07-30 | | `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 | -| Loading a legacy interpolator payload (no `format_version`) assumed an identity frame without a message; `copy()` returned an interpolator with no frame; a transform change after constraints existed gave a stale result | A legacy payload needs `local_origin` and `local_rotation`, or `declare_identity=True`, and raises `InterpolatorFormatError` otherwise. 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). Major-version note: the release that contains S3 must be a major version, or the maintainer records another decision in `ROADMAP.md` | 2026-11-05 | +| `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 | 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: 7 +- Active shims: 8 - 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/ROADMAP.md b/ROADMAP.md index a9e9c514f..088014500 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -93,31 +93,34 @@ against the plugin's own test/import suite on every PR/push to `master`, not just at release time. ### Legacy interpolator payloads (spec 005, FR-019) -Decision date: 2026-11-03 (spec 005, decision C-003, user decision). +Decision date: 2026-11-03 (spec 005, decision C-003, user decision). Amended on 2026-11-05 (C-018, user decision). A legacy interpolator payload has no recoverable transform. The payload does not store the local origin or the local rotation. The loader cannot find them from the data. -The caller must give one of these inputs when it loads a legacy payload: -- `local_origin` and `local_rotation`. The caller states the transform. +On the 1.x line, the loader keeps the old behaviour. A call with no choice +loads the payload with the identity frame. It emits one `FutureWarning`. +The warning names the choices and says that the next major release raises +an error. + +The caller can choose now: +- `local_origin` and `local_rotation` (both). The caller states the + transform. - `declare_identity=True`. The caller states that the transform is the identity. -If the caller gives neither input, the loader raises -`InterpolatorFormatError`. - -No warning-only identity fallback exists. The loader never loads a legacy -payload with an identity transform and only a warning. +A partial transform (only one of `local_origin` and `local_rotation`) +raises `InterpolatorFormatError`. -New payloads carry `format_version: 2`. +At the next major release, the loader raises `InterpolatorFormatError` for +a call with no choice. The code carries a note to remove the fallback +(`_STRICT_LEGACY` in `loop_interpolation._frame_persistence`). -This is the only approved break of the public loading behaviour in -spec 005 (FR-028). +New payloads carry `format_version: 2`. A reader from before S3 fails on +such a payload with a `TypeError` that names `format_version`. -The release that contains stage S3 must be a major version. If the -maintainer does not accept a major version, the maintainer records another -decision in this section. +No stage of spec 005 breaks the 1.x public API (FR-028). The policy for a transform change, after constraints exist, is "reject at use" (decided 2026-11-05). The interpolator raises `StaleFrameError` at the diff --git a/packages/loop_interpolation/src/loop_interpolation/_frame_persistence.py b/packages/loop_interpolation/src/loop_interpolation/_frame_persistence.py index 3350ee2d5..97ab51b7b 100644 --- a/packages/loop_interpolation/src/loop_interpolation/_frame_persistence.py +++ b/packages/loop_interpolation/src/loop_interpolation/_frame_persistence.py @@ -7,6 +7,7 @@ from __future__ import annotations import itertools +import warnings from typing import Any import numpy as np @@ -37,6 +38,17 @@ ) """Valid reasons for an `InterpolatorFormatError`.""" +# REMOVE AT THE NEXT MAJOR RELEASE (spec 005, C-018): delete the fallback in +# `payload_kind` and this constant. A legacy payload with no choice then always +# raises InterpolatorFormatError("legacy_no_transform", ...). +_STRICT_LEGACY: bool = False + +_LEGACY_IDENTITY_WARNING = ( + "legacy interpolator payload has no transform; assuming the identity frame. " + "Pass local_origin and local_rotation, or declare_identity=True, to choose " + "the frame. The next major release raises InterpolatorFormatError here." +) + class InterpolatorFormatError(ValueError): """Error raised when a stored interpolator payload cannot be used. @@ -108,6 +120,11 @@ def frame_payload(bounding_box: BoundingBox | None, dimensions: int) -> dict: def payload_kind(payload: dict, caller_frame: bool) -> str: """Return the kind of a stored payload. + On the 1.x line, a legacy payload (no `format_version`) with no choice of + the caller gives a `FutureWarning` and the kind `"legacy"`. The payload + then loads with the identity frame. If `_STRICT_LEGACY` is true, the + function raises an error in this case. + Parameters ---------- payload : dict @@ -123,9 +140,15 @@ def payload_kind(payload: dict, caller_frame: bool) -> str: Raises ------ InterpolatorFormatError - If the payload has no transform and the caller gives no frame - (`legacy_no_transform`), if the version is above `FORMAT_VERSION` - (`unsupported_version`), or if the payload is not valid (`malformed`). + If `_STRICT_LEGACY` is true and the payload has no transform and the + caller gives no frame (`legacy_no_transform`), if the version is above + `FORMAT_VERSION` (`unsupported_version`), or if the payload is not + valid (`malformed`). + + Warns + ----- + FutureWarning + If the payload has no transform and the caller gives no frame. """ version = payload.get("format_version") if version is None: @@ -136,11 +159,15 @@ def payload_kind(payload: dict, caller_frame: bool) -> str: "but no format_version", ) if not caller_frame: - raise InterpolatorFormatError( - "legacy_no_transform", - "legacy interpolator payload has no transform: pass local_origin " - "and local_rotation, or declare_identity=True", - ) + if _STRICT_LEGACY: + raise InterpolatorFormatError( + "legacy_no_transform", + "legacy interpolator payload has no transform: pass local_origin " + "and local_rotation, or declare_identity=True", + ) + # stacklevel 4: payload_kind <- InterpolatorFactory.from_dict + # <- GeologicalInterpolator.from_dict <- caller. + warnings.warn(_LEGACY_IDENTITY_WARNING, FutureWarning, stacklevel=4) return "legacy" if not isinstance(version, int) or isinstance(version, bool) or version < 1: raise InterpolatorFormatError( @@ -237,19 +264,22 @@ def resolve_frame( Number of spatial dimensions. local_origin : array_like, optional World-space origin of the local frame, given by the caller. A legacy - payload needs both `local_origin` and `local_rotation`, or - `declare_identity`. + payload with a choice needs both `local_origin` and `local_rotation`, + or `declare_identity`. A legacy payload with no choice gets the + identity frame. local_rotation : array_like, optional Rotation matrix of the local frame, given by the caller. A legacy - payload needs both `local_origin` and `local_rotation`, or - `declare_identity`. + payload with a choice needs both `local_origin` and `local_rotation`, + or `declare_identity`. A legacy payload with no choice gets the + identity frame. declare_identity : bool, optional True if the caller declares an identity frame. Returns ------- BoundingBox or None - Bounding box to attach. None for an identity frame with no box. + Bounding box to attach. None for an identity frame with no box. This + includes a legacy payload with no choice of the caller. Raises ------ @@ -273,6 +303,10 @@ def resolve_frame( ) caller_frame = local_origin is not None or local_rotation is not None or declare_identity + # Step 1b: a legacy payload with no choice gets the identity frame (C-018). + if kind == "legacy" and not caller_frame: + return None + # Step 2: check the transform of the caller. A legacy payload needs both values. if kind == "legacy" and caller_frame and not declare_identity: for name, value in (("local_origin", local_origin), ("local_rotation", local_rotation)): @@ -294,8 +328,6 @@ def resolve_frame( # Step 3: a legacy payload has no saved frame. if kind == "legacy": - if not caller_frame: - raise _malformed("legacy payload has no transform and the caller gives none") if declare_identity: return None try: diff --git a/specs/005-coordinate-frame-correctness/run-state.json b/specs/005-coordinate-frame-correctness/run-state.json index 5e6f248da..6cdec689d 100644 --- a/specs/005-coordinate-frame-correctness/run-state.json +++ b/specs/005-coordinate-frame-correctness/run-state.json @@ -909,8 +909,20 @@ "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 93.31 s", - "test_root": "1981 passed, 5 skipped, 13 warnings in 757.86 s (S2 head: 1760; +218 S3 cells +3 regression tests)", + "test_packages": "912 passed, 47 skipped, 2 xfailed, 7 warnings in 100.44 s (after C-018)", + "test_root": "1987 passed, 5 skipped, 13 warnings in 826.94 s (after C-018)", "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": "1987 passed, 5 skipped" + } + } + ] } diff --git a/specs/005-coordinate-frame-correctness/verification.md b/specs/005-coordinate-frame-correctness/verification.md index 5c62799b4..07dbd0460 100644 --- a/specs/005-coordinate-frame-correctness/verification.md +++ b/specs/005-coordinate-frame-correctness/verification.md @@ -308,6 +308,8 @@ repository): The identity-frame round-trip cells and the rebuild cells pass before S3. They are coverage, not defect evidence. +Since C-018, `test_legacy_payload_requires_choice` is replaced by `test_legacy_payload_without_choice_warns_and_loads_identity`, `test_legacy_identity_values_match_source_for_identity_frame` and `test_strict_legacy_switch_raises`. The pre-fix record above keeps the old name. + ## S3 rollback checks Method. The rollback target is the S3 start commit `91ffca4a` (the S2 head, with no S3 code). A throw-away worktree was made at this commit with `git worktree add --detach ../LoopStructural-005-s3-rollback 91ffca4a`. The file `tests/fixtures/coordinate_frames.py` was copied from the S3 worktree into it, and nothing else, because the old commit has no `r1_interpolator`. Each run used its own `PYTHONPATH`. Before each run, the script printed `loop_interpolation.__file__` and `LoopStructural.__file__`. For the old code, both paths were in `LoopStructural-005-s3-rollback`. For the S3 code, both paths were in `LoopStructural-005-s3`. The scripts were short and were not saved in the repository. The throw-away worktree was removed at the end. @@ -326,7 +328,7 @@ Check 1. The old `to_dict` saved a legacy payload (no `format_version`) for `fdi | `pli` | `ANCHORS[0]`, rotation 0 | `declare_identity=True` | 0.0 | 0.0 | | `p2` | `ANCHORS[0]`, rotation 0 | `declare_identity=True` | 0.0 | 0.0 | -All 9 errors are inside the limits. A legacy payload loaded with no choice (no transform and no `declare_identity`) raised `InterpolatorFormatError` with reason `legacy_no_transform` for `fdi`, `pli` and `p2` (3 of 3). The text is: `legacy interpolator payload has no transform: pass local_origin and local_rotation, or declare_identity=True`. +All 9 errors are inside the limits. With C-018, a legacy payload loaded with no choice (no transform and no `declare_identity`) loads with the identity frame and emits one `FutureWarning` for `fdi`, `pli` and `p2` (3 of 3). The message starts: "legacy interpolator payload has no transform; assuming the identity frame". The loaded interpolator has no bounding box. For the rotated cell (`ANCHORS[3]`, rotation 30), the world values are NaN. This is the behaviour of releases before S3, and it is the reason for the warning. With `_STRICT_LEGACY` set to true, the call raises `legacy_no_transform` (the behaviour of the next major release). Check 2. The S3 code wrote `r1_interpolator("fdi", ANCHORS[3], 30.0, exact=True)[0].to_json()` (it has `format_version`). The old reader (`GeologicalInterpolator.from_json(text)` at `91ffca4a`) raised `TypeError` with this exact message: `InterpolatorFactory.create_interpolator() got an unexpected keyword argument 'format_version'`. The call returned no object: the result variable stayed unset (OP5). The error names `format_version`, as expected. @@ -428,8 +430,10 @@ in `_geological_feature_builder.py` (lines 541 and 542). No call of `interpolator.copy()` exists in the feature code. OP7 note. The P2 value-only fit of R1 misses SC-002 in every frame. In the -unrotated control the value error is 1.8e-8 and the gradient error is 3.9e-7, -against a gradient limit of 4e-9. The S3 rebuild test uses the exact nodal +unrotated control the value error is 7.8e-8 against a limit of 7.5e-8, and the +gradient error is 3.9e-7 against a limit of 1e-8. At 30 degrees the value +error is 2e-7 to 7.6e-7. FDI and PLI stay below 3.2e-9 (value) and 1.7e-9 +(gradient). Issue #399 records it. The S3 rebuild test uses the exact nodal control for `p2` (maintainer decision 2026-11-05). ## S3 test run time (OP2) @@ -441,3 +445,40 @@ control for `p2` (maintainer decision 2026-11-05). | Both S3 files | about 36 s test time | | Full root suite (orchestrator) | 797.81 s (1052.40 s at the S2 head; the two runs had different machine load, so the times do not compare) | | Package suites (orchestrator) | 108.69 s | + +## S3 change C-018 (backward-compatible legacy load) + +Date: 2026-11-05. + +Behaviour. The legacy load stays backward compatible on the 1.x line. A legacy +payload (no `format_version`) with no choice of the caller loads with the +identity frame, as older releases did. It emits one `FutureWarning`. The warning +names the choices and says that the next major release raises an error. The +switch `_STRICT_LEGACY` in `loop_interpolation._frame_persistence` makes the +call raise `legacy_no_transform`. A partial transform, a conflict, a malformed +payload and an unsupported version still raise `InterpolatorFormatError`. + +Tests (focused runs of the orchestrator): + +| Command | Result | +|---|---| +| `tests/unit/modelling/test_frame_persistence.py` | 177 passed | +| `test_frame_copy_and_lifecycle.py -k "not fault_restoration"` | 48 passed | +| packages `test_geological_interpolator.py` and `test_p2_fold_registration.py` | 43 passed | +| `ruff` | pass | + +The full suites are run again after this change (see the next section). + +## S3 final gates after C-018 + +The orchestrator ran the full gates on the working tree after the C-018 change (HEAD `e519fc0b` plus the change). + +| Command | Result | +|---|---| +| `ruff check LoopStructural packages` | All checks passed | +| `pytest packages/loop_common/tests packages/loop_interpolation/tests -p no:cacheprovider -q` | 912 passed, 47 skipped, 2 xfailed, 7 warnings in 100.44 s | +| Root `pytest -p no:cacheprovider -q`, full | 1987 passed, 5 skipped, 13 warnings in 826.94 s | + +Before C-018 the root suite gave 1981 passed. The difference of 6 comes from `test_frame_persistence.py`: the 3 cells of `test_legacy_payload_requires_choice` became 9 cells (the warn test, the identity-values test and the strict-switch test, 3 engines each). The file now has 177 cells. + +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. diff --git a/tests/unit/modelling/test_frame_persistence.py b/tests/unit/modelling/test_frame_persistence.py index e91a5cb13..7b1ffd684 100644 --- a/tests/unit/modelling/test_frame_persistence.py +++ b/tests/unit/modelling/test_frame_persistence.py @@ -5,6 +5,7 @@ import numpy as np import pytest from loop_interpolation import GeologicalInterpolator, InterpolatorFactory, InterpolatorFormatError +from loop_interpolation import _frame_persistence as frame_persistence from tests.fixtures.coordinate_frames import ( ANCHORS, @@ -148,8 +149,42 @@ def _assert_world_values(loaded, source, anchor): @pytest.mark.parametrize("engine", ENGINE_CELLS) -def test_legacy_payload_requires_choice(engine): - """A legacy payload with no choice raises `legacy_no_transform`.""" +def test_legacy_payload_without_choice_warns_and_loads_identity(engine): + """A legacy payload with no choice loads with the identity frame and warns once (C-018).""" + interp, _ = _build(engine, 1, 30.0) + legacy = strip_frame_keys(interp.to_dict()) + before = copy.deepcopy(legacy) + with pytest.warns( + FutureWarning, + match="^legacy interpolator payload has no transform; assuming the identity frame", + ) as record: + loaded = GeologicalInterpolator.from_dict(legacy) + future = [item for item in record if issubclass(item.category, FutureWarning)] + assert len(future) == 1 + message = str(future[0].message) + assert "local_origin" in message + assert "local_rotation" in message + assert "declare_identity=True" in message + assert "major release" in message + assert loaded.bounding_box is None + _assert_same(legacy, before) + assert record[0].filename == __file__ + + +@pytest.mark.parametrize("engine", ENGINE_CELLS) +def test_legacy_identity_values_match_source_for_identity_frame(engine): + """With an identity source frame, the identity fallback gives the source values.""" + interp, anchor = _build(engine, 0, 0.0) + legacy = strip_frame_keys(interp.to_dict()) + with pytest.warns(FutureWarning): + loaded = GeologicalInterpolator.from_dict(legacy) + _assert_world_values(loaded, interp, anchor) + + +@pytest.mark.parametrize("engine", ENGINE_CELLS) +def test_strict_legacy_switch_raises(engine, monkeypatch): + """With `_STRICT_LEGACY` on, a legacy payload with no choice raises `legacy_no_transform`.""" + monkeypatch.setattr(frame_persistence, "_STRICT_LEGACY", True) interp, _ = _build(engine, 1, 30.0) legacy = strip_frame_keys(interp.to_dict()) before = copy.deepcopy(legacy) From 2121b9bed424884dd1cdad2059087efec6d57b8e Mon Sep 17 00:00:00 2001 From: lachlangrose Date: Thu, 5 Nov 2026 14:15:57 +1030 Subject: [PATCH 2/3] fix(interpolation): point the legacy-load warning at the caller and warn only after a successful load The FutureWarning for a legacy payload with no choice used a fixed stacklevel. It pointed at a library file for from_json and from_yaml, and one frame too high for a direct factory call. It also came before a later failure of the same call. Add warn_legacy_identity, which computes the stacklevel to the first frame outside the package, and call it as the last step of a successful load. Add tests for the four entry points and for a failed load. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01XADRvJ98bJ8Ey8Rw98F3j1 --- .../loop_interpolation/_frame_persistence.py | 72 +++++++++++++------ .../_interpolator_factory.py | 10 ++- .../unit/modelling/test_frame_persistence.py | 36 ++++++++++ 3 files changed, 97 insertions(+), 21 deletions(-) diff --git a/packages/loop_interpolation/src/loop_interpolation/_frame_persistence.py b/packages/loop_interpolation/src/loop_interpolation/_frame_persistence.py index 97ab51b7b..d7ad7aaf5 100644 --- a/packages/loop_interpolation/src/loop_interpolation/_frame_persistence.py +++ b/packages/loop_interpolation/src/loop_interpolation/_frame_persistence.py @@ -7,6 +7,8 @@ from __future__ import annotations import itertools +import os +import sys import warnings from typing import Any @@ -39,8 +41,9 @@ """Valid reasons for an `InterpolatorFormatError`.""" # REMOVE AT THE NEXT MAJOR RELEASE (spec 005, C-018): delete the fallback in -# `payload_kind` and this constant. A legacy payload with no choice then always -# raises InterpolatorFormatError("legacy_no_transform", ...). +# `payload_kind` and this constant. Also delete `warn_legacy_identity` and +# `_external_stacklevel`. A legacy payload with no choice then always raises +# InterpolatorFormatError("legacy_no_transform", ...). _STRICT_LEGACY: bool = False _LEGACY_IDENTITY_WARNING = ( @@ -117,13 +120,51 @@ def frame_payload(bounding_box: BoundingBox | None, dimensions: int) -> dict: } +def _external_stacklevel() -> int: + """Count the frames of this package above `warn_legacy_identity`, plus 1. + + The count starts at the caller of `warn_legacy_identity` and stops at the + first frame outside this package. + + Returns + ------- + int + Number of package frames above `warn_legacy_identity`, plus 1. The + value is the `stacklevel` of the first outside frame, counted from + the caller of `warn_legacy_identity`. + """ + package_dir = os.path.dirname(os.path.abspath(__file__)) + # Frame 0 is this function. Frame 1 is `warn_legacy_identity`. + frame = sys._getframe(2) + level = 1 + while frame is not None and os.path.dirname(frame.f_code.co_filename) == package_dir: + level += 1 + frame = frame.f_back + return level + + +def warn_legacy_identity() -> None: + """Warn that a legacy payload loaded with the identity frame. + + The warning is attributed to the first frame outside the + `loop_interpolation` package, which is the code of the user. + + Warns + ----- + FutureWarning + Always. + """ + warnings.warn(_LEGACY_IDENTITY_WARNING, FutureWarning, stacklevel=_external_stacklevel() + 1) + + def payload_kind(payload: dict, caller_frame: bool) -> str: """Return the kind of a stored payload. On the 1.x line, a legacy payload (no `format_version`) with no choice of - the caller gives a `FutureWarning` and the kind `"legacy"`. The payload - then loads with the identity frame. If `_STRICT_LEGACY` is true, the - function raises an error in this case. + the caller gives the kind `"legacy"`. The payload then loads with the + identity frame. The function does not warn. The caller uses + `warn_legacy_identity` after the load succeeds. If `_STRICT_LEGACY` is + true, the function raises an error in this case. Parameters ---------- @@ -144,11 +185,6 @@ def payload_kind(payload: dict, caller_frame: bool) -> str: caller gives no frame (`legacy_no_transform`), if the version is above `FORMAT_VERSION` (`unsupported_version`), or if the payload is not valid (`malformed`). - - Warns - ----- - FutureWarning - If the payload has no transform and the caller gives no frame. """ version = payload.get("format_version") if version is None: @@ -158,16 +194,12 @@ def payload_kind(payload: dict, caller_frame: bool) -> str: "malformed interpolator payload: legacy payload has frame keys " "but no format_version", ) - if not caller_frame: - if _STRICT_LEGACY: - raise InterpolatorFormatError( - "legacy_no_transform", - "legacy interpolator payload has no transform: pass local_origin " - "and local_rotation, or declare_identity=True", - ) - # stacklevel 4: payload_kind <- InterpolatorFactory.from_dict - # <- GeologicalInterpolator.from_dict <- caller. - warnings.warn(_LEGACY_IDENTITY_WARNING, FutureWarning, stacklevel=4) + if not caller_frame and _STRICT_LEGACY: + raise InterpolatorFormatError( + "legacy_no_transform", + "legacy interpolator payload has no transform: pass local_origin " + "and local_rotation, or declare_identity=True", + ) return "legacy" if not isinstance(version, int) or isinstance(version, bool) or version < 1: raise InterpolatorFormatError( diff --git a/packages/loop_interpolation/src/loop_interpolation/_interpolator_factory.py b/packages/loop_interpolation/src/loop_interpolation/_interpolator_factory.py index ed0d4ea96..e53563040 100644 --- a/packages/loop_interpolation/src/loop_interpolation/_interpolator_factory.py +++ b/packages/loop_interpolation/src/loop_interpolation/_interpolator_factory.py @@ -19,7 +19,12 @@ interpolator_string_map, support_interpolator_map, ) -from ._frame_persistence import InterpolatorFormatError, payload_kind, resolve_frame +from ._frame_persistence import ( + InterpolatorFormatError, + payload_kind, + resolve_frame, + warn_legacy_identity, +) from ._validation import ValidationError @@ -117,6 +122,7 @@ def from_dict(d, *, local_origin=None, local_rotation=None, declare_identity=Fal "malformed", "frame arguments apply only to a saved payload" ) kind = payload_kind(d, caller_frame) if saved else None + legacy_no_choice = saved and kind == "legacy" and not caller_frame d = d.copy() frame_keys = {} if saved: @@ -206,6 +212,8 @@ def from_dict(d, *, local_origin=None, local_rotation=None, declare_identity=Fal interpolator.up_to_date = up_to_date interpolator.valid = valid + if legacy_no_choice: + warn_legacy_identity() return interpolator @staticmethod diff --git a/tests/unit/modelling/test_frame_persistence.py b/tests/unit/modelling/test_frame_persistence.py index 7b1ffd684..99d1b79ab 100644 --- a/tests/unit/modelling/test_frame_persistence.py +++ b/tests/unit/modelling/test_frame_persistence.py @@ -1,9 +1,12 @@ """Frame keys, world-value round trips and repeated save/load cycles (U3.1, U3.2, U3.8; SC-006).""" import copy +import json +import warnings import numpy as np import pytest +import yaml from loop_interpolation import GeologicalInterpolator, InterpolatorFactory, InterpolatorFormatError from loop_interpolation import _frame_persistence as frame_persistence @@ -171,6 +174,39 @@ def test_legacy_payload_without_choice_warns_and_loads_identity(engine): assert record[0].filename == __file__ +@pytest.mark.parametrize("entry", ["dict", "json", "yaml", "factory"]) +def test_legacy_warning_points_at_caller(entry): + """Each entry point attributes the one legacy warning to the calling file (C-018).""" + interp, _ = _build("fdi", 1, 30.0) + legacy = strip_frame_keys(interp.to_dict()) + text = json.dumps(legacy, default=lambda o: o.tolist()) + with warnings.catch_warnings(record=True) as rec: + warnings.simplefilter("always") + if entry == "dict": + GeologicalInterpolator.from_dict(legacy) + elif entry == "json": + GeologicalInterpolator.from_json(text) + elif entry == "yaml": + GeologicalInterpolator.from_yaml(yaml.safe_dump(json.loads(text))) + else: + InterpolatorFactory.from_dict(legacy) + assert [r.filename for r in rec if issubclass(r.category, FutureWarning)] == [__file__] + + +@pytest.mark.parametrize("engine", PERSIST_ENGINES) +def test_legacy_warning_not_emitted_when_load_fails(engine): + """A legacy load that fails gives its error and no warning (C-018).""" + interp, _ = _build(engine, 1, 30.0) + legacy = strip_frame_keys(interp.to_dict()) + legacy["data"]["value"] = [[1.0, 2.0, 3.0, 4.0, 1.0], [1.0]] + with warnings.catch_warnings(record=True) as rec: + warnings.simplefilter("always") + with pytest.raises(InterpolatorFormatError) as info: + GeologicalInterpolator.from_dict(legacy) + assert info.value.reason == "malformed" + assert not [r for r in rec if issubclass(r.category, FutureWarning)] + + @pytest.mark.parametrize("engine", ENGINE_CELLS) def test_legacy_identity_values_match_source_for_identity_frame(engine): """With an identity source frame, the identity fallback gives the source values.""" From a24bd46db2b4a3f6bc472f986347f430821bc7db Mon Sep 17 00:00:00 2001 From: lachlangrose Date: Thu, 5 Nov 2026 14:16:16 +1030 Subject: [PATCH 3/3] docs(spec): record the final gates after the C-018 review fix Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01XADRvJ98bJ8Ey8Rw98F3j1 --- .../run-state.json | 16 +++++++++------- .../verification.md | 12 +++++++----- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/specs/005-coordinate-frame-correctness/run-state.json b/specs/005-coordinate-frame-correctness/run-state.json index 6cdec689d..c7d17a0f7 100644 --- a/specs/005-coordinate-frame-correctness/run-state.json +++ b/specs/005-coordinate-frame-correctness/run-state.json @@ -251,10 +251,10 @@ ] }, "findings": { - "found": 25, + "found": 28, "rejected": 1, "dropped": 0, - "fixed": 6, + "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).", @@ -277,7 +277,8 @@ "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)." + "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": [ @@ -909,8 +910,8 @@ "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 100.44 s (after C-018)", - "test_root": "1987 passed, 5 skipped, 13 warnings in 826.94 s (after C-018)", + "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": [ @@ -921,8 +922,9 @@ "gates": { "lint": "pass", "packages": "912 passed, 47 skipped, 2 xfailed", - "root": "1987 passed, 5 skipped" - } + "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" } ] } diff --git a/specs/005-coordinate-frame-correctness/verification.md b/specs/005-coordinate-frame-correctness/verification.md index 07dbd0460..23a4c0ec6 100644 --- a/specs/005-coordinate-frame-correctness/verification.md +++ b/specs/005-coordinate-frame-correctness/verification.md @@ -462,7 +462,7 @@ Tests (focused runs of the orchestrator): | Command | Result | |---|---| -| `tests/unit/modelling/test_frame_persistence.py` | 177 passed | +| `tests/unit/modelling/test_frame_persistence.py` | 184 passed (after the review fix below) | | `test_frame_copy_and_lifecycle.py -k "not fault_restoration"` | 48 passed | | packages `test_geological_interpolator.py` and `test_p2_fold_registration.py` | 43 passed | | `ruff` | pass | @@ -471,14 +471,16 @@ The full suites are run again after this change (see the next section). ## S3 final gates after C-018 -The orchestrator ran the full gates on the working tree after the C-018 change (HEAD `e519fc0b` plus the change). +The orchestrator ran the full gates on the working tree after the C-018 change (HEAD `dddaff50` plus the review fix). | Command | Result | |---|---| | `ruff check LoopStructural packages` | All checks passed | -| `pytest packages/loop_common/tests packages/loop_interpolation/tests -p no:cacheprovider -q` | 912 passed, 47 skipped, 2 xfailed, 7 warnings in 100.44 s | -| Root `pytest -p no:cacheprovider -q`, full | 1987 passed, 5 skipped, 13 warnings in 826.94 s | +| `pytest packages/loop_common/tests packages/loop_interpolation/tests -p no:cacheprovider -q` | 912 passed, 47 skipped, 2 xfailed, 7 warnings in 103.85 s | +| Root `pytest -p no:cacheprovider -q`, full | 1994 passed, 5 skipped, 13 warnings in 817.74 s | -Before C-018 the root suite gave 1981 passed. The difference of 6 comes from `test_frame_persistence.py`: the 3 cells of `test_legacy_payload_requires_choice` became 9 cells (the warn test, the identity-values test and the strict-switch test, 3 engines each). The file now has 177 cells. +Before C-018 the root suite gave 1981 passed. The difference of 13 comes from `test_frame_persistence.py`: the 3 cells of `test_legacy_payload_requires_choice` became 9 cells (the warn test, the identity-values test and the strict-switch test, 3 engines each), and 7 cells came with the review fix below. The file now has 184 cells. + +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.