diff --git a/dascore/core/attrs.py b/dascore/core/attrs.py index 5baf0250e..581ce0d5d 100644 --- a/dascore/core/attrs.py +++ b/dascore/core/attrs.py @@ -13,7 +13,6 @@ VALID_DATA_TYPES, max_lens, ) -from dascore.utils.attrs import _raise_if_coord_attr_updates from dascore.utils.misc import ( to_str, ) @@ -148,7 +147,6 @@ def from_dict( def update(self, **kwargs) -> Self: """Update an attribute in the model, return new model.""" - _raise_if_coord_attr_updates(kwargs) out = self.model_dump(exclude_unset=True) out.update(kwargs) return self.from_dict(out) diff --git a/dascore/core/coordmanager.py b/dascore/core/coordmanager.py index 8d5517d5d..03a395d73 100644 --- a/dascore/core/coordmanager.py +++ b/dascore/core/coordmanager.py @@ -44,14 +44,13 @@ from collections import defaultdict from collections.abc import Mapping, Sequence from itertools import zip_longest -from typing import Annotated, Any, TypeVar +from typing import Annotated, TypeVar import numpy as np from pydantic import field_validator, model_validator from rich.text import Text from typing_extensions import Self -import dascore as dc from dascore.constants import dascore_styles, select_values_description from dascore.core.coords import BaseCoord, CoordSummary, get_coord from dascore.exceptions import ( @@ -61,7 +60,6 @@ ParameterError, PatchBroadcastError, ) -from dascore.utils.attrs import separate_coord_info from dascore.utils.docs import compose_docstring from dascore.utils.mapping import FrozenDict from dascore.utils.misc import ( @@ -306,35 +304,6 @@ def _divide_kwargs(kwargs): # we need this here to maintain backwards compatibility update_coords = update - def update_from_attrs( - self, attrs: Mapping | dc.PatchAttrs - ) -> tuple[Self, dc.PatchAttrs]: - """ - Update coordinates from attrs. - - This will also return a PatchAttrs which conforms to coords. - - Parameters - ---------- - attrs - The attribute source, either PatchAttrs instance or mapping. - """ - coord_info, attr_info = separate_coord_info(attrs, dims=self.dims) - out = dict(self.coord_map) - for name in set(coord_info) & set(out): - maybe_updates = coord_info[name] - coord = self.coord_map[name] - # convert values to dict to determine which should be updated. - model_contents = coord.to_summary().model_dump(exclude_defaults=True) - # see what has changed. - diff = { - i: v for i, v in maybe_updates.items() if v != model_contents.get(i) - } - out[name] = coord.update(**diff) - coords = self.new(coord_map=out) - attrs = dc.PatchAttrs.from_dict(attr_info) - return coords, attrs - def sort( self, *coords, array: MaybeArray = None, reverse: bool = False ) -> tuple[Self, MaybeArray]: @@ -1147,7 +1116,6 @@ def _flip_coord(coord, axis): def get_coord_manager( coords: CoordManagerInput | CoordManager | None = None, dims: tuple[str, ...] | None = None, - attrs: dc.PatchAttrs | dict[str, Any] | None = None, shape=None, ) -> CoordManager: """ @@ -1162,11 +1130,6 @@ def get_coord_manager( [`CoordManager`](`dascore.core.CoordManager`). dims Tuple specify dimension names - attrs - Attributes which can be used to create coordinates. - Cannot be used with coords argument. - If you want to update [`CoordManager`](`dascore.core.CoordManager`) - use [`update_from_attrs`](`dascore.core.CoordManager.update_from_attrs`). shape The data array shape which will be managed by coord manager. This allows non-coordinate dimensions to be initiated. @@ -1193,12 +1156,6 @@ def get_coord_manager( >>> coords['quality'] = (("distance", "time"), quality) >>> cm = get_coord_manager(coords=coords, dims=dims) """ - if coords is not None and attrs is not None: - msg = ( - "Cannot use both attrs and coords in get_coord_manager. " - "Perhaps you want CoordManager.update_from_attrs?" - ) - raise ParameterError(msg) # return coords if we already have a coord manager. if isinstance(coords, CoordManager): # maybe try to rename dims. @@ -1220,12 +1177,6 @@ def get_coord_manager( for name in missing_dims: coord_map[name] = get_coord(shape=shape[dims.index(name)]) dim_map[name] = (name,) - if attrs: - coord_updates, _ = separate_coord_info(attrs, dims) - updateable_coords = set(coord_updates) - set(coord_map) - for name in updateable_coords: - coord_map[name] = get_coord(**coord_updates[name]) - dim_map[name] = (name,) out = CoordManager(coord_map=coord_map, dim_map=dim_map, dims=dims) return out diff --git a/dascore/core/coords.py b/dascore/core/coords.py index a4bdcc705..6fedda8bd 100644 --- a/dascore/core/coords.py +++ b/dascore/core/coords.py @@ -897,15 +897,6 @@ def index(self, indexer, axis: int | None = None) -> Self: array = self.data[indexer] return get_coord(data=array, units=self.units) - def get_attrs_dict(self, name): - """Get attrs dict.""" - out = {f"{name}_min": self.min(), f"{name}_max": self.max()} - if self.step: - out[f"{name}_step"] = self.step - if self.units: - out[f"{name}_units"] = self.units - return out - def to_summary(self, dims=()) -> CoordSummary: """Get the summary info about the coord.""" return CoordSummary( diff --git a/dascore/io/dasdae/_compat.py b/dascore/io/dasdae/_compat.py new file mode 100644 index 000000000..bd14fccbd --- /dev/null +++ b/dascore/io/dasdae/_compat.py @@ -0,0 +1,103 @@ +""" +Backward compatibility for legacy DASDAE metadata. + +Older DASDAE files mixed coordinate metadata into the patch attr namespace: +coord summaries were stored as a (sometimes pickled) ``coords`` attr, and +flat keys such as ``time_min`` or ``d_time`` lived alongside true patch +attrs. Files written after attrs and coords were fully separated carry the +``__attrs_coords_separate__`` root marker and store only true attrs. + +This module is private to ``dascore.io.dasdae``; nothing else in DASCore may +import it. Attrs and coords are independent everywhere else, so all knowledge +of the old mixed shapes is quarantined here. +""" + +from __future__ import annotations + +import contextlib +import pickle +from collections.abc import Iterable + +from dascore.config import get_config +from dascore.core.coords import CoordSummary +from dascore.exceptions import InvalidFiberFileError + +# Every flat coord-summary key an old file may contain ({name}_{field}). +_LEGACY_COORD_FIELDS = tuple(CoordSummary.model_fields) +# The subset legacy writers actually flattened into attrs; dims/fingerprint +# never appeared as flat keys, so translate re-emits only these while the +# strip above removes the full (superset) field family. +_LEGACY_FLAT_FIELDS = ("min", "max", "step", "units", "dtype", "len") + + +def strip_legacy_coord_fields(attrs: dict, coord_names: Iterable[str]) -> dict: + """ + Remove legacy flat coordinate metadata from an attr mapping. + + Only exact ``{name}_{field}`` compositions for the provided coord names + are removed (plus the deprecated ``d_{name}`` spelling and the structural + ``coords``/``dims`` keys). Nothing is inferred from key shape, so a true + attr like ``pulse_len`` survives unless the file really stores a ``pulse`` + coordinate. + """ + out = dict(attrs) + out.pop("coords", None) + out.pop("dims", None) + for name in coord_names: + for field in _LEGACY_COORD_FIELDS: + out.pop(f"{name}_{field}", None) + out.pop(f"d_{name}", None) + return out + + +def translate_legacy_attrs(attrs): + """Normalize legacy DASDAE attr payloads to flat coord metadata.""" + out = dict(attrs) + coords = out.pop("coords", {}) + if isinstance(coords, str): + # Older DASDAE files stored the coord-summary payload as a pickled + # string attr. Unpickling runs arbitrary code, so the opt-in gate + # must come before any decode attempt — a malicious payload executes + # during pickle.loads itself, not when the result is used. + if not get_config().allow_dasdae_format_unpickle: + msg = ( + "This DASDAE file contains legacy pickled coordinate metadata. " + "Unpickling DASDAE format metadata is disabled by default for " + "security. If you trust this file, enable legacy compatibility " + "with dc.set_config(allow_dasdae_format_unpickle=True)." + ) + raise InvalidFiberFileError(msg) + with contextlib.suppress( + AttributeError, + EOFError, + KeyError, + pickle.PickleError, + TypeError, + UnicodeError, + ValueError, + ): + coords = pickle.loads(coords.encode("latin1")) + if hasattr(coords, "to_summary_dict"): + coords = coords.to_summary_dict() + if not hasattr(coords, "items"): + coords = {} + for name, summary in coords.items(): + if hasattr(summary, "to_summary"): + summary = summary.to_summary() + if hasattr(summary, "model_dump"): + summary = summary.model_dump() + if not isinstance(summary, dict): + continue + for field in _LEGACY_FLAT_FIELDS: + key = f"{name}_{field}" + value = summary.get(field) + if key not in out and value not in (None, ""): + out[key] = value + dims = out.get("dims", "") + dims = tuple(dims.split(",")) if isinstance(dims, str) else tuple(dims or ()) + for name in dims: + old_name = f"d_{name}" + new_name = f"{name}_step" + if new_name not in out and old_name in out: + out[new_name] = out.pop(old_name) + return out diff --git a/dascore/io/dasdae/core.py b/dascore/io/dasdae/core.py index ed32967bc..ae090e9bf 100644 --- a/dascore/io/dasdae/core.py +++ b/dascore/io/dasdae/core.py @@ -15,8 +15,10 @@ from dascore.utils.patch import get_patch_names from .utils import ( - _get_attrs, _get_contents_from_patch_groups_generic, + _get_patch_attrs, + _is_legacy_file, + _is_legacy_group, _kwargs_empty, _matches_attr_filters, _read_patch, @@ -118,14 +120,16 @@ def read(self, resource: H5Reader, source_patch_id=(), **kwargs) -> SpoolType: waveform_group = resource["waveforms"] except (KeyError, IndexError): return dc.spool([]) + file_legacy = _is_legacy_file(resource) for patch_group in waveform_group.values(): patch_name = str(patch_group.name).rsplit("/", maxsplit=1)[-1] if source_patch_ids and patch_name not in source_patch_ids: continue - attrs = _get_attrs(patch_group) + legacy = _is_legacy_group(patch_group, file_legacy) + attrs = _get_patch_attrs(patch_group, legacy) if not _matches_attr_filters(attrs, kwargs): continue - patch = _read_patch(patch_group, **kwargs) + patch = _read_patch(patch_group, legacy=legacy, **kwargs) if not patch.data.size and not _kwargs_empty(kwargs): continue patches.append(patch) diff --git a/dascore/io/dasdae/utils.py b/dascore/io/dasdae/utils.py index c184c43f6..7294910f4 100644 --- a/dascore/io/dasdae/utils.py +++ b/dascore/io/dasdae/utils.py @@ -6,26 +6,22 @@ from __future__ import annotations -import contextlib import json -import pickle import numpy as np import pandas as pd import dascore as dc -from dascore.config import get_config from dascore.core.attrs import PatchAttrs from dascore.core.coordmanager import get_coord_manager from dascore.core.coords import get_coord -from dascore.exceptions import InvalidFiberFileError from dascore.io.core import _make_scan_payload +from dascore.io.dasdae._compat import strip_legacy_coord_fields, translate_legacy_attrs from dascore.utils.array import ( convert_bytes_to_strings, convert_strings_to_bytes, is_string_byte_serializable_array, ) -from dascore.utils.attrs import separate_coord_info from dascore.utils.misc import unbyte from dascore.utils.pd import filter_df from dascore.utils.time import to_int @@ -34,6 +30,9 @@ _KWARG_NON_KEYS = {"file_version", "file_format", "path", "source_patch_id"} _ATTR_PREFIX = "_attrs_" _ATTR_TYPE_PREFIX = "_attr_type_" +# Root marker set on files whose patch attr namespace holds only true attrs. +# Files without it may mix flat coord metadata into attrs (see _compat). +_SEPARATE_ATTRS_KEY = "__attrs_coords_separate__" # --- Functions for writing DASDAE format @@ -44,6 +43,40 @@ def _write_meta(hfile, file_version): hfile.attrs["__format__"] = "DASDAE" hfile.attrs["__DASDAE_version__"] = file_version hfile.attrs["__dascore__version__"] = dc.__version__ + # Mark the file as holding only true attrs (no flat coord metadata), + # unless appending to a legacy file that already contains mixed patches. + waveforms = hfile.get("waveforms") + has_legacy_patches = ( + waveforms is not None + and len(waveforms) + and not hfile.attrs.get(_SEPARATE_ATTRS_KEY, False) + ) + if not has_legacy_patches: + hfile.attrs[_SEPARATE_ATTRS_KEY] = True + + +def _is_legacy_file(h5) -> bool: + """Return True if the file may mix flat coord metadata into patch attrs.""" + return not h5.attrs.get(_SEPARATE_ATTRS_KEY, False) + + +def _is_legacy_group(patch_group, file_legacy: bool) -> bool: + """ + Return True if a patch group may mix flat coord metadata into attrs. + + New patch groups appended to a legacy file carry their own marker, so + they keep exact attr round-trips even though the file stays unmarked. + """ + return file_legacy and not patch_group.attrs.get(_SEPARATE_ATTRS_KEY, False) + + +def _get_group_coord_names(patch_group) -> set[str]: + """Get names of all dims/coords stored in a patch group.""" + names = set(_get_dims(patch_group)) + for key in patch_group: + if key.startswith("_coord_"): + names.add(key.removeprefix("_coord_")) + return names def _save_attrs_and_dims(patch, patch_group): @@ -109,6 +142,9 @@ def _save_patch(patch, wave_group, name): # Replace the entire patch group so stale datasets/attrs can't survive. del wave_group[name] patch_group = wave_group.create_group(name) + # Per-group marker: groups appended to a legacy file are still written + # in the separated-attrs form and must not be legacy-stripped on read. + patch_group.attrs[_SEPARATE_ATTRS_KEY] = True _save_attrs_and_dims(patch, patch_group) _save_coords(patch, patch_group) # add data @@ -161,62 +197,6 @@ def _read_array_sample(table_array, index): return out -def _translate_legacy_attrs(attrs): - """Normalize legacy DASDAE attr payloads to flat coord metadata.""" - out = dict(attrs) - coords = out.pop("coords", {}) - if isinstance(coords, str): - # Older DASDAE files stored the coord-summary payload as a pickled - # string attr. Decode only this legacy coord metadata so scan/read can - # recover units and steps without reviving general legacy attr unpickling. - with contextlib.suppress( - AttributeError, - EOFError, - KeyError, - pickle.PickleError, - TypeError, - UnicodeError, - ValueError, - ): - decoded = pickle.loads(coords.encode("latin1")) - if ( - hasattr(decoded, "items") - and not get_config().allow_dasdae_format_unpickle - ): - msg = ( - "This DASDAE file contains legacy pickled coordinate metadata. " - "Unpickling DASDAE format metadata is disabled by default for " - "security. If you trust this file, enable legacy compatibility " - "with dc.set_config(allow_dasdae_format_unpickle=True)." - ) - raise InvalidFiberFileError(msg) - coords = decoded - if hasattr(coords, "to_summary_dict"): - coords = coords.to_summary_dict() - if not hasattr(coords, "items"): - coords = {} - for name, summary in coords.items(): - if hasattr(summary, "to_summary"): - summary = summary.to_summary() - if hasattr(summary, "model_dump"): - summary = summary.model_dump() - if not isinstance(summary, dict): - continue - for field in ("min", "max", "step", "units", "dtype", "len"): - key = f"{name}_{field}" - value = summary.get(field) - if key not in out and value not in (None, ""): - out[key] = value - dims = out.get("dims", "") - dims = tuple(dims.split(",")) if isinstance(dims, str) else tuple(dims or ()) - for name in dims: - old_name = f"d_{name}" - new_name = f"{name}_step" - if new_name not in out and old_name in out: - out[new_name] = out.pop(old_name) - return out - - def _get_coords(patch_group, dims, attrs2): """Get the coordinates from a patch group.""" coord_dict = {} # just store coordinates here @@ -286,18 +266,33 @@ def is_nullish(value): } if not query: return True - attrs = _translate_legacy_attrs(attrs) - _, attr_info = separate_coord_info(attrs, dims=attrs.get("dims", ())) - attr_df = pd.DataFrame([attr_info]) + attr_df = pd.DataFrame([attrs]) return bool(filter_df(attr_df, ignore_bad_kwargs=True, **query)[0]) -def _read_patch(patch_group, attrs=None, **kwargs): +def _get_patch_attrs(patch_group, legacy: bool) -> dict: + """Get the true patch attrs, cleaning legacy coord metadata if needed.""" + attrs = _get_attrs(patch_group) + if legacy: + dims = _get_dims(patch_group) + attrs["dims"] = ",".join(dims) + attrs = translate_legacy_attrs(attrs) + attrs = strip_legacy_coord_fields(attrs, _get_group_coord_names(patch_group)) + return attrs + + +def _read_patch(patch_group, legacy: bool = True, **kwargs): """Read a patch group, return Patch.""" - attrs = _translate_legacy_attrs(_get_attrs(patch_group)) if attrs is None else attrs + attrs = _get_attrs(patch_group) dims = _get_dims(patch_group) - coords = _get_coords(patch_group, dims, attrs) - _, attr_info = separate_coord_info(attrs, dims=dims) + if legacy: + attrs["dims"] = ",".join(dims) + attrs = translate_legacy_attrs(attrs) + coords = _get_coords(patch_group, dims, attrs) + attr_info = strip_legacy_coord_fields(attrs, set(coords.coord_map) | set(dims)) + else: + coords = _get_coords(patch_group, dims, {}) + attr_info = attrs attr_info["_source_patch_id"] = patch_group.name.rsplit("/", maxsplit=1)[-1] attrs = PatchAttrs.from_dict(attr_info) # Note, previously this was wrapped with try, except (Index, KeyError) @@ -333,7 +328,7 @@ def _kwargs_empty(kwargs) -> bool: return not bool(out) -def _get_scan_payload_from_group(group): +def _get_scan_payload_from_group(group, legacy: bool = True): """Build one structured scan payload from a stored DASDAE patch group.""" attrs = group.attrs out = {} @@ -347,14 +342,15 @@ def _get_scan_payload_from_group(group): if isinstance(value, np.ndarray) and not value.shape: value = np.atleast_1d(value)[0] out[new_key] = unbyte(value) - # rename dims - out["dims"] = unbyte(attrs["_dims"]) - out = _translate_legacy_attrs(out) - dims_str = out["dims"] - dims = tuple(dims_str.split(",")) if dims_str else () - # Split flattened coord metadata from the remaining patch attrs. - _, attr_info = separate_coord_info(out, dims=dims) - coords = _get_coords(group, dims, out) + dims = _get_dims(group) + if legacy: + out["dims"] = ",".join(dims) + out = translate_legacy_attrs(out) + coords = _get_coords(group, dims, out) + attr_info = strip_legacy_coord_fields(out, set(coords.coord_map) | set(dims)) + else: + coords = _get_coords(group, dims, {}) + attr_info = out # Data shape/dtype come from the stored data node without loading the array. data_node = group.get("data") dtype = str(data_node.dtype) if data_node is not None else "" @@ -433,4 +429,8 @@ def _get_contents_from_patch_groups_generic(h5): waveforms = h5.get("waveforms") if waveforms is None: return [] - return [_get_scan_payload_from_group(group) for group in waveforms.values()] + file_legacy = _is_legacy_file(h5) + return [ + _get_scan_payload_from_group(group, legacy=_is_legacy_group(group, file_legacy)) + for group in waveforms.values() + ] diff --git a/dascore/io/index/backend.py b/dascore/io/index/backend.py index 84b68b756..242acee37 100644 --- a/dascore/io/index/backend.py +++ b/dascore/io/index/backend.py @@ -153,6 +153,8 @@ class SQLIndexBackend(AbstractIndexBackend): dialect: BaseDialect def __init__(self): + # collision name-sets already warned about (see _apply_attr_columns) + self._warned_attr_clobber: set[frozenset] = set() self._ensure_schema() # --- hooks each engine provides --------------------------------- @@ -684,10 +686,14 @@ def query(self, query=None, order_by=None, patch_ids=None) -> pd.DataFrame: patch_ids=patch_ids, ) df = self._fetch_df(sql, params) - df = self._flatten(df, attr_meta) + df, attr_columns = self._flatten(df, attr_meta) df = self._pivot_coords(df) + df = self._apply_attr_columns(df, attr_columns) if residuals: - df = apply_residuals(df, residuals) + # Residuals only ever come from attr predicates, so they must + # evaluate against the attr values even when a collision kept + # the attr column out of the flat frame. + df = apply_residuals(df, residuals, attr_columns) return df.reset_index(drop=True) def query_ids(self, query=None, order_by=None, patch_ids=None) -> list[int]: @@ -772,8 +778,17 @@ def export_records(self, patch_ids=None) -> list: sources, patches, attrs, links, defs, self._attr_meta() ) - def _flatten(self, df: pd.DataFrame, attr_meta: pd.DataFrame) -> pd.DataFrame: - """Post-process raw SQL output into the flat-relation contract.""" + def _flatten( + self, df: pd.DataFrame, attr_meta: pd.DataFrame + ) -> tuple[pd.DataFrame, dict[str, pd.Series]]: + """ + Post-process raw SQL output into the flat-relation contract. + + Returns the frame plus the dynamic attr columns keyed by original + attr name. The attr columns are applied by `_apply_attr_columns` + only after `_pivot_coords` has added the per-coord envelope columns, + so genuine name collisions can be detected against the full frame. + """ out = df.copy() # structural time columns: ns ints -> numpy time types (exactly) for col, flavor in _TIME_COLS.items(): @@ -788,10 +803,11 @@ def _flatten(self, df: pd.DataFrame, attr_meta: pd.DataFrame) -> pd.DataFrame: # Group the metadata once and drop every typed column in a single # pass: per-name refiltering plus one-drop-per-column was ~O(A^2) # metadata scans and frame copies for A dynamic attrs. - # Every name that could collide with a structural or envelope - # column (RESERVED_ATTR_COLUMNS and *_min/_max/_step) is refused at - # ingest, so a dynamic attr name never shadows an existing column - # here. + # Structural names (RESERVED_ATTR_COLUMNS) are refused at ingest, + # but an attr may legitimately share a name with a coordinate + # envelope column ({coord}_min/max/step) when another patch in the + # catalog has that coord; the coord column wins the flat name and + # the attr stays queryable through the _attrs namespace. cols_to_drop: list[str] = [] new_columns: dict[str, pd.Series] = {} for name, rows in attr_meta.groupby("attr_name", sort=False): @@ -821,8 +837,6 @@ def _flatten(self, df: pd.DataFrame, attr_meta: pd.DataFrame) -> pd.DataFrame: new_columns[name] = series if cols_to_drop: out = out.drop(columns=cols_to_drop) - for name, series in new_columns.items(): - out[name] = series # flat-contract names for source columns renames = { "source_path": "path", @@ -838,7 +852,37 @@ def _flatten(self, df: pd.DataFrame, attr_meta: pd.DataFrame) -> pd.DataFrame: + out.loc[has_base, "path"] ) out = out.drop(columns=["base_uri"]) - return out.drop(columns=["source_id"], errors="ignore") + return out.drop(columns=["source_id"], errors="ignore"), new_columns + + def _apply_attr_columns( + self, out: pd.DataFrame, new_columns: dict[str, pd.Series] + ) -> pd.DataFrame: + """ + Add dynamic attr columns to the flat frame, coord envelopes winning. + + Runs after `_pivot_coords` so every coordinate envelope column + exists; an attr whose name equals one is a genuine collision — it is + omitted from the flat view (still queryable via the _attrs + namespace) with a warning. + """ + clobbered = frozenset(name for name in new_columns if name in out.columns) + # The flat frame is also materialized internally (patch naming, + # chunking); warn once per backend per colliding name set so users + # learn about the shadowing without a warning on every access. + if clobbered and clobbered not in self._warned_attr_clobber: + self._warned_attr_clobber.add(clobbered) + names = ", ".join(sorted(clobbered)) + msg = ( + f"Attr(s) {names} collide with coordinate envelope columns " + "and are omitted from the flat contents; query them via the " + "_attrs namespace." + ) + warnings.warn(msg, UserWarning, stacklevel=2) + for name, series in new_columns.items(): + if name in out.columns: + continue + out[name] = series + return out @staticmethod def _add_envelope_objects(coords: pd.DataFrame) -> pd.DataFrame: diff --git a/dascore/io/index/ingest.py b/dascore/io/index/ingest.py index bbc245388..73b07fc90 100644 --- a/dascore/io/index/ingest.py +++ b/dascore/io/index/ingest.py @@ -27,23 +27,6 @@ # Attrs handled structurally or intentionally excluded from the index. _SKIPPED_ATTRS = frozenset({"history", "dims", "coords"}) -# Suffixes of the per-coordinate envelope columns the flat relation -# emits ({name}_min/{name}_max/{name}_step). An attr shaped like one of -# these is reserved catalog-wide — not just against the ingesting -# patch's own coords — so a flat-relation envelope column (e.g. -# "event_time_min") can never collide with a same-named attr contributed -# by a different patch, which would make one get_contents() column's -# meaning depend on which other patches share the catalog. (units/dtype -# do not become per-coord columns, so a real attr like "data_units" -# stays queryable, matching Patch.update_attrs.) -_ENVELOPE_SUFFIXES = ("min", "max", "step") - - -def _is_envelope_shaped(name: str) -> bool: - """True if name looks like a ``{coord}_{min,max,step}`` envelope column.""" - prefix, _, suffix = name.rpartition("_") - return bool(prefix) and suffix in _ENVELOPE_SUFFIXES - @dataclass(frozen=True) class TypedValue: @@ -222,13 +205,11 @@ def _extract_attrs(summary: PatchSummary) -> dict[str, TypedValue]: for name, value in raw.items(): if name in _SKIPPED_ATTRS or name.startswith("_"): continue - # Reserve structural columns and any coordinate-envelope-shaped - # name (catalog-wide, not just this patch's own coords) so the - # meaning of a flat-relation column never depends on which other - # patches share the catalog. - if sanitize_attr_name(name) in RESERVED_ATTR_COLUMNS or ( - _is_envelope_shaped(name) - ): + # Structural storage/contract column names cannot double as attr + # columns. Coordinate-envelope-shaped names (e.g. "channel_step") + # are ordinary attrs; a genuine collision with a coord envelope + # column is handled (warn, coord wins) when the flat view is built. + if sanitize_attr_name(name) in RESERVED_ATTR_COLUMNS: msg = ( f"Skipping reserved attr name {name!r}; it collides with a " "structural index column. The attr stays on the patch but " diff --git a/dascore/io/index/planned.py b/dascore/io/index/planned.py index 6fb6c27b9..b7579791e 100644 --- a/dascore/io/index/planned.py +++ b/dascore/io/index/planned.py @@ -215,7 +215,6 @@ def _output_records( """Convert plan output rows into ingestible source records.""" records = [] aux_info = aux_info or {} - envelope_suffixes = ("_min", "_max", "_step", "_units") for row in outputs.to_dict("records"): output_id = int(row["output_id"]) dims = str(row.get("dims") or "") @@ -233,12 +232,27 @@ def _output_records( record = _coord_record_from_row(info, name, dims=info["dims"]) if record is not None: coords.append(record) + # Envelope columns belong to coordinates actually present in the + # row; an attr that merely looks envelope-shaped (channel_step with + # no channel coord) is ordinary metadata and must be preserved. + coord_names = set(dim_names) | set(aux_info.get(output_id, {})) + coord_names |= { + key[1 : -len("_def_key")] + for key in row + if key.startswith("_") and key.endswith("_def_key") + } + coord_names |= {"time", "distance"} # fixed patches-table envelopes + envelope_keys = { + f"{name}_{sfx}" + for name in coord_names + for sfx in ("min", "max", "step", "units") + } attrs = {} for key, value in row.items(): if ( key in _NON_ATTR or key.startswith("_") - or any(key.endswith(sfx) for sfx in envelope_suffixes) + or key in envelope_keys or value is None or (np.isscalar(value) and pd.isnull(value)) ): diff --git a/dascore/io/index/query.py b/dascore/io/index/query.py index a7f79d8b3..a01770348 100644 --- a/dascore/io/index/query.py +++ b/dascore/io/index/query.py @@ -495,11 +495,21 @@ def build_sql( def apply_residuals( - df: pd.DataFrame, residuals: list[tuple[str, re.Pattern]] + df: pd.DataFrame, + residuals: list[tuple[str, re.Pattern]], + attr_columns: dict[str, pd.Series] | None = None, ) -> pd.DataFrame: - """Apply regex residual filters to the flat relation.""" + """ + Apply regex residual filters to the flat relation. + + Residuals are produced only for attr predicates, so when `attr_columns` + is given the original attr series is preferred over the flat column of + the same name — the flat column can instead hold a coordinate envelope + when the attr name collided with one. + """ + attr_columns = attr_columns or {} for name, pattern in residuals: - col = df[name] + col = attr_columns[name].loc[df.index] if name in attr_columns else df[name] keep = col.map( lambda x: bool(pattern.search(x)) if isinstance(x, str) else False ) diff --git a/dascore/io/index/schema.py b/dascore/io/index/schema.py index 249108568..d367bc5ec 100644 --- a/dascore/io/index/schema.py +++ b/dascore/io/index/schema.py @@ -206,6 +206,18 @@ "sample_count_total", "coord_def_id", "def_key", + # fixed time/distance envelope columns on the patches table: these + # exist for every row regardless of the patch's coords, so an attr + # with one of these names could not be told apart from structural + # metadata downstream (unlike other coords' envelope columns, which + # only exist when the coord does and are handled by the flat-view + # collision warning). + "time_min", + "time_max", + "time_step", + "distance_min", + "distance_max", + "distance_step", # flat-relation (spool-facing) names "path", "file_format", diff --git a/dascore/io/tdms/utils.py b/dascore/io/tdms/utils.py index ef2a80256..ded6dcd87 100644 --- a/dascore/io/tdms/utils.py +++ b/dascore/io/tdms/utils.py @@ -228,8 +228,6 @@ def _get_all_attrs(tdms_file, lead_in_length=28): ) t_coord = _get_time_coord(out, numofsamples) out["coords"] = {"time": t_coord, "distance": d_coord} - out.update(t_coord.get_attrs_dict("time")) - out.update(d_coord.get_attrs_dict("distance")) return out, fileinfo diff --git a/dascore/proc/basic.py b/dascore/proc/basic.py index 8dc8fdba9..2de68681d 100644 --- a/dascore/proc/basic.py +++ b/dascore/proc/basic.py @@ -16,7 +16,6 @@ from dascore.core.coords import get_coord from dascore.exceptions import ParameterError from dascore.utils.array import _apply_binary_ufunc -from dascore.utils.attrs import _raise_if_coord_attr_updates from dascore.utils.misc import _get_nullish from dascore.utils.models import ArrayLike from dascore.utils.patch import ( @@ -114,7 +113,6 @@ def update_attrs(self: PatchType, **attrs) -> PatchType: >>> # Add new custom attributes >>> with_custom = patch.update_attrs(processing_date="2024-01-01") """ - _raise_if_coord_attr_updates(attrs) new_attrs = self.attrs.model_dump(exclude_unset=True) new_attrs.update(attrs) validated = PatchAttrs.from_dict(new_attrs) diff --git a/dascore/utils/attrs.py b/dascore/utils/attrs.py index 96c12cf5e..50d9ab78b 100644 --- a/dascore/utils/attrs.py +++ b/dascore/utils/attrs.py @@ -4,7 +4,7 @@ from __future__ import annotations -from collections import ChainMap, defaultdict +from collections import ChainMap from collections.abc import Sequence from functools import reduce from typing import Literal @@ -13,11 +13,10 @@ import dascore as dc from dascore.constants import attr_conflict_description -from dascore.exceptions import AttributeMergeError, PatchAttributeError +from dascore.exceptions import AttributeMergeError from dascore.utils.docs import compose_docstring from dascore.utils.misc import ( _dict_list_diffs, - is_valid_coord_str, iterate, ) @@ -118,168 +117,3 @@ def _handle_other_attrs(mod_dict_list): ) cls = first_class if first_class is not dict else dc.PatchAttrs return cls(**mod_dict_list[0]) - - -def _raise_if_coord_attr_updates(update_map) -> None: - """Reject flat coord-summary keys in attr update calls.""" - if update_map is None: - return - coord_info, _ = separate_coord_info(update_map) - bad_keys = [] - for coord_name, info in coord_info.items(): - for field in info: - bad_keys.append(f"{coord_name}_{field}") - if bad_keys: - names = ", ".join(sorted(bad_keys)) - msg = ( - "PatchAttrs.update does not accept coordinate metadata. " - f"Received: {names}. Use update_coords(...) instead." - ) - raise PatchAttributeError(msg) - - -def separate_coord_info( - obj, - dims: tuple[str, ...] | None = None, - required: Sequence[str] | None = None, - cant_be_alone: tuple[str, ...] = ("units", "dtype"), -) -> tuple[dict, dict]: - """ - Separate coordinate information from mixed attr-like metadata. - - This helper is still needed because DASCore still accepts a few mixed - metadata shapes internally and in legacy IO paths. In particular, it is - used to: - - - normalize flat scan/index-style keys such as ``time_min`` and - ``distance_step`` into coordinate summary payloads - - split coordinate updates from pure attrs in coord-manager code - - unpack older nested ``{"coords": {...}}`` metadata payloads - - Supported input shapes include flat coord-style fields such as - ``{time_min, time_max, time_step, ...}`` and nested coord dictionaries - such as ``{coords: {time: {min, max, step}}}``. - - Parameters - ---------- - obj - The object or model to split. - dims - Optional dimension names used to recognize flat coord-style keys. - required - If provided, the required attributes (e.g., min, max, step). - cant_be_alone - Names which cannot be treated as coord info on their own. - - Returns - ------- - A tuple of ``(coord_dict, attrs_dict)`` where coordinate-like metadata has - been separated from the remaining pure attrs. - """ - coord_summary_fields = tuple(dc.core.CoordSummary.model_fields) - - def _split_coord_key(key, prefixes=None): - """Split flat coord summary key into coord name and field.""" - prefixes = tuple(iterate(prefixes)) if prefixes is not None else () - if prefixes: - for prefix in sorted(prefixes, key=len, reverse=True): - prefix_str = f"{prefix}_" - if key.startswith(prefix_str): - field = key[len(prefix_str) :] - if field in coord_summary_fields: - return prefix, field - return None - parts = key.rsplit("_", 1) - return tuple(parts) - - def _meets_required(coord_dict, strict=True): - """ - Return True coord dict meets the minimum required keys. - - coord_dict represents potential coordinate fields. - - Strict ensures all required values exist. - """ - if not coord_dict: - return False - if not required and (set(coord_dict) - cant_be_alone): - return True - if required or not strict: - return set(coord_dict).issuperset(required) - return False - - def _get_dims(obj): - """Try to ascertain dims from keys in obj.""" - # check first for coord manager - if isinstance(obj, dict) and hasattr(obj.get("coords", None), "dims"): - return obj["coords"].dims - - # This object already has dims, just honor it. - if dims := obj.get("dims", None): - return tuple(dims.split(",")) if isinstance(dims, str) else dims - - potential_keys = defaultdict(set) - for key in obj: - if not is_valid_coord_str(key): - continue - coord_name, field = _split_coord_key(key) - potential_keys[coord_name].add(field) - return tuple(i for i, v in potential_keys.items() if _meets_required(v)) - - def _get_coords_from_top_level(obj, out, dims): - """First get coord info from top level.""" - for dim in iterate(dims): - potential_coord = {} - for key, value in obj.items(): - split = _split_coord_key(key, prefixes=(dim,)) - if split is None: - continue - _, field = split - potential_coord[field] = value - if _meets_required(potential_coord, strict=False): - out[dim] = potential_coord - - def _get_coords_from_coord_level(obj, out): - """Get coords from coordinate level.""" - coords = obj.get("coords", {}) - if hasattr(coords, "to_summary_dict"): - coords = coords.to_summary_dict() - for key, value in coords.items(): - if hasattr(value, "to_summary"): - value = value.to_summary() - if hasattr(value, "model_dump"): - value = value.model_dump() - if _meets_required(value, strict=False): - out[key] = value - - def _pop_keys(obj, out): - """Pop out old keys for attrs, and unused keys from out.""" - # first coord subdict - obj.pop("coords", None) - # then top-level - for coord_name, sub_dict in out.items(): - for thing_name in sub_dict: - obj.pop(f"{coord_name}_{thing_name}", None) - if "step" in sub_dict: - obj.pop(f"d_{coord_name}", None) - - # sequence of short-circuit checks - coord_dict = {} - required = set(required) if required is not None else set() - cant_be_alone = set(cant_be_alone) - if obj is None: - return coord_dict, {} - if hasattr(obj, "model_dump"): - obj = obj.model_dump() - obj = dict(obj) - # Check if dims need to be updated. - new_dims = _get_dims(obj) - if new_dims and new_dims != dims: - dims = new_dims - # this is already a dict of coord info. - if dims and set(dims).issubset(set(obj)): - return obj, {} - _get_coords_from_coord_level(obj, coord_dict) - _get_coords_from_top_level(obj, coord_dict, dims) - _pop_keys(obj, coord_dict) - return coord_dict, obj diff --git a/dascore/utils/misc.py b/dascore/utils/misc.py index 78f0d1444..2309fcd8d 100644 --- a/dascore/utils/misc.py +++ b/dascore/utils/misc.py @@ -22,7 +22,6 @@ from scipy.linalg import solve from scipy.special import factorial -import dascore as dc from dascore.compat import UPath, is_array from dascore.constants import WARN_LEVELS from dascore.exceptions import ( @@ -629,12 +628,6 @@ def _matches_prefix_suffix(input_str, suffixes, prefixes=None): return bool(re.match(regex, input_str)) -def is_valid_coord_str(input_str, prefixes=None): - """Return True if an input string is valid for representing coord info.""" - _valid_keys = tuple(dc.core.CoordSummary.model_fields) - return _matches_prefix_suffix(input_str, _valid_keys, prefixes) - - def cached_method(func): """ Cache decorated method. diff --git a/docs/notes/spool_index.qmd b/docs/notes/spool_index.qmd index 08a91c870..ecf56bcb8 100644 --- a/docs/notes/spool_index.qmd +++ b/docs/notes/spool_index.qmd @@ -38,7 +38,7 @@ SQLite permits concurrent readers and serializes writers. Initialization and upd ## The flat relation -Spool-facing operations consume the tables through one flat relation: a dataframe with one row per patch carrying `{dim}_min/max/step` envelopes, private structural columns (`_patch_id`, `_{name}_def_key` coordinate identities, and `_{name}_units` canonical units — the chunk planner's unit-partition key), the `dims` signature, and one column per attribute. The chunk planner and selection both operate on this relation (see the [Spool Chunking](spool_chunking.qmd) and [Spool Selection](spool_selection.qmd) notes). The cell below runs against the real catalog so this description cannot silently drift. +Spool-facing operations consume the tables through one flat relation: a dataframe with one row per patch carrying `{dim}_min/max/step` envelopes, private structural columns (`_patch_id`, `_{name}_def_key` coordinate identities, and `_{name}_units` canonical units — the chunk planner's unit-partition key), the `dims` signature, and one column per attribute. Attrs and coords are independent namespaces, so an attr may share a name with a coordinate envelope column (e.g. an attr `channel_step` alongside a `channel` coordinate); in that rare genuine collision the envelope column owns the flat name, the attr column is omitted with a warning, and the attr remains queryable through the `_attrs` namespace. The chunk planner and selection both operate on this relation (see the [Spool Chunking](spool_chunking.qmd) and [Spool Selection](spool_selection.qmd) notes). The cell below runs against the real catalog so this description cannot silently drift. ```{python} import dascore as dc diff --git a/tests/test_core/test_attrs.py b/tests/test_core/test_attrs.py index 3ae15b8d3..f96dd5efb 100644 --- a/tests/test_core/test_attrs.py +++ b/tests/test_core/test_attrs.py @@ -11,7 +11,6 @@ from dascore.constants import VALID_DATA_TYPES, max_lens from dascore.core.attrs import PatchAttrs from dascore.core.coords import get_coord -from dascore.exceptions import PatchAttributeError @pytest.fixture(scope="class") @@ -223,14 +222,15 @@ def test_attrs_can_update_non_coord_fields(self, random_attrs): attrs = PatchAttrs.from_dict(random_attrs).update(tag="miles") assert attrs.tag == "miles" - def test_update_rejects_coord_like_fields(self, random_attrs): - """Flat coordinate-like fields should go through update_coords instead.""" - with pytest.raises(PatchAttributeError, match="update_coords"): - PatchAttrs.from_dict(random_attrs).update(time_min=1) + def test_update_accepts_coord_like_fields(self, random_attrs): + """Coord-shaped names are ordinary attrs; update never touches coords.""" + out = PatchAttrs.from_dict(random_attrs).update(time_min=1, channel_step=3) + assert out["time_min"] == 1 + assert out["channel_step"] == 3 def test_update_rejects_nested_coords(self, random_patch): """Passing coords directly should fail.""" - with pytest.raises(PatchAttributeError, match="coordinate metadata"): + with pytest.raises(ValueError, match="coordinate metadata"): PatchAttrs.from_dict(random_patch.attrs).update(coords=random_patch.coords) def test_update_ignores_dims(self, random_attrs): diff --git a/tests/test_core/test_coord_segmented.py b/tests/test_core/test_coord_segmented.py index 12c57064d..7b68c389e 100644 --- a/tests/test_core/test_coord_segmented.py +++ b/tests/test_core/test_coord_segmented.py @@ -635,13 +635,6 @@ def test_to_summary(self, float_gap_coord): assert summary.len == 20 assert summary.fingerprint == float_gap_coord.fingerprint() - def test_get_attrs_dict_no_step(self, float_gap_coord): - """The attrs dict omits the (null) step.""" - out = float_gap_coord.get_attrs_dict("time") - assert "time_step" not in out - assert out["time_min"] == 0.0 - assert out["time_max"] == 24.0 - def test_ns_precision_exact(self): """Nanosecond datetimes survive bit-exactly (no float pass).""" ns = np.timedelta64(1, "ns") diff --git a/tests/test_core/test_coordmanager.py b/tests/test_core/test_coordmanager.py index e68161822..a88906de1 100644 --- a/tests/test_core/test_coordmanager.py +++ b/tests/test_core/test_coordmanager.py @@ -42,19 +42,11 @@ class TestGetCoordManager: """Test suite for `get_coord_manager` helper function.""" - def test_coords_and_attrs_raise(self, random_patch): - """Ensure using coords and attrs raises.""" - msg = "Cannot use both attrs and coords" - coords, attrs = random_patch.coords, random_patch.attrs - with pytest.raises(ParameterError, match=msg): - get_coord_manager(coords=coords, attrs=attrs) - - def test_coords_from_attrs(self, random_patch): - """Patch attrs no longer reconstruct coordinates.""" - attrs = random_patch.attrs - out = get_coord_manager(attrs=attrs) - assert out.dims == () - assert not out.coord_map + def test_attrs_not_accepted(self, random_patch): + """Coordinates are never built from attrs; the param is gone.""" + attrs = {"time_min": 0, "time_max": 10, "time_step": 1} + with pytest.raises(TypeError, match="attrs"): + get_coord_manager(None, dims=("time",), attrs=attrs) def test_non_coord_dims(self): """Ensure non coordinate dimensions can be created using shape.""" @@ -322,16 +314,6 @@ def test_init_coord_manager_with_non_coord_dim(self, cm_non_coord_dim): assert cm.shape == (10, 5) -class TestCoordManagerWithAttrs: - """Tests for initing coord managing with attribute dict.""" - - def test_missing_dim(self): - """Coord manager should be able to pull missing info from attributes.""" - attrs = dict(distance_min=1, distance_max=100, distance_step=10) - new = get_coord_manager(None, ("distance",), attrs=attrs) - assert "distance" in new.coord_map - - class TestDrop: """Tests for dropping coords with coord manager.""" @@ -924,26 +906,24 @@ def test_rename_same_dims_extra_coords(self, cm_multidim): assert set(out.dim_map) == set(cm.dim_map) -class TestUpdateFromAttrs: - """Tests to ensure updating attrs can update coordinates.""" +class TestUpdateFlatCoordKwargs: + """Coord updates via {dim}_{field} kwargs against the manager's own dims.""" - def test_update_min(self, cm_basic): - """Ensure min time in attrs updates appropriate coord.""" + def test_update_max(self, cm_basic): + """Ensure a {dim}_max kwarg updates the appropriate coord.""" for dim in cm_basic.dims: coord = cm_basic.coord_map[dim] - attrs = {f"{dim}_max": coord.min()} - new, _ = cm_basic.update_from_attrs(attrs) + new = cm_basic.update(**{f"{dim}_max": coord.min()}) new_coord = new.coord_map[dim] assert len(new_coord) == len(coord) assert new_coord.max() == coord.min() - def test_update_max(self, cm_basic): - """Ensure max time in attrs updates appropriate coord.""" + def test_update_min(self, cm_basic): + """Ensure a {dim}_min kwarg updates the appropriate coord.""" for dim in cm_basic.dims: coord = cm_basic.coord_map[dim] - attrs = {f"{dim}_min": coord.max()} dist = coord.max() - coord.min() - new, _ = cm_basic.update_from_attrs(attrs) + new = cm_basic.update(**{f"{dim}_min": coord.max()}) new_coord = new.coord_map[dim] new_dist = new_coord.max() - new_coord.min() assert dist == new_dist @@ -954,36 +934,14 @@ def test_update_step(self, cm_basic): """Ensure the step can be updated which changes endtime.""" for dim in cm_basic.dims: coord = cm_basic.coord_map[dim] - attrs = {f"{dim}_step": coord.step * 10} dist = coord.max() - coord.min() - new, _ = cm_basic.update_from_attrs(attrs) + new = cm_basic.update(**{f"{dim}_step": coord.step * 10}) new_coord = new.coord_map[dim] new_dist = new_coord.max() - new_coord.min() assert (dist * 10) == new_dist assert len(new_coord) == len(coord) assert new_coord.min() == coord.min() - def test_attrs_as_dict(self, cm_basic): - """Returned attrs should stay pure metadata.""" - coord = cm_basic.coord_map["time"] - attrs = {"time_max": coord.min()} - cm, attrs = cm_basic.update_from_attrs(attrs) - assert "dims" not in attrs.model_dump() - assert cm.dims == cm_basic.dims - - def test_attrs_as_patch_attr(self, cm_basic): - """Ensure pure PatchAttrs inputs are still accepted.""" - attrs = dc.PatchAttrs(tag="example") - cm, new_attrs = cm_basic.update_from_attrs(attrs) - assert new_attrs == attrs - assert cm.dims == cm_basic.dims - - def test_consistent_attrs_leaves_coords_unchanged(self, random_patch): - """Attrs which are already consistent should leave coord unchanged.""" - attrs, coords = random_patch.attrs, random_patch.coords - new_coords, new_attrs = coords.update_from_attrs(attrs) - assert new_coords == coords - class TestUpdate: """Tests for updating coordinates.""" diff --git a/tests/test_core/test_patch.py b/tests/test_core/test_patch.py index 4e4e5a012..9d19a5f8e 100644 --- a/tests/test_core/test_patch.py +++ b/tests/test_core/test_patch.py @@ -1022,10 +1022,12 @@ def test_update_startttime2(self, random_patch): assert time_summary.min == new_start assert time_summary.max == new_start + duration - def test_update_attrs_rejects_coordinate_fields(self, random_patch): - """Flat coordinate-style attrs should go through update_coords.""" - with pytest.raises(ValueError, match="update_coords"): - random_patch.update_attrs(time_step=10) + def test_update_attrs_accepts_coordinate_shaped_names(self, random_patch): + """Coord-shaped names are ordinary attrs; coords are never affected.""" + out = random_patch.update_attrs(time_step=10, channel_step=3) + assert out.attrs["time_step"] == 10 + assert out.attrs["channel_step"] == 3 + assert out.coords == random_patch.coords def test_update_non_sorted_coord(self, wacky_dim_patch): """Ensure update_coords updates non-sorted coordinates.""" diff --git a/tests/test_integrations/test_attr_coord_independence.py b/tests/test_integrations/test_attr_coord_independence.py new file mode 100644 index 000000000..8193cb725 --- /dev/null +++ b/tests/test_integrations/test_attr_coord_independence.py @@ -0,0 +1,273 @@ +""" +Regression tests for full attr/coord independence. + +Attrs whose names look like flat coordinate metadata (``channel_step``, +``pulse_len``, ...) are ordinary attrs everywhere: model construction, +``update_attrs``, DASDAE round-trips, and the spool index. Coordinates are +never inferred from attr names. The one place the two worlds meet is the +flat ``get_contents()`` frame, where a genuine collision with a coordinate +envelope column warns and the coordinate wins. +""" + +from __future__ import annotations + +import warnings + +import numpy as np +import pytest + +import dascore as dc +from dascore.core.coordmanager import get_coord_manager + +# The suffix-capture family from the original bug report: each of these was +# previously reinterpreted as coordinate metadata for a phantom coordinate. +COORD_SHAPED_NAMES = ( + "channel_step", + "pulse_len", + "sensor_fingerprint", + "gain_max", + "noise_min", + "probe_dims", + "x_units", + "y_dtype", + "gauge_length", +) + + +@pytest.fixture() +def patch_with_coord_shaped_attrs(random_patch): + """A patch whose attrs include every coord-shaped name.""" + return random_patch.update_attrs(**{x: 1 for x in COORD_SHAPED_NAMES}) + + +class TestCoordShapedNamesAreAttrs: + """No attr name manufactures a phantom coordinate on any entry point.""" + + @pytest.mark.parametrize("name", COORD_SHAPED_NAMES) + def test_patch_attrs_construction(self, name): + """Cause 1: PatchAttrs(...) keeps the attr, invents no coords.""" + with warnings.catch_warnings(): + warnings.simplefilter("error") + attrs = dc.PatchAttrs(**{name: 1}) + assert attrs[name] == 1 + + @pytest.mark.parametrize("name", COORD_SHAPED_NAMES) + def test_attrs_update(self, name, random_patch): + """Cause 3: PatchAttrs.update accepts the name like any other.""" + with warnings.catch_warnings(): + warnings.simplefilter("error") + attrs = random_patch.attrs.update(**{name: 1}) + assert attrs[name] == 1 + + def test_patch_update_attrs(self, patch_with_coord_shaped_attrs, random_patch): + """Cause 3: update_attrs sets attrs and never touches coords.""" + patch = patch_with_coord_shaped_attrs + for name in COORD_SHAPED_NAMES: + assert patch.attrs[name] == 1 + assert patch.coords == random_patch.coords + + def test_subclass_field(self, random_patch): + """Cause 3: an explicit subclass field (the Sintela pattern) works.""" + + class _VendorAttrs(dc.PatchAttrs): + channel_step: int | None = None + + attrs = _VendorAttrs(channel_step=3) + patch = random_patch.update_attrs(**attrs.model_dump(exclude_unset=True)) + assert patch.attrs["channel_step"] == 3 + + def test_real_coords_unaffected(self, random_patch): + """Cause 2: supplying real coords plus a coord-shaped attr is safe.""" + patch = random_patch.update_attrs(channel_step=3) + assert patch.coords.coord_map.keys() == random_patch.coords.coord_map.keys() + assert patch.attrs["channel_step"] == 3 + + +class TestCoordsNeverBuiltFromAttrs: + """Cause 4: the get_coord_manager attrs branch is gone.""" + + def test_attrs_param_removed(self): + """The old crash repro now fails loudly at the signature.""" + attrs = {"time_min": 0, "time_max": 10, "time_step": 1, "channel_step": 3} + with pytest.raises(TypeError, match="attrs"): + get_coord_manager(None, dims=("time",), attrs=attrs) + + +class TestDasdaeRoundTrip: + """Cause 7a: coord-shaped attrs survive DASDAE write/read exactly.""" + + @pytest.fixture() + def round_tripped(self, patch_with_coord_shaped_attrs, tmp_path_factory): + """Write and read back a patch with coord-shaped attrs.""" + path = tmp_path_factory.mktemp("dasdae_attrs") / "patch.h5" + patch_with_coord_shaped_attrs.io.write(path, "dasdae") + return dc.spool(path)[0], dc.scan(path)[0] + + def test_read_keeps_attrs(self, round_tripped, patch_with_coord_shaped_attrs): + """Attrs and coords round-trip unchanged.""" + patch, _ = round_tripped + for name in COORD_SHAPED_NAMES: + assert patch.attrs[name] == 1 + assert patch.coords == patch_with_coord_shaped_attrs.coords + + def test_scan_keeps_attrs(self, round_tripped): + """Scan summaries carry the attrs too.""" + _, summary = round_tripped + for name in COORD_SHAPED_NAMES: + assert summary.attrs[name] == 1 + + def test_shadowing_attr_round_trips(self, random_patch, tmp_path_factory): + """An attr shadowing the patch's own coord envelope still survives.""" + patch = random_patch.rename_coords(distance="channel") + patch = patch.update_attrs(channel_step=999) + path = tmp_path_factory.mktemp("dasdae_shadow") / "patch.h5" + patch.io.write(path, "dasdae") + read_back = dc.spool(path)[0] + assert read_back.attrs["channel_step"] == 999 + assert read_back.coords == patch.coords + + +class TestIndexKeepsCoordShapedAttrs: + """Cause 5: ingest indexes every attr; no warn-and-drop.""" + + @pytest.fixture() + def spool_with_attrs(self, patch_with_coord_shaped_attrs): + """An in-memory spool holding the attr-decorated patch.""" + return dc.spool([patch_with_coord_shaped_attrs]) + + def test_no_warning_on_ingest(self, patch_with_coord_shaped_attrs): + """Building and materializing the spool emits no warnings.""" + with warnings.catch_warnings(): + warnings.simplefilter("error") + spool = dc.spool([patch_with_coord_shaped_attrs]) + df = spool.get_contents() + assert df["channel_step"].iloc[0] == 1 + + def test_attrs_queryable(self, spool_with_attrs): + """The attrs work as filters through the _attrs namespace.""" + assert len(spool_with_attrs.select(_attrs={"channel_step": 1})) == 1 + assert len(spool_with_attrs.select(_attrs={"channel_step": 2})) == 0 + + +class TestGetContentsClobberWarning: + """Cause 6: the one surviving check.""" + + @pytest.fixture() + def shadowing_patch(self, random_patch): + """A patch with an attr equal to one of its own envelope columns.""" + patch = random_patch.rename_coords(distance="channel") + return patch.update_attrs(channel_step=999) + + def test_warns_and_coord_wins(self, shadowing_patch): + """Genuine collision warns; the coord envelope owns the column.""" + name = "channel_step" + spool = dc.spool([shadowing_patch]) + with pytest.warns(UserWarning, match="collide with coordinate envelope"): + df = spool.get_contents() + # the flat column holds the coord step, not the attr value + step = shadowing_patch.get_coord("channel").step + assert df[name].iloc[0] == step + # the attr remains queryable through the explicit namespace + assert len(spool.select(_attrs={name: 999})) == 1 + + def test_cross_patch_collision(self, random_patch): + """An attr can collide with another patch's coord envelope.""" + renamed = random_patch.rename_coords(distance="channel") + with_attr = random_patch.update_attrs(channel_step=3) + # the warning fires on the first flat materialization, which a + # multi-patch spool performs during construction + with pytest.warns(UserWarning, match="collide with coordinate envelope"): + spool = dc.spool([renamed, with_attr]) + df = spool.get_contents() + assert "channel_step" in df.columns # the envelope column survives + + def test_no_collision_no_warning(self, patch_with_coord_shaped_attrs): + """Without a matching coord there is nothing to warn about.""" + spool = dc.spool([patch_with_coord_shaped_attrs]) + with warnings.catch_warnings(): + warnings.simplefilter("error") + df = spool.get_contents() + assert df["channel_step"].iloc[0] == 1 + + def test_regex_query_reads_attr_under_collision(self, random_patch): + """A regex _attrs query evaluates the attr, not the coord column.""" + import re + + # one patch carrying both the coord and a same-named string attr: + # the flat column is the (numeric) envelope even in the filtered + # frame, so the residual must fall back to the real attr values + patch = random_patch.rename_coords(distance="channel") + patch = patch.update_attrs(channel_min="vendor-a7") + spool = dc.spool([patch]) + with pytest.warns(UserWarning, match="collide with coordinate envelope"): + df = spool.get_contents() + assert "vendor-a7" not in set(df["channel_min"].astype(str)) + hits = spool.select(_attrs={"channel_min": re.compile("vendor-.*")}) + assert len(hits) == 1 + misses = spool.select(_attrs={"channel_min": re.compile("nope")}) + assert len(misses) == 0 + + +class TestFixedEnvelopeColumnsReserved: + """time/distance envelopes are patches-table columns, reserved at ingest.""" + + def test_fixed_envelope_attr_warns_and_skips(self, random_patch): + """An attr named like a fixed envelope column warns and is skipped.""" + patch = random_patch.update_attrs(time_step=123.0) + with pytest.warns(UserWarning, match="reserved attr name"): + spool = dc.spool([patch]) + df = spool.get_contents() + # the column keeps structural values; the attr stays on the patch + assert df["time_step"].iloc[0] != 123.0 + assert spool[0].attrs["time_step"] == 123.0 + + +class TestDerivedCatalogKeepsAttrs: + """Chunk/concat derived records must preserve coord-shaped attrs.""" + + def test_chunk_preserves_coord_shaped_attr(self, random_patch): + """A channel_step attr (no channel coord) survives spool.chunk.""" + patch = random_patch.update_attrs(channel_step=3) + chunked = dc.spool([patch]).chunk(time=1) + df = chunked.get_contents() + assert "channel_step" in df.columns + assert (df["channel_step"] == 3).all() + assert len(chunked.select(_attrs={"channel_step": 3})) == len(chunked) + + +class TestCompatStaysInDasdae: + """The legacy-metadata compat layer must not bleed out of dasdae.""" + + def test_import_graph(self): + """Nothing outside dascore.io.dasdae imports the _compat module.""" + import re + from pathlib import Path + + root = Path(dc.__file__).parent + dasdae_dir = root / "io" / "dasdae" + pattern = re.compile(r"\b_compat\b") + outside = [ + str(path.relative_to(root)) + for path in root.rglob("*.py") + if dasdae_dir not in path.parents + and pattern.search(path.read_text(encoding="utf-8")) + ] + assert not outside, f"_compat referenced outside dasdae: {outside}" + + +class TestLegacyDasdaeFile: + """Cause 7b: legacy fixture files still parse correctly.""" + + def test_legacy_fixture_reads(self): + """The shipped legacy file loads with coords intact, attrs pure.""" + pytest.importorskip("pooch") + from dascore.utils.downloader import fetch + + path = fetch("example_dasdae_event_1.h5") + patch = dc.spool(path)[0] + # coord metadata must have landed on coords, not attrs + attr_names = set(patch.attrs.model_dump()) + for coord in patch.coords.coord_map: + for field_ in ("min", "max", "step"): + assert f"{coord}_{field_}" not in attr_names + assert np.issubdtype(patch.coords.get_array("time").dtype, np.datetime64) diff --git a/tests/test_io/test_common_io.py b/tests/test_io/test_common_io.py index aca4f1d54..f79272138 100644 --- a/tests/test_io/test_common_io.py +++ b/tests/test_io/test_common_io.py @@ -459,6 +459,24 @@ def test_no_bytes(self, scanned_summaries): for key, value in model.items(): assert not isinstance(value, bytes | np.bytes_) + def test_no_coord_mirroring_attrs(self, scanned_summaries): + """ + Shipped readers must not mirror coord metadata into attrs. + + Attrs and coords are fully independent; an attr named + ``{coord}_{field}`` for one of the patch's own coords would shadow a + coordinate envelope column in the flat spool contents. Vendor attrs + that merely look coord-shaped (e.g. ``channel_step`` without a + ``channel`` coord) are fine. + """ + fields = tuple(dc.core.CoordSummary.model_fields) + for raw in scanned_summaries: + summary = _scan_summary(raw) + names = set(summary.coords) | set(summary.dims) + mirrored = {f"{c}_{f}" for c in names for f in fields} + bad = mirrored & set(summary.attrs.model_dump()) + assert not bad, f"attrs mirror coord metadata: {sorted(bad)}" + class TestWrite: """Tests for writing data to disk.""" diff --git a/tests/test_io/test_dasdae/test_dasdae.py b/tests/test_io/test_dasdae/test_dasdae.py index 847501536..ae5f5a379 100644 --- a/tests/test_io/test_dasdae/test_dasdae.py +++ b/tests/test_io/test_dasdae/test_dasdae.py @@ -18,6 +18,7 @@ from dascore.core.coords import CoordString from dascore.exceptions import InvalidFiberFileError from dascore.io import dasdae as dasdae_mod +from dascore.io.dasdae._compat import translate_legacy_attrs from dascore.io.dasdae.core import DASDAEV1 from dascore.io.dasdae.utils import ( _decode_attr_value, @@ -30,7 +31,6 @@ _get_scan_payload_from_group, _save_array, _save_patch, - _translate_legacy_attrs, ) from dascore.utils.downloader import fetch from dascore.utils.misc import register_func @@ -164,6 +164,32 @@ def test_reads_legacy_fixture(self): assert len(spool) == 1 assert spool[0].dims + def test_append_to_legacy_file_keeps_new_attrs( + self, random_patch, tmp_path_factory + ): + """New groups appended to a legacy file must not be legacy-stripped.""" + from dascore.io.dasdae.utils import _SEPARATE_ATTRS_KEY + + path = tmp_path_factory.mktemp("dasdae_legacy_append") / "mixed.h5" + old_patch = random_patch.update_attrs(tag="old") + old_patch.io.write(path, "dasdae") + # simulate a legacy file: strip the root and group markers + with h5py.File(path, "a") as h5: + del h5.attrs[_SEPARATE_ATTRS_KEY] + for group in h5["waveforms"].values(): + del group.attrs[_SEPARATE_ATTRS_KEY] + # append a new patch carrying an attr shadowing its own coord envelope + dim = random_patch.dims[0] + new_patch = random_patch.update_attrs(tag="new", **{f"{dim}_step": 999}) + new_patch.io.write(path, "dasdae") + with h5py.File(path, "r") as h5: + assert not h5.attrs.get(_SEPARATE_ATTRS_KEY, False) # file stays legacy + patches = {p.attrs["tag"]: p for p in dc.read(path, file_format="DASDAE")} + assert set(patches) == {"old", "new"} + # the appended group round-trips exactly; the legacy one still reads + assert patches["new"].attrs[f"{dim}_step"] == 999 + assert patches["old"].coords == old_patch.coords + def test_datetimes(self, tmp_path_factory, random_patch): """Ensure the datetimes in the attrs come back as datetimes.""" # create a patch with a custom dt attribute. @@ -322,7 +348,7 @@ class CoordManagerLike: def to_summary_dict(self): return {"time": {"units": "s", "step": 1}} - out = _translate_legacy_attrs({"coords": CoordManagerLike()}) + out = translate_legacy_attrs({"coords": CoordManagerLike()}) assert out["time_units"] == "s" assert out["time_step"] == 1 @@ -333,7 +359,7 @@ class SummaryLike: def to_summary(self): return dc.core.CoordSummary(min=0, max=1, step=2, units="m") - out = _translate_legacy_attrs( + out = translate_legacy_attrs( { "coords": {"distance": SummaryLike(), "time": object()}, "dims": "distance,time", @@ -345,18 +371,34 @@ def to_summary(self): assert out["time_step"] == 3 def test_translate_legacy_attrs_ignores_non_mapping_coords(self): - """Legacy stringified coord payloads should be ignored, not crash.""" - out = _translate_legacy_attrs({"coords": "pickled-coords-placeholder"}) + """Undecodable string coord payloads are ignored once opted in.""" + with set_config(allow_dasdae_format_unpickle=True): + out = translate_legacy_attrs({"coords": "pickled-coords-placeholder"}) assert "coords" not in out def test_translate_legacy_attrs_decodes_pickled_coord_payload(self): """Legacy pickled coord payloads should still restore coord metadata.""" payload = pickle.dumps({"distance": {"min": 0, "max": 1, "units": "m"}}) - out = _translate_legacy_attrs({"coords": payload.decode("latin1")}) + with set_config(allow_dasdae_format_unpickle=True): + out = translate_legacy_attrs({"coords": payload.decode("latin1")}) assert out["distance_units"] == "m" assert out["distance_min"] == 0 assert out["distance_max"] == 1 + def test_translate_legacy_attrs_never_unpickles_without_opt_in(self): + """The opt-in gate must fire before any pickle.loads call runs.""" + executed = [] + + class _Payload: + def __reduce__(self): + return (executed.append, ("pickle ran",)) + + payload = pickle.dumps(_Payload()).decode("latin1") + with set_config(allow_dasdae_format_unpickle=False): + with pytest.raises(InvalidFiberFileError, match="unpickle"): + translate_legacy_attrs({"coords": payload}) + assert not executed, "pickle.loads ran before the security gate" + def test_scan_preserves_legacy_coord_units_from_attr_payload(self): """Legacy attr coord units should backfill missing coord-node units.""" with set_config(allow_dasdae_format_unpickle=True): diff --git a/tests/test_io/test_index/test_index_edge_cases.py b/tests/test_io/test_index/test_index_edge_cases.py index 9aed8946b..e5c88a64c 100644 --- a/tests/test_io/test_index/test_index_edge_cases.py +++ b/tests/test_io/test_index/test_index_edge_cases.py @@ -1253,25 +1253,21 @@ def test_non_reserved_attr_round_trips(self): assert spool.get_contents()["experiment"].iloc[0] == "exp42" assert len(spool.select(experiment="exp42")) == 1 - def test_cross_patch_envelope_attr_reserved(self): - """An envelope-shaped attr is reserved even against another patch's coord.""" + def test_cross_patch_envelope_attr_clobbered_not_reserved(self): + """An envelope-shaped attr indexes normally; the coord wins the column.""" import dascore as dc base = dc.get_example_patch() - # construction-time attrs bypass the update_attrs coord-name guard. - p1 = dc.Patch( - data=base.data, - coords=base.coords, - dims=base.dims, - attrs=dict(base.attrs) | {"event_time_min": 123.0}, - ) + p1 = base.update_attrs(event_time_min=123.0) p2 = dc.get_example_patch().rename_coords(time="event_time") - with pytest.warns(UserWarning, match="reserved attr name 'event_time_min'"): + with pytest.warns(UserWarning, match="collide with coordinate envelope"): spool = dc.spool([p1, p2]) df = spool.get_contents() backend = spool._catalog.backend - assert "event_time_min" not in backend.attr_names() - # the column is the coordinate envelope, not the stray attr value. + # the attr is indexed and stays queryable via the _attrs namespace. + assert "event_time_min" in backend.attr_names() + assert len(spool.select(_attrs={"event_time_min": 123.0})) == 1 + # the flat column is the coordinate envelope, not the attr value. assert "event_time_min" in df.columns assert 123.0 not in set(df["event_time_min"].dropna()) diff --git a/tests/test_utils/test_attrs_utils.py b/tests/test_utils/test_attrs_utils.py index 542d230f8..2ba5e3cb5 100644 --- a/tests/test_utils/test_attrs_utils.py +++ b/tests/test_utils/test_attrs_utils.py @@ -6,55 +6,8 @@ import pytest -import dascore as dc from dascore import PatchAttrs -from dascore.utils.attrs import ( - _raise_if_coord_attr_updates, - combine_patch_attrs, - separate_coord_info, -) - - -def _validate_no_coords( - obj, - dims: tuple[str, ...] | None = None, - coord_manager=None, - raise_error: bool = True, - flat_keys: bool = True, -) -> dict: - """Test helper for the old attrs-purity behavior.""" - - def _normalize(obj): - if obj is None: - return {} - if hasattr(obj, "model_dump"): - return obj.model_dump() - return dict(obj) - - out = _normalize(obj) - direct_names = set() - if "coords" in out: - direct_names.add("coords") - out.pop("coords", None) - if "dims" in out: - direct_names.add("dims") - out.pop("dims", None) - known_names = set(() if dims is None else dims) | {"time", "distance"} - if coord_manager is not None: - dims = coord_manager.dims - known_names |= set(coord_manager.coord_map) - if flat_keys: - scan_dims = tuple(known_names) if known_names else dims - coord_info, attr_info = separate_coord_info(out, dims=scan_dims) - coord_names = direct_names | set(coord_info) - else: - attr_info = out - coord_names = direct_names - if coord_names and raise_error: - names = ", ".join(sorted(coord_names)) - msg = "PatchAttrs no longer accepts coordinate metadata. " f"Received: {names}." - raise ValueError(msg) - return attr_info +from dascore.utils.attrs import combine_patch_attrs class TestMergeAttrs: @@ -137,128 +90,3 @@ def test_private_attrs_are_ignored_for_merge_conflicts(self, random_patch): attrs2 = random_patch.attrs.update(_source_patch_id="two") out = combine_patch_attrs([attrs1, attrs2]) assert "_source_patch_id" not in out.model_dump() - - -class TestSeparateCoordInfo: - """Tests for separating coord info from attr dict.""" - - def test_empty(self): - """Empty args should return emtpy dicts.""" - out1, out2 = separate_coord_info(None) - assert out1 == out2 == {} - - def test_meets_reqs(self): - """Simple case for filtering out required attrs.""" - input_dict = {"coords": {"time": {"min": 10}}} - coords, attrs = separate_coord_info(input_dict) - assert coords == input_dict["coords"] - assert attrs == {} - - def test_dict_of_coord_info(self, random_patch): - """Passing in a dictionary of coord info should work.""" - coord_dict = random_patch.coords.to_summary_dict() - dims = random_patch.dims - coords, attrs = separate_coord_info(coord_dict, dims=dims) - assert coords == coord_dict - assert attrs == {} - - def test_ignores_keys_without_underscore(self): - """Keys without separators should stay in attrs.""" - coords, attrs = separate_coord_info({"time": 1, "tag": "x"}) - assert coords == {} - assert attrs == {"time": 1, "tag": "x"} - - def test_ignores_unknown_coord_suffix(self): - """Unknown coord suffixes should not be parsed as coord metadata.""" - coords, attrs = separate_coord_info({"time_bob": 1, "tag": "x"}) - assert coords == {} - assert attrs == {"time_bob": 1, "tag": "x"} - - def test_unsplittable_valid_coord_key_stays_attr(self): - """Suffix-only coord-looking keys should not crash dim inference.""" - coords, attrs = separate_coord_info({"units": "m", "tag": "x"}) - assert coords == {} - assert attrs == {"units": "m", "tag": "x"} - - def test_invalid_coord_like_key_ignored_in_dim_inference(self): - """Only valid coord summary keys should participate in inferred dims.""" - obj = {"time_bob": 1, "distance_min": 0, "distance_max": 10} - coords, attrs = separate_coord_info(obj) - assert set(coords) == {"distance"} - assert attrs == { - "time_bob": 1, - } - - def test_dim_inference_skips_unsplittable_keys(self): - """Unsplittable keys should be ignored while inferring dims.""" - obj = { - "distance": 1, - "distance_bob": 2, - "distance_min": 0, - "distance_max": 10, - } - coords, attrs = separate_coord_info(obj) - assert "distance" in coords - assert attrs == {} - - def test_coord_level_to_summary(self): - """Coord-level values exposing to_summary should be normalized.""" - - class CoordLike: - def to_summary(self): - return dc.core.CoordSummary(min=0, max=1, step=1) - - coords, attrs = separate_coord_info({"coords": {"time": CoordLike()}}) - assert attrs == {} - assert coords["time"]["min"] == 0 - - def test_coord_manager_input_uses_dims_and_summary_dict(self, random_patch): - """CoordManager inputs should use dims and to_summary_dict paths.""" - coords, attrs = separate_coord_info({"coords": random_patch.coords}) - assert attrs == {} - assert set(coords) == set(random_patch.coords.coord_map) - - -class TestValidateNoCoords: - """Tests for stripping or rejecting coordinate metadata from attrs-like input.""" - - def test_raise_on_coord_fields(self): - """Coordinate-like flat keys should raise by default.""" - with pytest.raises(ValueError, match="distance"): - _validate_no_coords({"distance_units": "miles"}) - - def test_raise_on_coords_container(self, random_patch): - """Nested coord containers should raise by default.""" - with pytest.raises(ValueError, match="coords"): - _validate_no_coords({"coords": random_patch.coords}) - - def test_raise_false_strips_coord_fields(self): - """raise_error=False should remove coordinate metadata and keep attrs.""" - out = _validate_no_coords( - {"distance_units": "miles", "tag": "x"}, raise_error=False - ) - assert out == {"tag": "x"} - - def test_raise_false_strips_fallback_coord_fields(self): - """Flat coord-only fields left by splitting should still be removed.""" - out = _validate_no_coords( - {"distance_units": "miles", "time_dtype": "datetime64[ns]", "tag": "x"}, - raise_error=False, - ) - assert out == {"tag": "x"} - - def test_raise_false_strips_coords_and_dims(self, random_patch): - """raise_error=False should remove nested structural coord metadata.""" - out = _validate_no_coords( - {"coords": random_patch.coords, "dims": random_patch.dims, "tag": "x"}, - raise_error=False, - ) - assert out == {"tag": "x"} - - -class TestRaiseIfCoordAttrUpdates: - """Tests for rejecting coord-summary keys in attr updates.""" - - def test_none_is_noop(self): - """None inputs should return without raising.""" - assert _raise_if_coord_attr_updates(None) is None diff --git a/tests/test_utils/test_coordmanager_utils.py b/tests/test_utils/test_coordmanager_utils.py index d00cbf66f..163ffa3ec 100644 --- a/tests/test_utils/test_coordmanager_utils.py +++ b/tests/test_utils/test_coordmanager_utils.py @@ -21,7 +21,7 @@ def _get_offset_coord_manager(self, cm, from_max=True, **kwargs): coord = cm.coord_map[name] start = coord.max() if from_max else coord.min() attr_name = f"{name}_min" - new, _ = cm.update_from_attrs({attr_name: start + value}) + new = cm.update(**{attr_name: start + value}) return new @pytest.fixture(scope="class")