From 6c2d954131cf67fabf90292bb92f8f6ddf66f387 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Houpert?= <10154151+lhoupert@users.noreply.github.com> Date: Tue, 14 Jul 2026 13:33:35 +0100 Subject: [PATCH] refactor(s2): validate CRS/EPSG and bbox attrs via typed constructors (#180 parity) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the typed-CRS/bbox idiom from the S1 RTC review (9ebad3e) to the pre-existing S2/generic conversion code: - types.py: add EPSGCode NewType + make_epsg_code validating constructor, the integer-code sibling of CRSCode/make_crs_code. Accepts 32631, "32631" and "EPSG:32631" (the forms found in stored attrs and CPM 2.6.0 metadata); corrupt values fail loudly with a clear TypeError. - s2_converter.py: route the CPM-2.6.0 hand-parsing and the proj:epsg read through make_epsg_code; _as_bbox delegates to make_bounding_box (keeping its lenient skip-on-malformed walker semantics); the store-root bbox walker validates proj:code via make_crs_code and carries BoundingBox2D. - conversion/geozarr.py, s2_multiscale.py: proj:epsg / CPM-2.6.0 reads via make_epsg_code. - s1_ingest.py: BACKSCATTER_CF_ATTRS gains the Final annotation its sibling TIME_CF_ATTRS already has. - tests: make_epsg_code unit tests; test_write_geo_metadata_invalid_crs now expects the earlier, typed error (TypeError vs pyproj CRSError deeper in the stack) — the one intentional behavior change. No on-disk output changes. Full-project pyright: 0 errors; pre-commit green; full non-network suite passes. Co-Authored-By: Claude Fable 5 --- src/eopf_geozarr/conversion/geozarr.py | 8 ++-- src/eopf_geozarr/conversion/s1_ingest.py | 2 +- .../s2_optimization/s2_converter.py | 44 ++++++++++--------- .../s2_optimization/s2_multiscale.py | 3 +- src/eopf_geozarr/types.py | 21 +++++++++ tests/test_s2_multiscale_geo_metadata.py | 6 +-- tests/test_types.py | 25 ++++++++++- 7 files changed, 78 insertions(+), 31 deletions(-) diff --git a/src/eopf_geozarr/conversion/geozarr.py b/src/eopf_geozarr/conversion/geozarr.py index 3d7a6bdc..f0682508 100644 --- a/src/eopf_geozarr/conversion/geozarr.py +++ b/src/eopf_geozarr/conversion/geozarr.py @@ -44,6 +44,7 @@ StandardXCoordAttrsJSON, StandardYCoordAttrsJSON, XarrayEncodingJSON, + make_epsg_code, ) from . import fs_utils, utils @@ -177,11 +178,10 @@ def setup_datatree_metadata_geozarr_spec_compliant( """ geozarr_groups: dict[str, xr.Dataset] = {} grid_mapping_var_name = "spatial_ref" - epsg_CPM_260 = dt.attrs.get("other_metadata", {}).get( + raw_epsg_cpm_260 = dt.attrs.get("other_metadata", {}).get( "horizontal_CRS_code", dt.attrs.get("other_metadata", {}).get("horizontal_crs_code", None) ) - if epsg_CPM_260 is not None: - epsg_CPM_260 = epsg_CPM_260.split(":")[-1] + epsg_CPM_260 = make_epsg_code(raw_epsg_cpm_260) if raw_epsg_cpm_260 is not None else None for key in groups: # Check if key exists in DataTree by attempting to access it @@ -234,7 +234,7 @@ def setup_datatree_metadata_geozarr_spec_compliant( # Set CRS if available if "proj:epsg" in ds[var_name].attrs: - epsg = ds[var_name].attrs["proj:epsg"] + epsg = make_epsg_code(ds[var_name].attrs["proj:epsg"]) log.info("Setting CRS for variable %s to EPSG %s", var_name, epsg) ds = ds.rio.write_crs(f"epsg:{epsg}") elif epsg_CPM_260: diff --git a/src/eopf_geozarr/conversion/s1_ingest.py b/src/eopf_geozarr/conversion/s1_ingest.py index 4c09565a..eac53a5d 100644 --- a/src/eopf_geozarr/conversion/s1_ingest.py +++ b/src/eopf_geozarr/conversion/s1_ingest.py @@ -359,7 +359,7 @@ def _create_spatial_coordinate_arrays( # zarr-direct, so the attribute must be set explicitly. FLOAT32_NAN_FILL_VALUE = FillValueCoder.encode(np.nan, np.dtype("float32")) # CF metadata for the backscatter bands (vv/vh). -BACKSCATTER_CF_ATTRS: S1BackscatterAttrsJSON = { +BACKSCATTER_CF_ATTRS: Final[S1BackscatterAttrsJSON] = { "standard_name": "surface_backwards_scattering_coefficient_of_radar_wave", "units": "1", "_FillValue": FLOAT32_NAN_FILL_VALUE, diff --git a/src/eopf_geozarr/s2_optimization/s2_converter.py b/src/eopf_geozarr/s2_optimization/s2_converter.py index 6a868087..2c0c982b 100644 --- a/src/eopf_geozarr/s2_optimization/s2_converter.py +++ b/src/eopf_geozarr/s2_optimization/s2_converter.py @@ -17,6 +17,12 @@ from eopf_geozarr.conversion.geozarr import get_zarr_group from eopf_geozarr.data_api.s1 import Sentinel1Root from eopf_geozarr.data_api.s2 import Sentinel2Root +from eopf_geozarr.types import ( + BoundingBox2D, + make_bounding_box, + make_crs_code, + make_epsg_code, +) from .s2_multiscale import create_multiscale_from_datatree @@ -43,13 +49,8 @@ def initialize_crs_from_dataset(dt_input: xr.DataTree) -> CRS | None: ) if epsg_cpm_260 is not None: try: - # Handle both integer (32632) and string ("EPSG:32632" or "32632") formats - if isinstance(epsg_cpm_260, str): - # Extract numeric part from string like "EPSG:32632" or "32632" - epsg_code = int(epsg_cpm_260.split(":")[-1]) - else: - # Already an integer - epsg_code = int(epsg_cpm_260) + # Handles both integer (32632) and string ("EPSG:32632" or "32632") formats + epsg_code = make_epsg_code(epsg_cpm_260) crs = CRS.from_epsg(epsg_code) log.info("Initialized CRS from CPM 2.6.0+ metadata", epsg=epsg_code) except Exception as e: @@ -97,7 +98,7 @@ def initialize_crs_from_dataset(dt_input: xr.DataTree) -> CRS | None: # Check for proj:epsg attribute if "proj:epsg" in var.attrs: try: - epsg = var.attrs["proj:epsg"] + epsg = make_epsg_code(var.attrs["proj:epsg"]) crs = CRS.from_epsg(epsg) log.info("Initialized CRS from EPSG code", epsg=epsg) except Exception: @@ -325,17 +326,17 @@ def simple_root_consolidation(output_path: str, datasets: Mapping[str, object]) zarr.consolidate_metadata(output_path, zarr_format=3) -def _as_bbox(value: object) -> tuple[float, float, float, float] | None: - """Return *value* as a 4-tuple of floats, or ``None`` if it is not one. +def _as_bbox(value: object) -> BoundingBox2D | None: + """Return *value* as a validated bounding box, or ``None`` if it is not one. ``spatial:bbox`` is read from stored metadata, so its type is not known - statically; this verifies the shape at runtime rather than asserting it. + statically; unlike a bare ``make_bounding_box`` call, the store walker + tolerates absent or malformed values by skipping the group. """ - if not isinstance(value, (list, tuple)) or len(value) != 4: - return None - if not all(isinstance(v, (int, float)) for v in value): + try: + return make_bounding_box(value) + except (TypeError, ValueError): return None - return (float(value[0]), float(value[1]), float(value[2]), float(value[3])) def write_store_root_bbox(output_path: str) -> None: @@ -350,24 +351,27 @@ def write_store_root_bbox(output_path: str) -> None: root = zarr.open_group(output_path, mode="r+") - bboxes_4326: list[tuple[float, float, float, float]] = [] + bboxes_4326: list[BoundingBox2D] = [] def _walk(group: zarr.Group) -> None: attrs = dict(group.attrs) bbox = attrs.get("spatial:bbox") - code = attrs.get("proj:code") + raw_code = attrs.get("proj:code") # spatial:bbox comes from stored metadata; verify it is a 4-element # numeric sequence before use rather than trusting the type. corners = _as_bbox(bbox) if corners is not None: x0, y0, x1, y1 = corners - if code and code != "EPSG:4326": + # A group with a bbox but no proj:code is treated as already EPSG:4326; + # a present-but-malformed code fails loudly rather than being skipped. + code = make_crs_code(raw_code) if raw_code is not None else None + if code is not None and code != "EPSG:4326": transformer = Transformer.from_crs(code, "EPSG:4326", always_xy=True) xmin, ymin = transformer.transform(x0, y0) xmax, ymax = transformer.transform(x1, y1) - bboxes_4326.append((xmin, ymin, xmax, ymax)) + bboxes_4326.append(BoundingBox2D((xmin, ymin, xmax, ymax))) else: - bboxes_4326.append((x0, y0, x1, y1)) + bboxes_4326.append(corners) for child in group.groups(): _walk(child[1]) diff --git a/src/eopf_geozarr/s2_optimization/s2_multiscale.py b/src/eopf_geozarr/s2_optimization/s2_multiscale.py index a79a9832..683936fe 100644 --- a/src/eopf_geozarr/s2_optimization/s2_multiscale.py +++ b/src/eopf_geozarr/s2_optimization/s2_multiscale.py @@ -31,6 +31,7 @@ ) from eopf_geozarr.s2_optimization.common import DISTRIBUTED_AVAILABLE from eopf_geozarr.s2_optimization.s2_band_mapping import BAND_INFO +from eopf_geozarr.types import make_epsg_code from .s2_resampling import determine_variable_type, downsample_variable @@ -1150,7 +1151,7 @@ def write_geo_metadata( crs = var.rio.crs break if "proj:epsg" in var.attrs: - epsg = var.attrs["proj:epsg"] + epsg = make_epsg_code(var.attrs["proj:epsg"]) crs = CRS.from_epsg(epsg) break diff --git a/src/eopf_geozarr/types.py b/src/eopf_geozarr/types.py index 373118ea..88e65b7b 100644 --- a/src/eopf_geozarr/types.py +++ b/src/eopf_geozarr/types.py @@ -21,6 +21,27 @@ def make_crs_code(value: object) -> CRSCode: return CRSCode(value) +EPSGCode = NewType("EPSGCode", int) +"""A bare EPSG integer code, e.g. ``32631`` (the legacy ``proj:epsg`` attribute value).""" + + +def make_epsg_code(value: object) -> EPSGCode: + """Validate an untyped value (e.g. a zarr attribute) as an EPSG integer code. + + Accepts an int (``32631``) or the string forms found in stored attrs and CPM + metadata (``"32631"``, ``"EPSG:32631"``). Validation is intentionally light: + pyproj's ``CRS.from_epsg`` raises ``CRSError`` downstream on codes it does + not recognize. + """ + if isinstance(value, int) and not isinstance(value, bool): + return EPSGCode(value) + if isinstance(value, str): + tail = value.strip().split(":")[-1] + if tail.isdigit(): + return EPSGCode(int(tail)) + raise TypeError(f"EPSG code must be an int or 'EPSG:' string, got {value!r}") + + def make_bounding_box(value: object) -> BoundingBox2D: """Validate an untyped value (e.g. a zarr attribute) as a 4-number bounding box. diff --git a/tests/test_s2_multiscale_geo_metadata.py b/tests/test_s2_multiscale_geo_metadata.py index d465bdc8..26396572 100644 --- a/tests/test_s2_multiscale_geo_metadata.py +++ b/tests/test_s2_multiscale_geo_metadata.py @@ -288,10 +288,8 @@ def test_write_geo_metadata_invalid_crs( # Add invalid EPSG code ds["test_var"].attrs["proj:epsg"] = "invalid_epsg" - # Method should raise an exception for invalid CRS (normal behavior) - from pyproj.exceptions import CRSError - - with pytest.raises(CRSError): + # make_epsg_code rejects the corrupt attr before it reaches pyproj + with pytest.raises(TypeError, match="EPSG code"): write_geo_metadata(ds) def test_write_geo_metadata_mixed_crs_variables( diff --git a/tests/test_types.py b/tests/test_types.py index bb2e1a01..d5d5c2a5 100644 --- a/tests/test_types.py +++ b/tests/test_types.py @@ -8,7 +8,7 @@ import pytest -from eopf_geozarr.types import make_bounding_box, make_crs_code +from eopf_geozarr.types import make_bounding_box, make_crs_code, make_epsg_code # ============================================================================= # make_crs_code @@ -31,6 +31,29 @@ def test_crs_code_rejects_empty_or_whitespace(bad: str) -> None: make_crs_code(bad) +# ============================================================================= +# make_epsg_code +# ============================================================================= + + +def test_epsg_code_accepts_int() -> None: + assert make_epsg_code(32631) == 32631 + + +@pytest.mark.parametrize("value", ["32631", "EPSG:32631", " EPSG:32631 "]) +def test_epsg_code_accepts_string_forms(value: str) -> None: + # Stored attrs and CPM metadata carry EPSG codes as "32631" or "EPSG:32631". + code = make_epsg_code(value) + assert code == 32631 + assert isinstance(code, int) + + +@pytest.mark.parametrize("bad", [None, True, 32631.0, ["EPSG:32631"], "EPSG:", "EPSG:abc", ""]) +def test_epsg_code_rejects_non_codes(bad: object) -> None: + with pytest.raises(TypeError, match="EPSG code"): + make_epsg_code(bad) + + # ============================================================================= # make_bounding_box # =============================================================================