From 24c75934e0b0e044fa8bbf365f77724212953681 Mon Sep 17 00:00:00 2001 From: rhaegar325 Date: Mon, 7 Sep 2026 22:36:09 +1000 Subject: [PATCH] pre-commit and code coverage fix --- src/access_moppy/atmosphere.py | 25 +++-- src/access_moppy/base.py | 62 +++++++++++ src/access_moppy/ocean.py | 5 + src/access_moppy/sea_ice.py | 5 + tests/unit/test_variable_attributes.py | 146 +++++++++++++++++++++++++ 5 files changed, 235 insertions(+), 8 deletions(-) diff --git a/src/access_moppy/atmosphere.py b/src/access_moppy/atmosphere.py index 6eaec9c2..09874985 100644 --- a/src/access_moppy/atmosphere.py +++ b/src/access_moppy/atmosphere.py @@ -431,15 +431,19 @@ def update_attributes(self): if not cmor_attrs.get("positive"): self.ds[self.cmor_name].attrs.pop("positive", None) - # Drop model-native attributes inherited from the source variable via the - # rename above that have no place in CMIP6 output: - # - grid_mapping: a regular lat-lon CMIP6 grid carries none, and its - # container variable is not carried into the output, so the attribute - # is a dangling reference that fails the CF grid-mapping check. - # - um_stash_source: a UM STASH provenance code, absent from the - # published reference. + # Drop a model-native attribute inherited from the source variable via + # the rename above that has no place in CMIP output: a regular lat-lon + # CMIP grid carries no grid_mapping, and its container variable is not + # carried into the output, so the attribute is a dangling reference + # that fails the CF grid-mapping check. It stays here rather than + # joining _MODEL_NATIVE_VARIABLE_ATTRIBUTES because a model on a + # projected grid would legitimately need it. + # + # um_stash_source used to be popped here too. It is now dropped by + # _drop_model_native_attributes at the end of this method, which also + # reaches the orog that model-level files carry as a formula_terms + # target — where this pop, scoped to cmor_name, never did. self.ds[self.cmor_name].attrs.pop("grid_mapping", None) - self.ds[self.cmor_name].attrs.pop("um_stash_source", None) # CMIP7 tables don't carry a per-variable "type" (unlike CMIP6), so # falling back to a hardcoded "double" here silently upcasts every @@ -611,3 +615,8 @@ def update_attributes(self): # CF-1.11 units_metadata for the temperature and time units, last so it # sees the final variable units and the normalized calendar. self._apply_units_metadata() + + # Last of all: strip the ACCESS-native attributes the raw files carry. + # After _check_calendar, which reads and rewrites calendar_type, and + # after every step above that sets attributes of its own. + self._drop_model_native_attributes() diff --git a/src/access_moppy/base.py b/src/access_moppy/base.py index 07a31fd1..3075cf57 100644 --- a/src/access_moppy/base.py +++ b/src/access_moppy/base.py @@ -1692,6 +1692,68 @@ def _drop_stale_range_attributes(self, cmor_attrs: Dict[str, Any]): if cmor_attrs.get(attr) in (None, ""): attrs.pop(attr, None) + #: Attributes the raw model output carries that describe how ACCESS wrote a + #: field, not what the field is. Unlike the CMOR table directives filtered + #: by :data:`_CMOR_VARIABLE_ATTRIBUTES`, these are already on the variable + #: by the time the vocabulary is applied — they arrive with the source file + #: — so an allowlist on the table entry cannot reach them and they have to + #: be named for removal. The set is empirical: it is every non-CF attribute + #: found on a variable across the 30,373 CMORised ACCESS-ESM1-6 files under + #: /scratch/p73/ESM1p6_CMORised, which covers all three components. + #: + #: * ``time_avg_info``, ``cartesian_axis``, ``calendar_type``, ``edges`` — + #: MOM5 + #: * ``time_rep`` — CICE5 + #: * ``um_stash_source``, ``um_version``, ``source`` — UM. ``source`` is a + #: legitimate *global* attribute (CMIP7 Global Attributes, Table 4); this + #: drops it only where UM put it, on a variable. + #: + #: A denylist rather than an allowlist because the attributes that must + #: survive are set all over the pipeline — ``coordinates``, + #: ``units_metadata``, ``computed_standard_name``, ``formula_terms``, + #: ``bounds`` — and a name missing from an allowlist would silently delete + #: mandatory CF metadata. This mirrors the choice already made in + #: :meth:`_drop_stale_range_attributes`. + _MODEL_NATIVE_VARIABLE_ATTRIBUTES = frozenset( + { + "cartesian_axis", + "calendar_type", + "edges", + "source", + "time_avg_info", + "time_rep", + "um_stash_source", + "um_version", + } + ) + + def _drop_model_native_attributes(self): + """Drop ACCESS-native attributes from every variable in the dataset. + + Two things distinguish this from the other attribute cleanups. + + It sweeps *every* variable, not just ``self.cmor_name``. The existing + per-variable pops reach only the data variable, which is why + ``um_stash_source`` — named for removal in + :meth:`Atmosphere_CMORiser.update_attributes` since before this — + still reached the published archive on the ``orog`` that model-level + files carry as a ``formula_terms`` target, and why ``calendar_type`` + survived on ``time``. + + It must run **after** :meth:`_check_calendar`. ``calendar_type`` is not + inert: MOM writes it instead of the CF ``calendar``, so + ``detect_time_frequency_lazy`` and ``_detect_frequency_from_bounds`` + read it to infer the calendar, and ``_check_calendar`` rewrites a + ``GREGORIAN`` value to ``proleptic_gregorian``. Both readers run during + ``select_and_process_variables``, well before this, but + ``_check_calendar`` runs inside ``update_attributes`` — so this belongs + at the end of that method, not at its start. + """ + for name in self.ds.variables: + attrs = self.ds[name].attrs + for attr in self._MODEL_NATIVE_VARIABLE_ATTRIBUTES: + attrs.pop(attr, None) + #: CF Appendix D — the standard name of the quantity each parametric #: vertical coordinate computes from its ``formula_terms``. Neither #: ``CMIP7_coordinate.json`` nor ``CMIP6_coordinate.json`` carries a diff --git a/src/access_moppy/ocean.py b/src/access_moppy/ocean.py index 43570db6..acc7a80c 100644 --- a/src/access_moppy/ocean.py +++ b/src/access_moppy/ocean.py @@ -497,6 +497,11 @@ def update_attributes(self): # sees the final variable units and the normalized calendar. self._apply_units_metadata() + # Last of all: strip the ACCESS-native attributes the raw files carry. + # After _check_calendar, which reads and rewrites calendar_type, and + # after every step above that sets attributes of its own. + self._drop_model_native_attributes() + class Ocean_CMORiser_OM2(Ocean_CMORiser): """CMORiser for ocean variables on the ACCESS-OM2 model using B-grid supergrid coordinates.""" diff --git a/src/access_moppy/sea_ice.py b/src/access_moppy/sea_ice.py index 7790b1a4..cfa24999 100644 --- a/src/access_moppy/sea_ice.py +++ b/src/access_moppy/sea_ice.py @@ -418,3 +418,8 @@ def update_attributes(self): # CF-1.11 units_metadata for the temperature and time units, last so it # sees the final variable units and the normalized calendar. self._apply_units_metadata() + + # Last of all: strip the ACCESS-native attributes the raw files carry. + # After _check_calendar, which reads and rewrites calendar_type, and + # after every step above that sets attributes of its own. + self._drop_model_native_attributes() diff --git a/tests/unit/test_variable_attributes.py b/tests/unit/test_variable_attributes.py index 4d87645a..a6c28e74 100644 --- a/tests/unit/test_variable_attributes.py +++ b/tests/unit/test_variable_attributes.py @@ -155,3 +155,149 @@ def test_empty_table_values_are_still_skipped(tmp_path): attrs = cmoriser.ds["tos"].attrs assert "comment" not in attrs assert "positive" not in attrs + + +#: What ACCESS writes on its own variables and CMIP does not want. Unlike the +#: table directives above these arrive with the source file, so the allowlist +#: applied to the table entry never sees them. +MODEL_NATIVE = { + "time_avg_info": "average_T1,average_T2,average_DT", # MOM5 + "time_rep": "averaged", # CICE5 + "cartesian_axis": "T", # MOM5 + "edges": "st_edges_ocean", # MOM5 + "um_stash_source": "m01s00i033", # UM + "um_version": "7.3", # UM + "source": "Unified Model", # UM +} + + +def _dataset_with_native_attrs(): + """A dataset shaped like a model-level file: data variable, coordinate, aux. + + ``orog`` stands in for the ``formula_terms`` target that hybrid-height + files carry. It is the case the old per-variable pops could not reach. + """ + ds = xr.Dataset( + { + "tos": xr.DataArray( + np.asarray([1.0, 2.0], dtype=np.float32), + dims=["time"], + coords={"time": xr.DataArray([0, 1], dims=["time"])}, + ), + "orog": xr.DataArray(np.asarray([0.0], dtype=np.float32), dims=["cell"]), + } + ) + ds["tos"].attrs.update( + { + "standard_name": "sea_surface_temperature", + "time_avg_info": MODEL_NATIVE["time_avg_info"], + } + ) + ds["time"].attrs.update( + {"axis": "T", "cartesian_axis": "T", "calendar_type": "GREGORIAN"} + ) + ds["orog"].attrs.update( + { + "standard_name": "surface_altitude", + "um_stash_source": MODEL_NATIVE["um_stash_source"], + "um_version": MODEL_NATIVE["um_version"], + "source": MODEL_NATIVE["source"], + } + ) + return ds + + +def _cmoriser_with_native_attrs(tmp_path): + cmoriser = _cmoriser(tmp_path, CMIP6_ENTRY) + cmoriser.ds = _dataset_with_native_attrs() + return cmoriser + + +@pytest.mark.unit +def test_model_native_attributes_are_dropped_from_the_data_variable(tmp_path): + """MOM's ``time_avg_info`` reached the published archive on every ocean variable.""" + cmoriser = _cmoriser_with_native_attrs(tmp_path) + + cmoriser._drop_model_native_attributes() + + assert "time_avg_info" not in cmoriser.ds["tos"].attrs + + +@pytest.mark.unit +def test_model_native_attributes_are_dropped_from_coordinates(tmp_path): + """``calendar_type``/``cartesian_axis`` sit on ``time``, never on the data variable. + + The pops that predate this method are scoped to ``self.cmor_name``, so + nothing reached a coordinate. + """ + cmoriser = _cmoriser_with_native_attrs(tmp_path) + + cmoriser._drop_model_native_attributes() + + attrs = cmoriser.ds["time"].attrs + assert "cartesian_axis" not in attrs + assert "calendar_type" not in attrs + + +@pytest.mark.unit +def test_model_native_attributes_are_dropped_from_auxiliary_variables(tmp_path): + """The ``orog`` a model-level file carries as a ``formula_terms`` target. + + ``um_stash_source`` was already named for removal in the atmosphere + CMORiser, yet 919 published files carry it here, because that pop only + ever looked at the data variable. + """ + cmoriser = _cmoriser_with_native_attrs(tmp_path) + + cmoriser._drop_model_native_attributes() + + attrs = cmoriser.ds["orog"].attrs + for name in ("um_stash_source", "um_version", "source"): + assert name not in attrs, f"{name} survived on an auxiliary variable" + + +@pytest.mark.unit +def test_describing_attributes_survive_the_sweep(tmp_path): + """A denylist, so nothing outside it may be touched on any variable.""" + cmoriser = _cmoriser_with_native_attrs(tmp_path) + + cmoriser._drop_model_native_attributes() + + assert cmoriser.ds["tos"].attrs["standard_name"] == "sea_surface_temperature" + assert cmoriser.ds["orog"].attrs["standard_name"] == "surface_altitude" + assert cmoriser.ds["time"].attrs["axis"] == "T" + + +@pytest.mark.unit +def test_calendar_type_is_read_before_it_is_dropped(tmp_path): + """Order matters: ``calendar_type`` is an input, not just noise. + + MOM writes it instead of the CF ``calendar``, and ``_check_calendar`` + rewrites a ``GREGORIAN`` value. Sweeping before that would take the + calendar with it, so the sweep belongs at the end of + ``update_attributes``. + """ + cmoriser = _cmoriser_with_native_attrs(tmp_path) + cmoriser.ds["time"].attrs.update( + {"calendar": "GREGORIAN", "units": "days since 0001-01-01"} + ) + + cmoriser._check_calendar("time") + cmoriser._drop_model_native_attributes() + + assert cmoriser.ds["time"].attrs["calendar"] == "proleptic_gregorian" + assert "calendar_type" not in cmoriser.ds["time"].attrs + + +@pytest.mark.unit +def test_global_source_attribute_is_untouched(tmp_path): + """``source`` is a legitimate CMIP7 global attribute (Global Attributes, Table 4). + + Only the variable-level ``source`` the UM writes is dropped. + """ + cmoriser = _cmoriser_with_native_attrs(tmp_path) + cmoriser.ds.attrs["source"] = "ACCESS-ESM1-6" + + cmoriser._drop_model_native_attributes() + + assert cmoriser.ds.attrs["source"] == "ACCESS-ESM1-6"