fix: keep the legacy interpolator load backward compatible until the next major release (spec 005 S3)

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

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

packages/loop_interpolation/src/loop_interpolation/_frame_persistence.py

@@ -7,6 +7,9 @@
 from __future__ import annotations
 
 import itertools
+import os
+import sys
+import warnings
 from typing import Any
 
 import numpy as np
@@ -37,6 +40,18 @@
 )
 """Valid reasons for an `InterpolatorFormatError`."""
 
+# REMOVE AT THE NEXT MAJOR RELEASE (spec 005, C-018): delete the fallback in
+# `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 = (
+    "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.
@@ -105,9 +120,52 @@ 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 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
     ----------
     payload : dict
@@ -123,9 +181,10 @@ 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`).
     """
     version = payload.get("format_version")
     if version is None:
@@ -135,7 +194,7 @@ 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 not caller_frame and _STRICT_LEGACY:
             raise InterpolatorFormatError(
                 "legacy_no_transform",
                 "legacy interpolator payload has no transform: pass local_origin "
@@ -237,19 +296,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 +335,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 +360,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:

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

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,21 @@
     "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 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"
+    }
+  ]
 }