From 4cb58afa79de429968e303c3d4ea85d7b5039303 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sun, 16 Aug 2026 20:01:10 +0200 Subject: [PATCH 1/5] Let an annotation set read and write itself A set is stored as a directory naming its three parts -- attrs, the annotations table, and the vertices any path or polygon needs -- or as a bare CSV whose dimensions the caller states. dc.annotations is the one door every source goes through, as dc.spool is for patches. Reading a table types its cells before the models see it: a dimension column holds numbers or times, a basis cell holds the JSON its curve dumps, and everything else is read the way it was written. --- dascore/__init__.py | 1 + dascore/core/annotation_loader.py | 344 +++++++++++++++++ dascore/core/annotations.py | 219 ++++++++++- dascore/exceptions.py | 4 + tests/test_core/test_annotation_loader.py | 426 ++++++++++++++++++++++ tests/test_core/test_annotations.py | 26 ++ 6 files changed, 1005 insertions(+), 15 deletions(-) create mode 100644 dascore/core/annotation_loader.py create mode 100644 tests/test_core/test_annotation_loader.py diff --git a/dascore/__init__.py b/dascore/__init__.py index 0006ccb81..b1e385ae5 100644 --- a/dascore/__init__.py +++ b/dascore/__init__.py @@ -11,6 +11,7 @@ from dascore.core.attrs import PatchAttrs from dascore.core.summary import PatchSummary from dascore.core.spool import BaseSpool, Spool, spool +from dascore.core.annotation_loader import annotations from dascore.core.annotations import AnnotationSet from dascore.core.inventory import Inventory from dascore.core.inventory_loader import inventory diff --git a/dascore/core/annotation_loader.py b/dascore/core/annotation_loader.py new file mode 100644 index 000000000..e739c1fe3 --- /dev/null +++ b/dascore/core/annotation_loader.py @@ -0,0 +1,344 @@ +""" +Read annotation sets from storage. + +A set is stored either as a directory naming what it holds -- ``attrs`` +stating the dimensions and provenance, ``annotations.csv`` holding one row +per annotation, and ``vertices.csv`` where any path or polygon needs one -- +or as a bare table whose dimensions the caller states. + +CSV has no types, so this module decides what each column holds before the +models see it: a dimension column is numbers or times, a ``basis`` cell is +the JSON document its curve dumps, and every other cell is read the way it +was written. Tables are read strictly, through +[`read_table`](`dascore.utils.tables.read_table`), and the neutral errors +that raises are named as annotation errors here, at the one boundary which +knows the format. +""" + +from __future__ import annotations + +import json +import os +from collections.abc import Mapping, Sequence +from contextlib import suppress +from pathlib import Path +from typing import Any + +import numpy as np +import pandas as pd + +from dascore.core.annotations import ( + _END, + _START, + _VERTEX_COLUMNS, + ANNOTATION_STEM, + ATTRS_STEM, + TABLE_SUFFIX, + VERTEX_STEM, + AnnotationSet, +) +from dascore.exceptions import InvalidAnnotationError, ParameterError +from dascore.models.registry import TAG_FIELD +from dascore.utils.misc import optional_import +from dascore.utils.paths import quote_path +from dascore.utils.tables import parse_cell, read_table +from dascore.utils.time import to_datetime64 + +# The spellings an attrs file takes; the table stems come from the set, +# which writes the names this reads. +_OBJECT_SUFFIXES = (".yaml", ".yml", ".json") + +# What an attrs file declares itself to be; the model writes its own tag. +_SET_TAG = "AnnotationSetAttrs" + +# Columns whose cells stay text however they are spelled: an id which +# looks like a number is still the label the vertices name it by. +_TEXT_COLUMNS = frozenset( + {"id", "group", "tags", "parent", "geometry", "acquisition_key"} +) + +# The column a vertex states its place in the order by; a number. +_ORDINAL = _VERTEX_COLUMNS[1] + + +def _read_object(path: Path) -> dict[str, Any]: + """Parse one YAML or JSON object file into a mapping.""" + try: + text = path.read_text(encoding="utf-8-sig") + except (OSError, UnicodeDecodeError) as error: + msg = f"Could not read {quote_path(path)}: {error}." + raise ParameterError(msg) from error + if path.suffix == ".json": + try: + data = json.loads(text) + except ValueError as error: + msg = f"Could not parse JSON from {quote_path(path)}: {error}." + raise ParameterError(msg) from error + else: + yaml = optional_import("yaml", required_for="YAML annotation storage") + try: + data = yaml.safe_load(text) + except yaml.YAMLError as error: + msg = f"Could not parse YAML from {quote_path(path)}: {error}." + raise ParameterError(msg) from error + if not isinstance(data, Mapping): + msg = f"{quote_path(path)} holds no mapping, so it states no attributes." + raise ParameterError(msg) + return dict(data) + + +def _one_spelling(directory: Path, stem: str, suffixes: Sequence[str]) -> Path | None: + """Return the one file a stem names, or None; two spellings raise.""" + found = [x for x in (directory / f"{stem}{y}" for y in suffixes) if x.exists()] + if len(found) > 1: + listed = ", ".join(sorted(x.name for x in found)) + msg = ( + f"{quote_path(directory)} states {stem} more than once: {listed}. " + "A set spells each of its parts once." + ) + raise ParameterError(msg) + return found[0] if found else None + + +def _read_attrs(directory: Path) -> dict[str, Any]: + """Return the attributes a set directory states, which may be none.""" + path = _one_spelling(directory, ATTRS_STEM, _OBJECT_SUFFIXES) + if path is None: + return {} + data = _read_object(path) + declared = data.pop(TAG_FIELD, None) + if declared is not None and declared != _SET_TAG: + msg = ( + f"{quote_path(path)} declares {declared!r}, but the attributes of " + f"an annotation set declare {_SET_TAG!r}." + ) + raise ParameterError(msg) + return data + + +def _read_dimension(series: pd.Series, path: Path) -> pd.Series: + """ + Read a dimension column as the numbers or times its cells state. + + Numbers are tried first because every datetime spelling this writes is + an ISO string, which is not a number, while seconds from the epoch are + a number a distance column would lose to a date. + """ + stated = series.notna() + if not stated.any(): + return series + with suppress(TypeError, ValueError): + return pd.to_numeric(series) + try: + values = to_datetime64(series[stated].to_numpy(dtype=str)) + except (TypeError, ValueError) as error: + msg = ( + f"The column {series.name!r} of {quote_path(path)} states neither " + f"numbers nor times: {error}." + ) + raise ParameterError(msg) from error + out = pd.Series( + np.datetime64("NaT", "ns"), index=series.index, dtype="datetime64[ns]" + ) + out[stated] = values + return out + + +def _read_ordinal(series: pd.Series, path: Path) -> pd.Series: + """Read the vertex order column as the numbers it states.""" + try: + return pd.to_numeric(series) + except (TypeError, ValueError) as error: + msg = ( + f"{quote_path(path)} has a non-numeric {_ORDINAL}: {error}. A vertex " + "states its place in the order as a number." + ) + raise ParameterError(msg) from error + + +def _read_basis(series: pd.Series, path: Path) -> pd.Series: + """Read a basis column as the documents its cells hold.""" + + def read(cell): + if not isinstance(cell, str): + return cell + try: + return json.loads(cell) + except ValueError as error: + msg = ( + f"A basis in {quote_path(path)} is not a JSON document: {error}. " + "A stored basis is what its curve dumps." + ) + raise ParameterError(msg) from error + + return series.map(read) + + +def _dimension_spellings(dims: Sequence[str]) -> frozenset[str]: + """Every column name a declared dimension may be spelled with.""" + return frozenset(x for dim in dims for x in (dim, f"{dim}{_START}", f"{dim}{_END}")) + + +def _read_cells(frame: pd.DataFrame, dims: Sequence[str], path: Path) -> pd.DataFrame: + """Read a table's text cells as the values each column holds.""" + spellings = _dimension_spellings(dims) + out = {} + for name in frame.columns: + series = frame[name] + if str(name) in spellings: + out[name] = _read_dimension(series, path) + elif str(name) == _ORDINAL: + out[name] = _read_ordinal(series, path) + elif str(name) == "basis": + out[name] = _read_basis(series, path) + elif str(name) in _TEXT_COLUMNS: + out[name] = series + else: + out[name] = series.map(lambda x: parse_cell(x) if isinstance(x, str) else x) + return pd.DataFrame(out) + + +def _read_set_table(path: Path, dims: Sequence[str], what: str) -> pd.DataFrame | None: + """ + Read one of a set's tables, with its cells typed. + + A table stating nothing at all is what a set of no annotations writes, + so it reads back as none rather than as a table with no columns. + """ + if _is_blank(path): + return None + return _read_cells(read_table(path, what=what), dims, path) + + +def _is_blank(path: Path) -> bool: + """Whether a table holds nothing but whitespace.""" + try: + return not path.read_text(encoding="utf-8-sig").strip() + except (OSError, UnicodeDecodeError): + # Unreadable is the table reader's to name, with its own message. + return False + + +def _refuse_stray_tables(directory: Path) -> None: + """ + Refuse a table whose name names no part of a set. + + A ``vertexes.csv`` beside an ``annotations.csv`` claims to participate + in this convention and gets it wrong, which is worth more than being + quietly skipped. + """ + known = {ANNOTATION_STEM, VERTEX_STEM} + stray = sorted( + x.name for x in directory.glob(f"*{TABLE_SUFFIX}") if x.stem not in known + ) + if stray: + msg = ( + f"{quote_path(directory)} holds the table(s) {', '.join(stray)}, which " + f"name no part of a set. A set states {ANNOTATION_STEM}{TABLE_SUFFIX} " + f"and, where it has vertices, {VERTEX_STEM}{TABLE_SUFFIX}." + ) + raise ParameterError(msg) + + +def _load_directory(directory: Path, dims, **kwargs) -> AnnotationSet: + """Load the set a directory holds.""" + attrs = _read_attrs(directory) + _refuse_stray_tables(directory) + table = directory / f"{ANNOTATION_STEM}{TABLE_SUFFIX}" + if not table.exists(): + msg = ( + f"{quote_path(directory)} holds no {ANNOTATION_STEM}{TABLE_SUFFIX}, " + "so it states no annotations." + ) + raise ParameterError(msg) + stated = _declared_dims(attrs, dims, directory) + frame = _read_set_table(table, stated, "no annotations") + vertex_path = directory / f"{VERTEX_STEM}{TABLE_SUFFIX}" + vertices = None + if vertex_path.exists(): + vertices = _read_set_table(vertex_path, stated, "no vertices") + return AnnotationSet(frame, dims=dims, vertices=vertices, attrs=attrs, **kwargs) + + +def _load_file(path: Path, dims, **kwargs) -> AnnotationSet: + """Load the set a bare table holds.""" + if path.suffix.lower() != TABLE_SUFFIX: + msg = ( + f"{quote_path(path)} is not a table an annotation set is read from. " + f"A bare set is a {TABLE_SUFFIX} file; a set with vertices is a " + "directory." + ) + raise ParameterError(msg) + stated = _declared_dims({}, dims, path) + return AnnotationSet( + _read_set_table(path, stated, "no annotations"), dims=stated, **kwargs + ) + + +def _declared_dims(attrs: Mapping, dims, source: Path) -> tuple[str, ...]: + """ + Return the dimensions a source states, from its attrs or the caller. + + The cells cannot be read before this is known -- which columns hold + times rather than text is exactly what a dimension decides -- so a + source stating none fails here rather than as a puzzling column later. + """ + stated = dims if dims is not None else attrs.get("dims") + if not stated: + msg = ( + f"{quote_path(source)} states no dimensions, and none were given. " + "Annotations are read in the dimensions they are stated in: write " + f"them in {ATTRS_STEM}{_OBJECT_SUFFIXES[0]} or pass " + "dims=('distance', 'time')." + ) + raise ParameterError(msg) + return tuple(str(x) for x in stated) + + +def annotations( + source: AnnotationSet | str | os.PathLike | Any = None, + dims: Sequence[str] | None = None, + **kwargs, +) -> AnnotationSet: + """ + Load annotations from whatever holds them. + + The one door every source goes through, as + [`dascore.spool`](`dascore.spool`) is for patches: a set comes back + from a set, a directory, a table on disk, or anything a dataframe can + be built from. + + Parameters + ---------- + source + An `AnnotationSet`, a path to a set directory or a CSV table, or a + dataframe of one row per annotation. + dims + The patch dimensions the annotations are stated in. Required unless + the source states them itself. + **kwargs + Passed to [`AnnotationSet`](`dascore.core.annotations.AnnotationSet`). + + Examples + -------- + >>> import pandas as pd + >>> import dascore as dc + >>> frame = pd.DataFrame({"group": ["event"], "distance": [10.0]}) + >>> picks = dc.annotations(frame, dims=("distance",)) + >>> len(picks) + 1 + """ + if isinstance(source, AnnotationSet): + return source + if isinstance(source, str | os.PathLike): + path = Path(source) + try: + if path.is_dir(): + return _load_directory(path, dims, **kwargs) + if path.exists(): + return _load_file(path, dims, **kwargs) + except ParameterError as error: + raise InvalidAnnotationError(str(error)) from error + msg = f"{quote_path(path)} does not exist, so it holds no annotations." + raise InvalidAnnotationError(msg) + return AnnotationSet(source, dims=dims, **kwargs) diff --git a/dascore/core/annotations.py b/dascore/core/annotations.py index 00bcd831b..679e38850 100644 --- a/dascore/core/annotations.py +++ b/dascore/core/annotations.py @@ -17,7 +17,13 @@ from __future__ import annotations import datetime +import json + +# Whole rather than by name: this module's own Path is a geometry. +import pathlib +import re from collections.abc import Iterable, Mapping, Sequence +from contextlib import suppress from typing import Annotated, Any, ClassVar, Literal, NamedTuple import numpy as np @@ -47,7 +53,12 @@ ) from dascore.utils.intervals import normalize_value, value_kind from dascore.utils.mapping import FrozenDict -from dascore.utils.misc import iterate, to_str, validate_acquisition_key +from dascore.utils.misc import ( + iterate, + optional_import, + to_str, + validate_acquisition_key, +) from dascore.utils.time import to_datetime64, to_timedelta64 # Columns any set may carry, whatever dimensions it declares. @@ -73,6 +84,13 @@ # The vertices frame's own scaffolding; every other column is a dimension. _VERTEX_COLUMNS = ("id", "seq") +# The three parts a stored set spells itself with, and the suffix a table +# takes. The loader reads these names; `save` writes them. +ATTRS_STEM = "attrs" +ANNOTATION_STEM = "annotations" +VERTEX_STEM = "vertices" +TABLE_SUFFIX = ".csv" + # What a range column is spelled with. _START, _END = "_start", "_end" @@ -112,10 +130,11 @@ def _document(value): # A coordinate may be a time, which json has no type for. Written as the -# string DASCore writes every datetime as; reading it back as a time again -# is the loader's job, since it knows what each dimension holds. One -# serializer rather than two stacked: a python-mode dump is what equality -# compares and what `new` rebuilds from, so it keeps the values themselves. +# string DASCore writes every datetime as; `_coordinate` reads that +# spelling back, so a document holds the coordinates it was dumped from +# without anything downstream having to know which dimension is a time. +# One serializer rather than two stacked: a python-mode dump is what +# equality compares and what `new` rebuilds from, so it keeps the values. def _serialize_coordinates(value, info): """Write a mapping of coordinates, as a document only in json mode.""" if info.mode != "json": @@ -123,12 +142,43 @@ def _serialize_coordinates(value, info): return {k: [_document(x) for x in values] for k, values in value.items()} +# Exactly the spelling `to_str` gives a datetime64: a date, optionally a +# time after it. Anything looser would read a label as a coordinate. +_DATETIME_TEXT = re.compile(r"^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}:\d{2}(\.\d+)?)?$") + + +def _coordinate(value): + """Read one coordinate, a datetime written as text becoming one again.""" + if not (isinstance(value, str) and _DATETIME_TEXT.match(value)): + return value + # Shaped like a date without being one: a label reading '2020-13-45' + # is still the label it was written as. + with suppress(ValueError, TypeError): + return to_datetime64(value) + return value + + +def _read_coordinates(value): + """Read a mapping of coordinate sequences.""" + if not isinstance(value, Mapping): + return value + return {k: [_coordinate(x) for x in iterate(v)] for k, v in value.items()} + + +def _read_place(value): + """Read a mapping of one coordinate per dimension.""" + if not isinstance(value, Mapping): + return value + return {k: _coordinate(v) for k, v in value.items()} + + _freeze_map = AfterValidator(lambda x: FrozenDict(x)) _write_map = PlainSerializer(_serialize_coordinates, return_type=dict) +_read_map = BeforeValidator(_read_coordinates) -Bounds = Annotated[Mapping[str, tuple[Any, Any]], _freeze_map, _write_map] +Bounds = Annotated[Mapping[str, tuple[Any, Any]], _read_map, _freeze_map, _write_map] -Vertices = Annotated[Mapping[str, tuple[Any, ...]], _freeze_map, _write_map] +Vertices = Annotated[Mapping[str, tuple[Any, ...]], _read_map, _freeze_map, _write_map] def _serialize_place(value, info): @@ -140,6 +190,7 @@ def _serialize_place(value, info): Point = Annotated[ Mapping[str, Any], + BeforeValidator(_read_place), _freeze_map, PlainSerializer(_serialize_place, return_type=dict), ] @@ -613,14 +664,14 @@ def __init__( self._attrs = _build_attrs( attrs, dims, creation_info, acquisition_key, history, columns ) - frame = _coerce_frame(data, "annotations") + frame = _normalize_times(_coerce_frame(data, "annotations")) spellings = _read_spellings(frame, self._attrs.dims) _check_columns(frame, self._attrs) _check_ranges(frame, spellings) _check_values(frame) ids = _check_ids(frame) - _check_basis(frame, self._attrs.dims) - vertex_frame = _coerce_frame(vertices, "vertices") + frame = _normalize_tags(_normalize_basis(frame, self._attrs.dims)) + vertex_frame = _normalize_times(_coerce_frame(vertices, "vertices")) self._vertices = _check_vertices(vertex_frame, frame, ids, self._attrs.dims) self._df = _fill_vertex_bounds(frame, self._vertices, spellings) # Read again: filling a derived bounding region adds the range @@ -647,6 +698,59 @@ def to_vertices(self) -> pd.DataFrame: """Return the vertices of every path and polygon as a tidy dataframe.""" return self._vertices.copy() + # --- writing the set out + + def to_csv(self, path=None) -> str: + """ + Return the annotations as CSV text, optionally writing it to a path. + + A bare table states one grain, so a set holding vertices is written + with [save](`dascore.core.annotations.AnnotationSet.save`) instead; + this is the spelling for a set of regions. The set's dimensions are + not part of the table, so reading one back states them again. + + Parameters + ---------- + path + Where to write the text, or None to only return it. + """ + if not self._vertices.empty: + msg = ( + "This set holds vertices, which a bare table has no row for. " + "Save it as a directory, which states its vertices beside its " + "annotations." + ) + raise ParameterError(msg) + return _write_table(self._df, path) + + def save(self, path) -> pathlib.Path: + """ + Write the set to a directory, creating it if needed. + + The directory states the set in three parts: what it is and which + dimensions it holds, its annotations, and -- where any path or + polygon needs them -- its vertices. It reads back through + [dascore.annotations](`dascore.annotations`). + + Parameters + ---------- + path + The directory to write into. + """ + yaml = optional_import("yaml", required_for="YAML annotation storage") + directory = pathlib.Path(path) + directory.mkdir(parents=True, exist_ok=True) + # Defaults are dropped, so the document says what the set says; + # dims has no default, so it is always written. The attributes name + # their own model, which is what the file holds. + document = self._attrs.model_dump(mode="json", exclude_defaults=True) + with open(directory / f"{ATTRS_STEM}.yaml", "w") as stream: + stream.write(yaml.safe_dump(document, sort_keys=False)) + _write_table(self._df, directory / f"{ANNOTATION_STEM}{TABLE_SUFFIX}") + if not self._vertices.empty: + _write_table(self._vertices, directory / f"{VERTEX_STEM}{TABLE_SUFFIX}") + return directory + # --- what the set holds def __len__(self) -> int: @@ -1149,12 +1253,54 @@ def _freeze(value): return _scalar(value) -def _check_basis(frame, dims) -> None: - """Read every stated curve at load, so a bad one is not found later.""" +def _normalize_basis(frame, dims) -> pd.DataFrame: + """ + Replace every basis cell with the curve it states. + + Reading them at load is what catches a bad one before a row is asked + for; keeping what was read is what makes a set hold one spelling of a + curve, so a set written out and read back is the set it was rather + than the same curves spelled as documents. + """ if "basis" not in frame.columns: - return - for value in frame["basis"]: - _read_basis(value, dims) + return frame + read = [_read_basis(x, dims) for x in frame["basis"]] + return frame.assign(basis=pd.Series(read, index=frame.index, dtype=object)) + + +def _normalize_times(frame: pd.DataFrame) -> pd.DataFrame: + """ + Hold every time at nanoseconds, the resolution DASCore keeps them at. + + A column arriving at another resolution states the same times, but + everything which reads one back -- a stored table, a coordinate, a + curve -- states them at DASCore's, so a set which kept both spellings + would differ from itself over nothing. + """ + changed = {} + for name in frame.columns: + series = frame[name] + kind = getattr(series.dtype, "kind", "") + if kind == "M" and series.dtype != np.dtype("datetime64[ns]"): + changed[name] = to_datetime64(series) + elif kind == "m" and series.dtype != np.dtype("timedelta64[ns]"): + changed[name] = to_timedelta64(series) + if not changed: + return frame + # Assigned by item rather than by keyword: a column need not be named + # anything a keyword can spell. + out = frame.copy() + for name, series in changed.items(): + out[name] = series + return out + + +def _normalize_tags(frame) -> pd.DataFrame: + """Replace every tags cell with the tags it states, one spelling.""" + if "tags" not in frame.columns: + return frame + read = [_read_tags(x) or None for x in frame["tags"]] + return frame.assign(tags=pd.Series(read, index=frame.index, dtype=object)) def _read_basis(value, dims): @@ -1196,6 +1342,49 @@ def _read_tags(value) -> tuple[str, ...]: return (str(value),) +def _write_table(frame: pd.DataFrame, path=None) -> str: + """Return a frame as CSV text, optionally writing it to a path.""" + spelled = pd.DataFrame({name: _writable(frame[name]) for name in frame.columns}) + text = spelled.to_csv(index=False) + if path is not None: + with open(path, "w", newline="", encoding="utf-8") as stream: + stream.write(text) + return text + + +def _writable(series: pd.Series) -> pd.Series: + """Return one column as the text a table states it with.""" + return series.map(_writable_cell) + + +def _writable_cell(value): + """ + Spell one cell the way a table holds it. + + A value a CSV has no column shape for is written as its own document: + a basis as the JSON its curve dumps, a sequence as the comma-separated + list `tags` is read from. An extra holding a nested object survives as + that text rather than as the object, which is what a table can say. + """ + if not _stated(value): + return value + # Through _scalar first: mapping a datetime column hands over pandas + # Timestamps, which str() spells with a space where numpy uses a T, + # and only the numpy spelling reads back as a time. + value = _scalar(value) + if isinstance(value, np.datetime64 | np.timedelta64): + return to_str(value) + if isinstance(value, AnnotationBasis): + return json.dumps(value.model_dump(mode="json")) + if isinstance(value, str): + return value + if isinstance(value, Mapping): + return json.dumps(dict(value), default=_document) + if isinstance(value, Iterable): + return ", ".join(str(_writable_cell(x)) for x in value) + return value + + def _scalar(value): """ Return a cell's value as the plainest thing which still says it. diff --git a/dascore/exceptions.py b/dascore/exceptions.py index df9f90301..9d4c1cb23 100644 --- a/dascore/exceptions.py +++ b/dascore/exceptions.py @@ -173,5 +173,9 @@ class InvalidInventoryError(ValueError, DASCoreError): """Raised when inventory metadata violates the DASDAE inventory model.""" +class InvalidAnnotationError(ValueError, DASCoreError): + """Raised when stored annotations violate the DASCore annotation model.""" + + class InvalidModelTagError(ValueError, DASCoreError): """Raised when a serialized document names its model class illegally.""" diff --git a/tests/test_core/test_annotation_loader.py b/tests/test_core/test_annotation_loader.py new file mode 100644 index 000000000..b772458bd --- /dev/null +++ b/tests/test_core/test_annotation_loader.py @@ -0,0 +1,426 @@ +"""Tests for reading and writing stored annotation sets.""" + +from __future__ import annotations + +import json + +import numpy as np +import pandas as pd +import pytest + +import dascore as dc +from dascore.core.annotations import Line, Moveout +from dascore.exceptions import InvalidAnnotationError, ParameterError + +DIMS = ("distance", "time") + + +@pytest.fixture +def curve() -> Moveout: + """A moveout a path may be drawn from.""" + return Moveout( + apex_distance=100.0, + apex_time=np.datetime64("2020-01-01T00:00:05"), + velocity=1500.0, + standoff=30.0, + distance_start=0.0, + distance_end=200.0, + ) + + +@pytest.fixture +def regions() -> dc.AnnotationSet: + """A set of regions, which a bare table can hold.""" + frame = pd.DataFrame( + { + "id": ["r1", "r2"], + "group": ["noise", "noise"], + "tags": [("road", "car"), None], + "distance_start": [120.0, 10.0], + "distance_end": [340.0, 60.0], + "time_start": [ + np.datetime64("2020-01-01T00:00:10"), + np.datetime64("2020-01-01T00:00:20"), + ], + "time_end": [ + np.datetime64("2020-01-01T00:00:12"), + np.datetime64("2020-01-01T00:00:22"), + ], + "note": ["traffic", "walker"], + "score": [0.9, 0.2], + "checked": [True, False], + } + ) + return dc.AnnotationSet( + frame, dims=DIMS, acquisition_key="NET.ARR.00.das", history=("decimate",) + ) + + +@pytest.fixture +def with_vertices(curve) -> dc.AnnotationSet: + """A set holding a hand-drawn path and one drawn from a curve.""" + drawn = curve.vertices(5) + vertices = pd.DataFrame( + { + "id": ["p1", "p1", "p1", *["p2"] * 5], + "seq": [0, 1, 2, *range(5)], + "distance": [10.0, 95.0, 185.0, *drawn["distance"]], + "time": [ + np.datetime64("2020-01-01T00:00:00.1"), + np.datetime64("2020-01-01T00:00:01"), + np.datetime64("2020-01-01T00:00:01.9"), + *drawn["time"], + ], + } + ) + frame = pd.DataFrame( + { + "id": ["p1", "p2", "r1"], + "group": ["picks", "picks", "noise"], + "geometry": ["path", "path", "region"], + "basis": [None, curve, None], + "distance_start": [np.nan, np.nan, 5.0], + "distance_end": [np.nan, np.nan, 15.0], + } + ) + return dc.AnnotationSet(frame, dims=DIMS, vertices=vertices) + + +class TestRoundTrip: + """A set written out and read back is the set it was.""" + + def test_regions_through_a_directory(self, regions, tmp_path): + """Bounds, extras and provenance all survive a directory.""" + regions.save(tmp_path / "picks") + assert dc.annotations(tmp_path / "picks") == regions + + def test_regions_through_a_bare_table(self, regions, tmp_path): + """A set of regions is a table, and its dims are stated again.""" + path = tmp_path / "picks.csv" + regions.to_csv(path) + loaded = dc.annotations(path, dims=DIMS) + assert loaded.to_dataframe().equals(regions.to_dataframe()) + + def test_vertices_and_basis(self, with_vertices, curve, tmp_path): + """Vertices and the curve they were drawn from both survive.""" + with_vertices.save(tmp_path / "picks") + loaded = dc.annotations(tmp_path / "picks") + assert loaded == with_vertices + assert loaded[1].geometry.basis == curve + + def test_extras_keep_their_kind(self, regions, tmp_path): + """A cell written as a number or a boolean reads back as one.""" + loaded = dc.annotations(regions.save(tmp_path / "picks")) + assert loaded[0].extra["score"] == 0.9 + assert loaded[0].extra["checked"] is True + assert loaded[1].extra["checked"] is False + + def test_tags_keep_their_shape(self, regions, tmp_path): + """Tags are one spelling however they arrive.""" + loaded = dc.annotations(regions.save(tmp_path / "picks")) + assert loaded[0].tags == ("road", "car") + assert loaded[1].tags == () + + def test_times_keep_their_type(self, regions, tmp_path): + """A time endpoint reads back as a time, not as its text.""" + loaded = dc.annotations(regions.save(tmp_path / "picks")) + start, _ = loaded[0].region.bounds["time"] + assert isinstance(start, np.datetime64) + + def test_line_basis(self, tmp_path): + """A line survives a round trip as the curve it is.""" + line = Line( + start={"distance": 0.0, "time": np.datetime64("2020-01-01")}, + end={"distance": 50.0, "time": np.datetime64("2020-01-01")}, + ) + vertices = pd.DataFrame( + { + "id": ["p1", "p1"], + "seq": [0, 1], + "distance": [0.0, 50.0], + "time": [np.datetime64("2020-01-01")] * 2, + } + ) + frame = pd.DataFrame({"id": ["p1"], "geometry": ["path"], "basis": [line]}) + annotations = dc.AnnotationSet(frame, dims=DIMS, vertices=vertices) + loaded = dc.annotations(annotations.save(tmp_path / "picks")) + assert loaded[0].geometry.basis == line + + def test_an_unstated_bound(self, tmp_path): + """An empty dimension cell reads back as unconstrained.""" + frame = pd.DataFrame( + { + "group": ["a", "b"], + "distance_start": [1.0, np.nan], + "distance_end": [2.0, np.nan], + } + ) + annotations = dc.AnnotationSet(frame, dims=("distance",)) + loaded = dc.annotations(annotations.save(tmp_path / "picks")) + assert loaded == annotations + assert "distance" not in loaded[1].region.bounds + + +class TestTheDoor: + """Everything a set may be loaded from goes through one function.""" + + def test_a_set_is_itself(self, regions): + """Loading a set which is already loaded hands it back.""" + assert dc.annotations(regions) is regions + + def test_a_dataframe(self): + """A frame becomes a set, as the constructor makes one.""" + frame = pd.DataFrame({"group": ["a"], "distance": [1.0]}) + assert len(dc.annotations(frame, dims=("distance",))) == 1 + + def test_nothing(self): + """A set of nothing is still a set.""" + assert len(dc.annotations(dims=("distance",))) == 0 + + def test_a_path_which_is_not_there(self, tmp_path): + """A path naming nothing says so, rather than reading nothing.""" + with pytest.raises(InvalidAnnotationError, match="does not exist"): + dc.annotations(tmp_path / "missing", dims=DIMS) + + def test_a_file_which_is_not_a_table(self, tmp_path): + """Only a table is a bare set.""" + path = tmp_path / "picks.txt" + path.write_text("group\na\n") + with pytest.raises(InvalidAnnotationError, match="not a table"): + dc.annotations(path, dims=DIMS) + + def test_errors_are_annotation_errors(self, tmp_path): + """The neutral errors the table reader raises are named here.""" + directory = tmp_path / "picks" + directory.mkdir() + (directory / "annotations.csv").write_text("group\nnoise,extra\n") + with pytest.raises(InvalidAnnotationError, match="states 2 cells"): + dc.annotations(directory, dims=DIMS) + + def test_a_set_of_none(self, tmp_path): + """A set of no annotations writes an empty table and reads back.""" + empty = dc.annotations(dims=DIMS) + assert dc.annotations(empty.save(tmp_path / "picks")) == empty + + +class TestDeclaringDimensions: + """Cells cannot be read before the dimensions are known.""" + + def test_stated_by_the_attrs(self, regions, tmp_path): + """A directory states its own dimensions.""" + assert dc.annotations(regions.save(tmp_path / "picks")).dims == DIMS + + def test_stated_by_the_caller(self, regions, tmp_path): + """A bare table has the caller state them.""" + path = tmp_path / "picks.csv" + regions.to_csv(path) + assert dc.annotations(path, dims=DIMS).dims == DIMS + + def test_stated_by_neither(self, regions, tmp_path): + """A source stating none fails saying how to state them.""" + path = tmp_path / "picks.csv" + regions.to_csv(path) + with pytest.raises(InvalidAnnotationError, match="states no dimensions"): + dc.annotations(path) + + def test_the_caller_wins(self, regions, tmp_path): + """A caller stating dimensions states them for the whole read.""" + directory = regions.save(tmp_path / "picks") + assert dc.annotations(directory, dims=("time", "distance")).dims == ( + "time", + "distance", + ) + + +class TestTheAttrsFile: + """What a set directory says about itself.""" + + def test_json_spelling(self, regions, tmp_path): + """One data model stands behind both spellings.""" + directory = regions.save(tmp_path / "picks") + document = json.loads( + regions.attrs.model_dump_json(exclude_defaults=True), + ) + (directory / "attrs.json").write_text(json.dumps(document)) + (directory / "attrs.yaml").unlink() + assert dc.annotations(directory) == regions + + def test_two_spellings(self, regions, tmp_path): + """A set spells each of its parts once.""" + directory = regions.save(tmp_path / "picks") + (directory / "attrs.json").write_text("{}") + with pytest.raises(InvalidAnnotationError, match="more than once"): + dc.annotations(directory) + + def test_the_wrong_object(self, regions, tmp_path): + """A file declaring another model is a misfiled object.""" + directory = regions.save(tmp_path / "picks") + (directory / "attrs.yaml").write_text("object_type: Inventory\ndims: [time]\n") + with pytest.raises(InvalidAnnotationError, match="declares 'Inventory'"): + dc.annotations(directory) + + def test_which_is_not_a_mapping(self, regions, tmp_path): + """A document stating a list defines no attributes.""" + directory = regions.save(tmp_path / "picks") + (directory / "attrs.yaml").write_text("- distance\n- time\n") + with pytest.raises(InvalidAnnotationError, match="no mapping"): + dc.annotations(directory) + + def test_which_does_not_parse(self, regions, tmp_path): + """Unparseable YAML names the file rather than the parser.""" + directory = regions.save(tmp_path / "picks") + (directory / "attrs.yaml").write_text("dims: [\n") + with pytest.raises(InvalidAnnotationError, match="Could not parse YAML"): + dc.annotations(directory) + + def test_bad_json(self, regions, tmp_path): + """Unparseable JSON names the file too.""" + directory = regions.save(tmp_path / "picks") + (directory / "attrs.yaml").unlink() + (directory / "attrs.json").write_text("{") + with pytest.raises(InvalidAnnotationError, match="Could not parse JSON"): + dc.annotations(directory) + + def test_which_cannot_be_read(self, regions, tmp_path): + """A file which does not decode names itself, not the codec.""" + directory = regions.save(tmp_path / "picks") + (directory / "attrs.yaml").write_bytes(b"dims: [\xff\xfe]\n") + with pytest.raises(InvalidAnnotationError, match="Could not read"): + dc.annotations(directory) + + def test_no_attrs_file(self, regions, tmp_path): + """A directory without one is read on the caller's dimensions.""" + directory = regions.save(tmp_path / "picks") + (directory / "attrs.yaml").unlink() + assert dc.annotations(directory, dims=DIMS).dims == DIMS + + +class TestTheTables: + """What a set directory holds, and what it may not.""" + + def test_no_annotations_table(self, tmp_path): + """A directory without one states no annotations.""" + directory = tmp_path / "picks" + directory.mkdir() + with pytest.raises(InvalidAnnotationError, match=r"no annotations\.csv"): + dc.annotations(directory, dims=DIMS) + + def test_a_table_which_cannot_be_read(self, regions, tmp_path): + """A table which does not decode is the table reader's to name.""" + directory = regions.save(tmp_path / "picks") + (directory / "annotations.csv").write_bytes(b"group\n\xff\xfe\n") + with pytest.raises(InvalidAnnotationError, match="Could not read"): + dc.annotations(directory) + + def test_a_stray_table(self, regions, tmp_path): + """A near-miss on the convention raises rather than being skipped.""" + directory = regions.save(tmp_path / "picks") + (directory / "vertexes.csv").write_text("id,seq\n") + with pytest.raises(InvalidAnnotationError, match=r"vertexes\.csv"): + dc.annotations(directory) + + def test_a_basis_which_is_not_json(self, with_vertices, tmp_path): + """A stored basis is what its curve dumps.""" + directory = with_vertices.save(tmp_path / "picks") + table = directory / "annotations.csv" + table.write_text(table.read_text().replace('"{""object_type', '"{oops')) + with pytest.raises(InvalidAnnotationError, match="not a JSON document"): + dc.annotations(directory) + + def test_a_basis_which_is_not_a_curve(self, with_vertices, tmp_path): + """A document which parses but names no curve is still refused.""" + directory = with_vertices.save(tmp_path / "picks") + table = directory / "annotations.csv" + original = table.read_text() + start = original.index('"{""object_type') + end = original.index('"', start + 1) + while original[end : end + 2] == '""': + end = original.index('"', end + 2) + table.write_text(original[:start] + '"{}"' + original[end + 1 :]) + with pytest.raises(InvalidAnnotationError, match="as a curve"): + dc.annotations(directory) + + def test_a_non_numeric_seq(self, with_vertices, tmp_path): + """A vertex states its place in the order as a number.""" + directory = with_vertices.save(tmp_path / "picks") + table = directory / "vertices.csv" + table.write_text(table.read_text().replace("p1,0,", "p1,first,")) + with pytest.raises(InvalidAnnotationError, match="non-numeric seq"): + dc.annotations(directory) + + def test_a_dimension_which_is_neither(self, regions, tmp_path): + """A dimension column holds numbers or times, and says so.""" + directory = regions.save(tmp_path / "picks") + table = directory / "annotations.csv" + table.write_text(table.read_text().replace("120.0", "far")) + with pytest.raises(InvalidAnnotationError, match="neither numbers nor times"): + dc.annotations(directory) + + def test_a_dimension_no_row_states(self, tmp_path): + """A column every row leaves empty constrains nothing.""" + path = tmp_path / "picks.csv" + path.write_text("group,time_start,time_end\nquiet,,\n") + loaded = dc.annotations(path, dims=("time",)) + assert "time" not in loaded[0].region.bounds + + def test_an_id_which_looks_like_a_number(self, tmp_path): + """An id is the label its vertices name it by, never a number.""" + frame = pd.DataFrame({"id": ["1"], "geometry": ["path"]}) + vertices = pd.DataFrame( + {"id": ["1", "1"], "seq": [0, 1], "distance": [1.0, 2.0]} + ) + annotations = dc.AnnotationSet(frame, dims=("distance",), vertices=vertices) + loaded = dc.annotations(annotations.save(tmp_path / "picks")) + assert loaded[0].id == "1" + + +class TestWriting: + """How a set spells itself out.""" + + def test_to_csv_returns_text(self, regions): + """The text comes back whether or not it is written.""" + text = regions.to_csv() + assert text.splitlines()[0].startswith("id,group,tags") + + def test_to_csv_refuses_vertices(self, with_vertices): + """A bare table states one grain.""" + with pytest.raises(ParameterError, match="holds vertices"): + with_vertices.to_csv() + + def test_save_makes_the_directory(self, regions, tmp_path): + """Saving into a directory which is not there makes it.""" + directory = regions.save(tmp_path / "deep" / "picks") + assert directory.is_dir() + + def test_save_writes_no_empty_vertices(self, regions, tmp_path): + """A set without vertices states no vertices table.""" + directory = regions.save(tmp_path / "picks") + assert not (directory / "vertices.csv").exists() + + def test_save_writes_what_it_holds(self, with_vertices, tmp_path): + """A set with vertices states all three parts.""" + directory = with_vertices.save(tmp_path / "picks") + written = {x.name for x in directory.iterdir()} + assert written == {"attrs.yaml", "annotations.csv", "vertices.csv"} + + def test_save_over_itself(self, regions, tmp_path): + """Saving twice into one directory rewrites it.""" + regions.save(tmp_path / "picks") + assert dc.annotations(regions.save(tmp_path / "picks")) == regions + + def test_times_are_written_unambiguously(self, regions, tmp_path): + """A time is written the way DASCore writes every datetime.""" + text = regions.to_csv() + assert "2020-01-01T00:00:10.000000000" in text + + def test_a_nested_extra_is_written_as_its_document(self, tmp_path): + """A cell a table has no column shape for is written as text.""" + frame = pd.DataFrame({"group": ["a"], "distance": [1.0], "meta": [{"n": 1}]}) + text = dc.AnnotationSet(frame, dims=("distance",)).to_csv() + assert '{""n"": 1}' in text + + def test_the_attrs_name_their_model(self, regions, tmp_path): + """The document says what it holds, as every stored object does.""" + directory = regions.save(tmp_path / "picks") + text = (directory / "attrs.yaml").read_text() + assert "object_type: AnnotationSetAttrs" in text diff --git a/tests/test_core/test_annotations.py b/tests/test_core/test_annotations.py index d59e8a87a..ab66159a8 100644 --- a/tests/test_core/test_annotations.py +++ b/tests/test_core/test_annotations.py @@ -1112,6 +1112,32 @@ def test_vertices_write_a_document(self): written = path.model_dump(mode="json")["vertices"]["time"] assert written == [str(TIMES[0]), str(TIMES[1])] + def test_datetime_bounds_read_back_as_times(self): + """A time written as text is a time again, not the text.""" + region = Region(bounds={"time": (TIMES[0], TIMES[2])}) + assert Region(**region.model_dump(mode="json")) == region + + def test_datetime_vertices_read_back_as_times(self): + """The same holds for a path's vertices and a line's endpoints.""" + line = Line(start={"time": TIMES[0]}, end={"time": TIMES[2]}) + path = Path( + region=Region(bounds={}), + vertices={"time": (TIMES[0], TIMES[1])}, + basis=line, + ) + assert Path(**path.model_dump(mode="json")) == path + + def test_a_label_is_not_a_time(self): + """Only the spelling DASCore writes a datetime with is read as one.""" + region = Region(bounds={"stage": ("2020-13-45", "before")}) + assert region.bounds["stage"] == ("2020-13-45", "before") + + @pytest.mark.parametrize("model", [Region, Line]) + def test_coordinates_which_are_not_a_mapping(self, model): + """A coordinate map which is not a map is pydantic's to refuse.""" + with pytest.raises(ValidationError): + model(bounds="everywhere", start="here", end="there") + def test_geometry_kinds_are_distinct(self): """A polygon is not a path which happens to close.""" assert not isinstance( From e4d4b830495f0249e656f043f62f50454528b110 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sun, 16 Aug 2026 20:20:58 +0200 Subject: [PATCH 2/5] Answer an adversarial pass over the store Attacking the round trip with hostile input found four ways a value could change meaning between save and load, and one way a stored document could escape as pydantic's report rather than as a bad file: - a column named seq in the annotations table was read as the vertex order, so an annotation carrying its own seq had to be a number - a tag holding a comma became two tags, which is now refused, since a comma is what separates one tag from the next - an empty cell and an empty string are one thing to a table, so a set says unset for both - a column no row states says nothing about what it holds, so its emptiness is no longer read as a type - dimensions stated as a bare string were a sequence of their own letters The attributes are also written as JSON rather than YAML. PyYAML is optional, so requiring it to store a set contradicted the zero-dependency floor the CSV tables are chosen for. YAML remains an accepted spelling for a set authored by hand. --- dascore/core/annotation_loader.py | 42 +++++++-- dascore/core/annotations.py | 62 ++++++++++--- tests/test_core/test_annotation_loader.py | 101 ++++++++++++++++++---- tests/test_core/test_annotations.py | 6 ++ 4 files changed, 176 insertions(+), 35 deletions(-) diff --git a/dascore/core/annotation_loader.py b/dascore/core/annotation_loader.py index e739c1fe3..86a05c70f 100644 --- a/dascore/core/annotation_loader.py +++ b/dascore/core/annotation_loader.py @@ -26,6 +26,7 @@ import numpy as np import pandas as pd +from pydantic import ValidationError from dascore.core.annotations import ( _END, @@ -179,26 +180,38 @@ def _dimension_spellings(dims: Sequence[str]) -> frozenset[str]: return frozenset(x for dim in dims for x in (dim, f"{dim}{_START}", f"{dim}{_END}")) -def _read_cells(frame: pd.DataFrame, dims: Sequence[str], path: Path) -> pd.DataFrame: - """Read a table's text cells as the values each column holds.""" +def _read_cells( + frame: pd.DataFrame, dims: Sequence[str], path: Path, ordered: bool = False +) -> pd.DataFrame: + """ + Read a table's text cells as the values each column holds. + + Only the vertices are ordered, so only they read ``seq`` as the number + it is: the annotations table does not reserve that name, and an + annotation carrying its own ``seq`` means whatever it says. + """ spellings = _dimension_spellings(dims) out = {} for name in frame.columns: series = frame[name] if str(name) in spellings: out[name] = _read_dimension(series, path) - elif str(name) == _ORDINAL: + elif ordered and str(name) == _ORDINAL: out[name] = _read_ordinal(series, path) elif str(name) == "basis": out[name] = _read_basis(series, path) - elif str(name) in _TEXT_COLUMNS: + elif str(name) in _TEXT_COLUMNS or series.isna().all(): + # A column no row states says nothing about what it holds, and + # reading its emptiness as a type would invent one. out[name] = series else: out[name] = series.map(lambda x: parse_cell(x) if isinstance(x, str) else x) return pd.DataFrame(out) -def _read_set_table(path: Path, dims: Sequence[str], what: str) -> pd.DataFrame | None: +def _read_set_table( + path: Path, dims: Sequence[str], what: str, ordered: bool = False +) -> pd.DataFrame | None: """ Read one of a set's tables, with its cells typed. @@ -207,7 +220,7 @@ def _read_set_table(path: Path, dims: Sequence[str], what: str) -> pd.DataFrame """ if _is_blank(path): return None - return _read_cells(read_table(path, what=what), dims, path) + return _read_cells(read_table(path, what=what), dims, path, ordered=ordered) def _is_blank(path: Path) -> bool: @@ -256,7 +269,7 @@ def _load_directory(directory: Path, dims, **kwargs) -> AnnotationSet: vertex_path = directory / f"{VERTEX_STEM}{TABLE_SUFFIX}" vertices = None if vertex_path.exists(): - vertices = _read_set_table(vertex_path, stated, "no vertices") + vertices = _read_set_table(vertex_path, stated, "no vertices", ordered=True) return AnnotationSet(frame, dims=dims, vertices=vertices, attrs=attrs, **kwargs) @@ -292,6 +305,16 @@ def _declared_dims(attrs: Mapping, dims, source: Path) -> tuple[str, ...]: "dims=('distance', 'time')." ) raise ParameterError(msg) + # A lone string is a sequence of its own letters, and typing the cells + # against eight one-character dimensions would fail somewhere far from + # here. One dimension is still stated as a list of one. + if isinstance(stated, str): + msg = ( + f"{quote_path(source)} states its dimensions as the string " + f"{stated!r}, which is a sequence of letters. State them as a " + f"list, even a list of one: ['{stated}']." + ) + raise ParameterError(msg) return tuple(str(x) for x in stated) @@ -337,7 +360,10 @@ def annotations( return _load_directory(path, dims, **kwargs) if path.exists(): return _load_file(path, dims, **kwargs) - except ParameterError as error: + # ValidationError too: a stored document which does not build the + # models is a bad file, and it is named as one here rather than + # arriving as pydantic's report on a call the caller did not make. + except (ParameterError, ValidationError) as error: raise InvalidAnnotationError(str(error)) from error msg = f"{quote_path(path)} does not exist, so it holds no annotations." raise InvalidAnnotationError(msg) diff --git a/dascore/core/annotations.py b/dascore/core/annotations.py index 679e38850..69c5202e6 100644 --- a/dascore/core/annotations.py +++ b/dascore/core/annotations.py @@ -53,12 +53,7 @@ ) from dascore.utils.intervals import normalize_value, value_kind from dascore.utils.mapping import FrozenDict -from dascore.utils.misc import ( - iterate, - optional_import, - to_str, - validate_acquisition_key, -) +from dascore.utils.misc import iterate, to_str, validate_acquisition_key from dascore.utils.time import to_datetime64, to_timedelta64 # Columns any set may carry, whatever dimensions it declares. @@ -664,7 +659,7 @@ def __init__( self._attrs = _build_attrs( attrs, dims, creation_info, acquisition_key, history, columns ) - frame = _normalize_times(_coerce_frame(data, "annotations")) + frame = _normalize_blanks(_normalize_times(_coerce_frame(data, "annotations"))) spellings = _read_spellings(frame, self._attrs.dims) _check_columns(frame, self._attrs) _check_ranges(frame, spellings) @@ -732,20 +727,23 @@ def save(self, path) -> pathlib.Path: polygon needs them -- its vertices. It reads back through [dascore.annotations](`dascore.annotations`). + The attributes are written as JSON rather than as YAML, so storing + a set needs nothing beyond the standard library; a set authored by + hand may spell them in YAML, which reads back the same. + Parameters ---------- path The directory to write into. """ - yaml = optional_import("yaml", required_for="YAML annotation storage") directory = pathlib.Path(path) directory.mkdir(parents=True, exist_ok=True) # Defaults are dropped, so the document says what the set says; # dims has no default, so it is always written. The attributes name # their own model, which is what the file holds. document = self._attrs.model_dump(mode="json", exclude_defaults=True) - with open(directory / f"{ATTRS_STEM}.yaml", "w") as stream: - stream.write(yaml.safe_dump(document, sort_keys=False)) + with open(directory / f"{ATTRS_STEM}.json", "w") as stream: + json.dump(document, stream, indent=2) _write_table(self._df, directory / f"{ANNOTATION_STEM}{TABLE_SUFFIX}") if not self._vertices.empty: _write_table(self._vertices, directory / f"{VERTEX_STEM}{TABLE_SUFFIX}") @@ -1296,13 +1294,51 @@ def _normalize_times(frame: pd.DataFrame) -> pd.DataFrame: def _normalize_tags(frame) -> pd.DataFrame: - """Replace every tags cell with the tags it states, one spelling.""" + """ + Replace every tags cell with the tags it states, one spelling. + + A comma is what separates one tag from the next, so a tag holding one + is refused rather than quietly becoming two the next time the set is + read. + """ if "tags" not in frame.columns: return frame read = [_read_tags(x) or None for x in frame["tags"]] + split = sorted({x for tags in read if tags for x in tags if "," in x}) + if split: + listed = ", ".join(repr(x) for x in split) + msg = ( + f"The tag(s) {listed} hold a comma, which is what separates one " + "tag from the next; a tag is one label." + ) + raise ParameterError(msg) return frame.assign(tags=pd.Series(read, index=frame.index, dtype=object)) +def _normalize_blanks(frame: pd.DataFrame) -> pd.DataFrame: + """ + Read a cell holding the empty string as stating nothing. + + A table cannot tell an empty cell from a cell holding no characters, + and reading one back says unset, so a set says unset too rather than + holding a value which cannot survive being written down. + """ + changed = {} + for name in frame.columns: + series = frame[name] + if getattr(series.dtype, "kind", "") not in "OTU": + continue + blank = series.map(lambda x: isinstance(x, str) and not x) + if blank.any(): + changed[name] = series.where(~blank, None) + if not changed: + return frame + out = frame.copy() + for name, series in changed.items(): + out[name] = series + return out + + def _read_basis(value, dims): """ Return the curve a cell states, which may be the model or its document. @@ -1365,6 +1401,10 @@ def _writable_cell(value): a basis as the JSON its curve dumps, a sequence as the comma-separated list `tags` is read from. An extra holding a nested object survives as that text rather than as the object, which is what a table can say. + + The same holds for an extra holding a time: only a declared dimension + is known to hold times, so only it is read back as one, and an extra + keeps the text it was written as. """ if not _stated(value): return value diff --git a/tests/test_core/test_annotation_loader.py b/tests/test_core/test_annotation_loader.py index b772458bd..cb7fd5d8b 100644 --- a/tests/test_core/test_annotation_loader.py +++ b/tests/test_core/test_annotation_loader.py @@ -8,6 +8,11 @@ import pandas as pd import pytest +try: + import yaml +except ImportError: + yaml = None + import dascore as dc from dascore.core.annotations import Line, Moveout from dascore.exceptions import InvalidAnnotationError, ParameterError @@ -161,6 +166,48 @@ def test_an_unstated_bound(self, tmp_path): assert "distance" not in loaded[1].region.bounds +class TestWhatATableCannotSay: + """A CSV has no types, and these are the corners where that shows.""" + + def test_an_extra_named_seq(self, tmp_path): + """Only the vertices order by seq; an annotation's is its own.""" + frame = pd.DataFrame( + {"group": ["a"], "distance": [1.0], "seq": ["third"]}, + ) + annotations = dc.AnnotationSet(frame, dims=("distance",)) + loaded = dc.annotations(annotations.save(tmp_path / "picks")) + assert loaded[0].extra["seq"] == "third" + + def test_an_empty_cell_is_unset(self, tmp_path): + """A table cannot tell an empty cell from an empty string.""" + frame = pd.DataFrame({"group": ["", "b"], "distance": [1.0, 2.0]}) + annotations = dc.AnnotationSet(frame, dims=("distance",)) + assert dc.annotations(annotations.save(tmp_path / "picks")) == annotations + assert annotations[0].group == "" + + def test_a_datetime_extra_reads_back_as_text(self, tmp_path): + """Only a declared dimension is known to hold times, so only it is read + as one; an extra keeps the text it was written as. + """ + frame = pd.DataFrame( + { + "group": ["a"], + "distance": [1.0], + "when": [np.datetime64("2020-01-01T00:00:00")], + } + ) + annotations = dc.AnnotationSet(frame, dims=("distance",)) + loaded = dc.annotations(annotations.save(tmp_path / "picks")) + assert loaded[0].extra["when"] == "2020-01-01T00:00:00.000000000" + + def test_a_numeric_looking_extra_reads_as_a_number(self, tmp_path): + """A cell is read the way its own text states it, as every table is.""" + frame = pd.DataFrame({"group": ["a"], "distance": [1.0], "zip": ["01234"]}) + annotations = dc.AnnotationSet(frame, dims=("distance",)) + loaded = dc.annotations(annotations.save(tmp_path / "picks")) + assert loaded[0].extra["zip"] == 1234 + + class TestTheDoor: """Everything a set may be loaded from goes through one function.""" @@ -223,6 +270,24 @@ def test_stated_by_neither(self, regions, tmp_path): with pytest.raises(InvalidAnnotationError, match="states no dimensions"): dc.annotations(path) + def test_stated_as_a_bare_string(self, tmp_path): + """A lone string is a sequence of its own letters, so it is refused.""" + directory = tmp_path / "picks" + directory.mkdir() + (directory / "attrs.json").write_text('{"dims": "distance"}') + (directory / "annotations.csv").write_text("group,distance\na,1.0\n") + with pytest.raises(InvalidAnnotationError, match="sequence of letters"): + dc.annotations(directory) + + def test_a_document_which_does_not_build(self, tmp_path): + """A bad stored document is named as a bad file, not as a bad call.""" + directory = tmp_path / "picks" + directory.mkdir() + (directory / "attrs.json").write_text('{"dims": ["distance"], "n": 1}') + (directory / "annotations.csv").write_text("group,distance\na,1.0\n") + with pytest.raises(InvalidAnnotationError, match="Extra inputs"): + dc.annotations(directory) + def test_the_caller_wins(self, regions, tmp_path): """A caller stating dimensions states them for the whole read.""" directory = regions.save(tmp_path / "picks") @@ -235,40 +300,45 @@ def test_the_caller_wins(self, regions, tmp_path): class TestTheAttrsFile: """What a set directory says about itself.""" - def test_json_spelling(self, regions, tmp_path): - """One data model stands behind both spellings.""" + @pytest.mark.skipif(yaml is None, reason="pyyaml is not installed") + def test_yaml_spelling(self, regions, tmp_path): + """One data model stands behind both spellings; a set may be authored + in the more readable one. + """ directory = regions.save(tmp_path / "picks") - document = json.loads( - regions.attrs.model_dump_json(exclude_defaults=True), - ) - (directory / "attrs.json").write_text(json.dumps(document)) - (directory / "attrs.yaml").unlink() + document = json.loads((directory / "attrs.json").read_text()) + (directory / "attrs.yaml").write_text(yaml.safe_dump(document)) + (directory / "attrs.json").unlink() assert dc.annotations(directory) == regions def test_two_spellings(self, regions, tmp_path): """A set spells each of its parts once.""" directory = regions.save(tmp_path / "picks") - (directory / "attrs.json").write_text("{}") + (directory / "attrs.yml").write_text("{}") with pytest.raises(InvalidAnnotationError, match="more than once"): dc.annotations(directory) def test_the_wrong_object(self, regions, tmp_path): """A file declaring another model is a misfiled object.""" directory = regions.save(tmp_path / "picks") - (directory / "attrs.yaml").write_text("object_type: Inventory\ndims: [time]\n") + (directory / "attrs.json").write_text( + '{"object_type": "Inventory", "dims": ["time"]}' + ) with pytest.raises(InvalidAnnotationError, match="declares 'Inventory'"): dc.annotations(directory) def test_which_is_not_a_mapping(self, regions, tmp_path): """A document stating a list defines no attributes.""" directory = regions.save(tmp_path / "picks") - (directory / "attrs.yaml").write_text("- distance\n- time\n") + (directory / "attrs.json").write_text('["distance", "time"]') with pytest.raises(InvalidAnnotationError, match="no mapping"): dc.annotations(directory) + @pytest.mark.skipif(yaml is None, reason="pyyaml is not installed") def test_which_does_not_parse(self, regions, tmp_path): """Unparseable YAML names the file rather than the parser.""" directory = regions.save(tmp_path / "picks") + (directory / "attrs.json").unlink() (directory / "attrs.yaml").write_text("dims: [\n") with pytest.raises(InvalidAnnotationError, match="Could not parse YAML"): dc.annotations(directory) @@ -276,7 +346,6 @@ def test_which_does_not_parse(self, regions, tmp_path): def test_bad_json(self, regions, tmp_path): """Unparseable JSON names the file too.""" directory = regions.save(tmp_path / "picks") - (directory / "attrs.yaml").unlink() (directory / "attrs.json").write_text("{") with pytest.raises(InvalidAnnotationError, match="Could not parse JSON"): dc.annotations(directory) @@ -284,14 +353,14 @@ def test_bad_json(self, regions, tmp_path): def test_which_cannot_be_read(self, regions, tmp_path): """A file which does not decode names itself, not the codec.""" directory = regions.save(tmp_path / "picks") - (directory / "attrs.yaml").write_bytes(b"dims: [\xff\xfe]\n") + (directory / "attrs.json").write_bytes(b'{"dims": ["\xff\xfe"]}') with pytest.raises(InvalidAnnotationError, match="Could not read"): dc.annotations(directory) def test_no_attrs_file(self, regions, tmp_path): """A directory without one is read on the caller's dimensions.""" directory = regions.save(tmp_path / "picks") - (directory / "attrs.yaml").unlink() + (directory / "attrs.json").unlink() assert dc.annotations(directory, dims=DIMS).dims == DIMS @@ -401,7 +470,7 @@ def test_save_writes_what_it_holds(self, with_vertices, tmp_path): """A set with vertices states all three parts.""" directory = with_vertices.save(tmp_path / "picks") written = {x.name for x in directory.iterdir()} - assert written == {"attrs.yaml", "annotations.csv", "vertices.csv"} + assert written == {"attrs.json", "annotations.csv", "vertices.csv"} def test_save_over_itself(self, regions, tmp_path): """Saving twice into one directory rewrites it.""" @@ -422,5 +491,5 @@ def test_a_nested_extra_is_written_as_its_document(self, tmp_path): def test_the_attrs_name_their_model(self, regions, tmp_path): """The document says what it holds, as every stored object does.""" directory = regions.save(tmp_path / "picks") - text = (directory / "attrs.yaml").read_text() - assert "object_type: AnnotationSetAttrs" in text + text = (directory / "attrs.json").read_text() + assert '"object_type": "AnnotationSetAttrs"' in text diff --git a/tests/test_core/test_annotations.py b/tests/test_core/test_annotations.py index ab66159a8..9b30551b2 100644 --- a/tests/test_core/test_annotations.py +++ b/tests/test_core/test_annotations.py @@ -380,6 +380,12 @@ def test_absent(self): """No tags is an empty tuple, not None.""" assert AnnotationSet(pd.DataFrame({"group": ["a"]}), dims=DIMS)[0].tags == () + def test_a_tag_holding_a_comma(self): + """A comma separates tags, so a tag holding one would become two.""" + frame = pd.DataFrame({"tags": [("a,b", "c")]}) + with pytest.raises(ParameterError, match="hold a comma"): + AnnotationSet(frame, dims=DIMS) + class TestIdentity: """Ids are the producer's, and nothing here invents one.""" From 6aaac29a33d5744a473ba4d8ac7a2927d9c22f07 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sun, 16 Aug 2026 20:55:17 +0200 Subject: [PATCH 3/5] Answer the adversarial review of the store Six blind reviews of the branch found defects the round-trip tests did not. Three of them independently flagged the same two: - `_DATETIME_TEXT` required a seconds field, so an hour- or minute- resolution datetime64 -- which numpy writes without one -- read back as a string and then raised on arithmetic. The comment claiming the pattern was exactly what `to_str` writes was wrong for two of the eight resolutions. - `save` wrote the parts a set had without clearing the parts it did not, so a stale vertices table, or the YAML the attributes used to be spelled in, left a directory which loaded before the save refusing to load after it. The rest, each with a test: - a value column a table would read back as another kind is refused at the write, rather than written and then refused at the read - overrides given to a source which states them for itself now raise: a built set silently dropped them, a directory raised a bare TypeError - a declared non-nanosecond datetime dtype says a set holds times at nanoseconds, rather than blaming data the caller cannot change - a dimension column holding date-like text is read as times, so the frame and the region built from it stop disagreeing - padded and empty tags are held as they read back - a nested extra json has no type for is written as its text, not left to die as a circular reference - a cell reading 'nan' stays text rather than being deleted as unset - object files are matched without regard to case, as the inventory matches its own - one dimension may be a bare string from a file, as it already could in memory - the text columns derive from the reserved ones, so a column added to the set cannot silently fall through to cell typing --- dascore/core/annotation_loader.py | 123 +++++++++++++----- dascore/core/annotations.py | 141 ++++++++++++++++++--- tests/test_core/test_annotation_loader.py | 145 +++++++++++++++++++++- tests/test_core/test_annotations.py | 51 +++++++- 4 files changed, 402 insertions(+), 58 deletions(-) diff --git a/dascore/core/annotation_loader.py b/dascore/core/annotation_loader.py index 86a05c70f..999dc3a6b 100644 --- a/dascore/core/annotation_loader.py +++ b/dascore/core/annotation_loader.py @@ -1,10 +1,10 @@ """ Read annotation sets from storage. -A set is stored either as a directory naming what it holds -- ``attrs`` -stating the dimensions and provenance, ``annotations.csv`` holding one row -per annotation, and ``vertices.csv`` where any path or polygon needs one -- -or as a bare table whose dimensions the caller states. +A set is stored either as a directory naming what it holds -- always +``annotations.csv``, one row per annotation; ``attrs`` where it states its +own dimensions and provenance; ``vertices.csv`` where any path or polygon +needs one -- or as a bare table whose dimensions the caller states. CSV has no types, so this module decides what each column holds before the models see it: a dimension column is numbers or times, a ``basis`` cell is @@ -18,6 +18,7 @@ from __future__ import annotations import json +import math import os from collections.abc import Mapping, Sequence from contextlib import suppress @@ -34,29 +35,28 @@ _VERTEX_COLUMNS, ANNOTATION_STEM, ATTRS_STEM, + OBJECT_SUFFIXES, + RESERVED_COLUMNS, TABLE_SUFFIX, VERTEX_STEM, AnnotationSet, ) from dascore.exceptions import InvalidAnnotationError, ParameterError from dascore.models.registry import TAG_FIELD -from dascore.utils.misc import optional_import +from dascore.utils.misc import iterate, optional_import from dascore.utils.paths import quote_path from dascore.utils.tables import parse_cell, read_table from dascore.utils.time import to_datetime64 -# The spellings an attrs file takes; the table stems come from the set, -# which writes the names this reads. -_OBJECT_SUFFIXES = (".yaml", ".yml", ".json") - # What an attrs file declares itself to be; the model writes its own tag. _SET_TAG = "AnnotationSetAttrs" # Columns whose cells stay text however they are spelled: an id which -# looks like a number is still the label the vertices name it by. -_TEXT_COLUMNS = frozenset( - {"id", "group", "tags", "parent", "geometry", "acquisition_key"} -) +# looks like a number is still the label the vertices name it by. Derived +# rather than listed, so a column added to the reserved set is text by +# default rather than silently falling through to `parse_cell`. `value` +# holds whichever kind its group holds, and `basis` holds a document. +_TEXT_COLUMNS = frozenset(RESERVED_COLUMNS) - {"value", "basis"} # The column a vertex states its place in the order by; a number. _ORDINAL = _VERTEX_COLUMNS[1] @@ -69,7 +69,10 @@ def _read_object(path: Path) -> dict[str, Any]: except (OSError, UnicodeDecodeError) as error: msg = f"Could not read {quote_path(path)}: {error}." raise ParameterError(msg) from error - if path.suffix == ".json": + # Casefolded, as the inventory matches its object files: a shouted + # ATTRS.JSON is the file attrs.json would be, and reading it as YAML + # for its spelling would fail on a document which is not wrong. + if path.suffix.casefold() == OBJECT_SUFFIXES[0]: try: data = json.loads(text) except ValueError as error: @@ -90,7 +93,11 @@ def _read_object(path: Path) -> dict[str, Any]: def _one_spelling(directory: Path, stem: str, suffixes: Sequence[str]) -> Path | None: """Return the one file a stem names, or None; two spellings raise.""" - found = [x for x in (directory / f"{stem}{y}" for y in suffixes) if x.exists()] + found = [ + x + for x in sorted(directory.iterdir()) + if x.stem == stem and x.suffix.casefold() in suffixes + ] if len(found) > 1: listed = ", ".join(sorted(x.name for x in found)) msg = ( @@ -103,7 +110,7 @@ def _one_spelling(directory: Path, stem: str, suffixes: Sequence[str]) -> Path | def _read_attrs(directory: Path) -> dict[str, Any]: """Return the attributes a set directory states, which may be none.""" - path = _one_spelling(directory, ATTRS_STEM, _OBJECT_SUFFIXES) + path = _one_spelling(directory, ATTRS_STEM, OBJECT_SUFFIXES) if path is None: return {} data = _read_object(path) @@ -205,10 +212,26 @@ def _read_cells( # reading its emptiness as a type would invent one. out[name] = series else: - out[name] = series.map(lambda x: parse_cell(x) if isinstance(x, str) else x) + out[name] = series.map(_read_extra) return pd.DataFrame(out) +def _read_extra(cell): + """ + Read one cell of a column the set does not model. + + A cell reading 'nan' or 'inf' parses as a float which every later + reader treats as unset, so the value would be deleted rather than + retyped; those stay the text the table plainly states. + """ + if not isinstance(cell, str): + return cell + value = parse_cell(cell) + if isinstance(value, float) and not math.isfinite(value): + return cell + return value + + def _read_set_table( path: Path, dims: Sequence[str], what: str, ordered: bool = False ) -> pd.DataFrame | None: @@ -253,8 +276,33 @@ def _refuse_stray_tables(directory: Path) -> None: raise ParameterError(msg) +def _refuse_overrides(what: str, **stated) -> None: + """ + Refuse an argument a source states for itself. + + Silently dropping one is the worse failure: a caller passing + ``dims=patch.dims`` to whatever it was handed would get the source's + dimensions from one kind of source and its own from another, with + nothing said either way. + """ + given = sorted(k for k, v in stated.items() if v is not None) + if given: + msg = ( + f"{', '.join(given)} was given for {what}, which states it. " + "Read it and change it, rather than reading it as something else." + ) + raise ParameterError(msg) + + def _load_directory(directory: Path, dims, **kwargs) -> AnnotationSet: """Load the set a directory holds.""" + # Both are the directory's to state, and passing them through would + # reach AnnotationSet twice as a bare TypeError. + _refuse_overrides( + "a set directory", + attrs=kwargs.pop("attrs", None), + vertices=kwargs.pop("vertices", None), + ) attrs = _read_attrs(directory) _refuse_stray_tables(directory) table = directory / f"{ANNOTATION_STEM}{TABLE_SUFFIX}" @@ -270,7 +318,10 @@ def _load_directory(directory: Path, dims, **kwargs) -> AnnotationSet: vertices = None if vertex_path.exists(): vertices = _read_set_table(vertex_path, stated, "no vertices", ordered=True) - return AnnotationSet(frame, dims=dims, vertices=vertices, attrs=attrs, **kwargs) + # The read dimensions rather than the given ones: they are the same + # names, already a tuple, so a file spelling one dimension as a bare + # string builds the set the same way the constructor would. + return AnnotationSet(frame, dims=stated, vertices=vertices, attrs=attrs, **kwargs) def _load_file(path: Path, dims, **kwargs) -> AnnotationSet: @@ -290,7 +341,7 @@ def _load_file(path: Path, dims, **kwargs) -> AnnotationSet: def _declared_dims(attrs: Mapping, dims, source: Path) -> tuple[str, ...]: """ - Return the dimensions a source states, from its attrs or the caller. + Return the dimensions to read a source in: the caller's, else its own. The cells cannot be read before this is known -- which columns hold times rather than text is exactly what a dimension decides -- so a @@ -301,21 +352,16 @@ def _declared_dims(attrs: Mapping, dims, source: Path) -> tuple[str, ...]: msg = ( f"{quote_path(source)} states no dimensions, and none were given. " "Annotations are read in the dimensions they are stated in: write " - f"them in {ATTRS_STEM}{_OBJECT_SUFFIXES[0]} or pass " + f"them in {ATTRS_STEM}{OBJECT_SUFFIXES[0]} or pass " "dims=('distance', 'time')." ) raise ParameterError(msg) - # A lone string is a sequence of its own letters, and typing the cells - # against eight one-character dimensions would fail somewhere far from - # here. One dimension is still stated as a list of one. - if isinstance(stated, str): - msg = ( - f"{quote_path(source)} states its dimensions as the string " - f"{stated!r}, which is a sequence of letters. State them as a " - f"list, even a list of one: ['{stated}']." - ) - raise ParameterError(msg) - return tuple(str(x) for x in stated) + # Through `iterate`, as the set itself reads them: a lone string is one + # dimension rather than a sequence of its own letters, and typing the + # cells against eight one-character dimensions would fail somewhere far + # from here. Refusing it instead would leave the same input accepted + # in memory and rejected from a file. + return tuple(str(x) for x in iterate(stated)) def annotations( @@ -338,9 +384,12 @@ def annotations( dataframe of one row per annotation. dims The patch dimensions the annotations are stated in. Required unless - the source states them itself. + the source states them itself, and overriding it where it does. **kwargs Passed to [`AnnotationSet`](`dascore.core.annotations.AnnotationSet`). + A source already holding what one states -- a set, or a directory + holding its own attributes and vertices -- refuses it rather than + dropping it. Examples -------- @@ -350,8 +399,18 @@ def annotations( >>> picks = dc.annotations(frame, dims=("distance",)) >>> len(picks) 1 + + A set is handed straight back, so a function taking either a set or a + path may simply call this on whatever it was given. + + >>> dc.annotations(picks) is picks + True """ if isinstance(source, AnnotationSet): + # A built set states everything these would override, and building + # it again from its frame would quietly drop whatever the overrides + # did not restate. + _refuse_overrides("a set which is already built", dims=dims, **kwargs) return source if isinstance(source, str | os.PathLike): path = Path(source) diff --git a/dascore/core/annotations.py b/dascore/core/annotations.py index 69c5202e6..cd8c4af3f 100644 --- a/dascore/core/annotations.py +++ b/dascore/core/annotations.py @@ -54,6 +54,7 @@ from dascore.utils.intervals import normalize_value, value_kind from dascore.utils.mapping import FrozenDict from dascore.utils.misc import iterate, to_str, validate_acquisition_key +from dascore.utils.tables import parse_cell from dascore.utils.time import to_datetime64, to_timedelta64 # Columns any set may carry, whatever dimensions it declares. @@ -79,12 +80,14 @@ # The vertices frame's own scaffolding; every other column is a dimension. _VERTEX_COLUMNS = ("id", "seq") -# The three parts a stored set spells itself with, and the suffix a table -# takes. The loader reads these names; `save` writes them. +# The three parts a stored set spells itself with, and the suffixes each +# takes. The loader reads these names; `save` writes them, and clears the +# spellings it supersedes, which is why both need the whole list. ATTRS_STEM = "attrs" ANNOTATION_STEM = "annotations" VERTEX_STEM = "vertices" TABLE_SUFFIX = ".csv" +OBJECT_SUFFIXES = (".json", ".yaml", ".yml") # What a range column is spelled with. _START, _END = "_start", "_end" @@ -137,9 +140,13 @@ def _serialize_coordinates(value, info): return {k: [_document(x) for x in values] for k, values in value.items()} -# Exactly the spelling `to_str` gives a datetime64: a date, optionally a -# time after it. Anything looser would read a label as a coordinate. -_DATETIME_TEXT = re.compile(r"^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}:\d{2}(\.\d+)?)?$") +# Every spelling `to_str` gives a datetime64 from a whole date down. Numpy +# writes only the fields the value's unit carries, so a `datetime64[m]` is +# '2020-01-01T12:30' with no seconds to match, and requiring them read an +# ordinary pick time back as a string. A bare year or month is left out on +# purpose: '2020' is as readily a label as a time, and nothing tells them +# apart. +_DATETIME_TEXT = re.compile(r"^\d{4}-\d{2}-\d{2}(T\d{2}(:\d{2}(:\d{2}(\.\d+)?)?)?)?$") def _coordinate(value): @@ -659,14 +666,17 @@ def __init__( self._attrs = _build_attrs( attrs, dims, creation_info, acquisition_key, history, columns ) - frame = _normalize_blanks(_normalize_times(_coerce_frame(data, "annotations"))) + frame = _coerce_frame(data, "annotations") + frame = _normalize_blanks(_normalize_times(frame, self._attrs.dims)) spellings = _read_spellings(frame, self._attrs.dims) _check_columns(frame, self._attrs) _check_ranges(frame, spellings) _check_values(frame) ids = _check_ids(frame) frame = _normalize_tags(_normalize_basis(frame, self._attrs.dims)) - vertex_frame = _normalize_times(_coerce_frame(vertices, "vertices")) + vertex_frame = _normalize_times( + _coerce_frame(vertices, "vertices"), self._attrs.dims + ) self._vertices = _check_vertices(vertex_frame, frame, ids, self._attrs.dims) self._df = _fill_vertex_bounds(frame, self._vertices, spellings) # Read again: filling a derived bounding region adds the range @@ -731,22 +741,49 @@ def save(self, path) -> pathlib.Path: a set needs nothing beyond the standard library; a set authored by hand may spell them in YAML, which reads back the same. + Writing states the whole directory, so a part this set does not + have is removed rather than left behind. A stale vertices table, or + the YAML the attributes used to be spelled in, would otherwise sit + beside what was written and leave a directory which loaded before + the save refusing to load after it. + Parameters ---------- path The directory to write into. + + Returns + ------- + The directory written to, so a save reads straight back. """ directory = pathlib.Path(path) directory.mkdir(parents=True, exist_ok=True) + vertex_table = directory / f"{VERTEX_STEM}{TABLE_SUFFIX}" + attrs_file = directory / f"{ATTRS_STEM}{OBJECT_SUFFIXES[0]}" + # Only the spellings this format claims, and matched the way the + # loader matches them: a notes.txt or an attrs.bak beside them + # participates in no convention and is not this function's to + # delete, while a shouted attrs.YAML is one the loader would read. + superseded = [ + x + for x in directory.iterdir() + if x.stem == ATTRS_STEM + and x.suffix.casefold() in OBJECT_SUFFIXES + and x != attrs_file + ] + if self._vertices.empty: + superseded.append(vertex_table) + for stale in superseded: + stale.unlink(missing_ok=True) # Defaults are dropped, so the document says what the set says; # dims has no default, so it is always written. The attributes name # their own model, which is what the file holds. document = self._attrs.model_dump(mode="json", exclude_defaults=True) - with open(directory / f"{ATTRS_STEM}.json", "w") as stream: + with open(attrs_file, "w") as stream: json.dump(document, stream, indent=2) _write_table(self._df, directory / f"{ANNOTATION_STEM}{TABLE_SUFFIX}") if not self._vertices.empty: - _write_table(self._vertices, directory / f"{VERTEX_STEM}{TABLE_SUFFIX}") + _write_table(self._vertices, vertex_table) return directory # --- what the set holds @@ -926,9 +963,18 @@ def _check_columns(frame: pd.DataFrame, attrs: AnnotationSetAttrs) -> None: raise ParameterError(msg) from error # Compared by name rather than by identity: a column documented as # `category` says it is categorical, not which categories it holds, - # and the two dtypes are otherwise unequal. Names still tell a - # datetime64[ns] from a datetime64[us]. + # and the two dtypes are otherwise unequal. if declared.name != actual.name: + # A time is held at nanoseconds whatever it arrived as, so + # another unit is not a column this set could ever hold, and + # saying it "holds datetime64[ns]" reads as a mistake the + # caller could correct by supplying different data. + if declared.kind in "Mm": + msg = ( + f"The column {name!r} states dtype {column.dtype}, but a set " + f"holds every time at nanoseconds: state {actual} instead." + ) + raise ParameterError(msg) msg = f"The column {name!r} states dtype {column.dtype} but holds {actual}." raise ParameterError(msg) @@ -1266,7 +1312,15 @@ def _normalize_basis(frame, dims) -> pd.DataFrame: return frame.assign(basis=pd.Series(read, index=frame.index, dtype=object)) -def _normalize_times(frame: pd.DataFrame) -> pd.DataFrame: +def _states_times(series: pd.Series) -> bool: + """Whether every cell a column states is a datetime written as text.""" + stated = [x for x in series if _stated(x)] + return bool(stated) and all( + isinstance(x, str) and _DATETIME_TEXT.match(x) for x in stated + ) + + +def _normalize_times(frame: pd.DataFrame, dims: Sequence[str] = ()) -> pd.DataFrame: """ Hold every time at nanoseconds, the resolution DASCore keeps them at. @@ -1274,7 +1328,13 @@ def _normalize_times(frame: pd.DataFrame) -> pd.DataFrame: everything which reads one back -- a stored table, a coordinate, a curve -- states them at DASCore's, so a set which kept both spellings would differ from itself over nothing. + + A dimension column holding times as *text* is read as times for the + same reason: the geometry a row builds reads that spelling back, so a + frame which kept the text would disagree with the region built from + it about what the row says. """ + spelled = {x for dim in dims for x in (dim, f"{dim}{_START}", f"{dim}{_END}")} changed = {} for name in frame.columns: series = frame[name] @@ -1283,6 +1343,8 @@ def _normalize_times(frame: pd.DataFrame) -> pd.DataFrame: changed[name] = to_datetime64(series) elif kind == "m" and series.dtype != np.dtype("timedelta64[ns]"): changed[name] = to_timedelta64(series) + elif str(name) in spelled and _states_times(series): + changed[name] = to_datetime64(series.astype(str)).where(series.notna()) if not changed: return frame # Assigned by item rather than by keyword: a column need not be named @@ -1305,6 +1367,13 @@ def _normalize_tags(frame) -> pd.DataFrame: return frame read = [_read_tags(x) or None for x in frame["tags"]] split = sorted({x for tags in read if tags for x in tags if "," in x}) + # Held as `_read_tags` will read them back: the writer joins with a + # comma and the reader strips and drops the empties, so a tag padded + # with spaces or a tag holding nothing would not survive being + # written down. + read = [ + tuple(y.strip() for y in x if y.strip()) or None if x else None for x in read + ] if split: listed = ", ".join(repr(x) for x in split) msg = ( @@ -1319,9 +1388,9 @@ def _normalize_blanks(frame: pd.DataFrame) -> pd.DataFrame: """ Read a cell holding the empty string as stating nothing. - A table cannot tell an empty cell from a cell holding no characters, - and reading one back says unset, so a set says unset too rather than - holding a value which cannot survive being written down. + A table writes an unset cell and a cell holding the empty string the + same way, and reads that back as unset, so a set says unset for both + rather than holding a value which cannot survive being written down. """ changed = {} for name in frame.columns: @@ -1378,8 +1447,35 @@ def _read_tags(value) -> tuple[str, ...]: return (str(value),) +def _refuse_ambiguous_values(frame: pd.DataFrame) -> None: + """ + Refuse a value a table would read back as a different kind. + + An extra losing its type is a documented cost of a format with none, + but `value` is a column the set models and checks -- a group holds one + kind of value -- so a string reading back as a boolean or a number can + make a group mix kinds, leaving a directory this library wrote and + then refuses to read. Better to refuse the write. + """ + if "value" not in frame.columns: + return + ambiguous = sorted( + {x for x in frame["value"] if isinstance(x, str) and parse_cell(x) != x} + ) + if ambiguous: + listed = ", ".join(repr(x) for x in ambiguous) + msg = ( + f"The value(s) {listed} are text a table would read back as a " + "boolean or a number, and a group holds one kind of value. A " + "table has no way to mark a cell as text; spell the value as " + "something only text can be." + ) + raise ParameterError(msg) + + def _write_table(frame: pd.DataFrame, path=None) -> str: """Return a frame as CSV text, optionally writing it to a path.""" + _refuse_ambiguous_values(frame) spelled = pd.DataFrame({name: _writable(frame[name]) for name in frame.columns}) text = spelled.to_csv(index=False) if path is not None: @@ -1393,6 +1489,19 @@ def _writable(series: pd.Series) -> pd.Series: return series.map(_writable_cell) +def _json_default(value): + """ + Spell a nested value json has no type of its own for. + + A hook which hands back what it was given is re-dispatched until json + reports a circular reference, so anything this cannot spell goes in as + its text: a bare `ValueError: Circular reference detected` names + neither the cell nor the file it was being written to. + """ + spelled = _writable_cell(value) + return spelled if spelled is not value else str(value) + + def _writable_cell(value): """ Spell one cell the way a table holds it. @@ -1419,7 +1528,7 @@ def _writable_cell(value): if isinstance(value, str): return value if isinstance(value, Mapping): - return json.dumps(dict(value), default=_document) + return json.dumps(dict(value), default=_json_default) if isinstance(value, Iterable): return ", ".join(str(_writable_cell(x)) for x in value) return value diff --git a/tests/test_core/test_annotation_loader.py b/tests/test_core/test_annotation_loader.py index cb7fd5d8b..086094625 100644 --- a/tests/test_core/test_annotation_loader.py +++ b/tests/test_core/test_annotation_loader.py @@ -200,6 +200,42 @@ def test_a_datetime_extra_reads_back_as_text(self, tmp_path): loaded = dc.annotations(annotations.save(tmp_path / "picks")) assert loaded[0].extra["when"] == "2020-01-01T00:00:00.000000000" + def test_an_ambiguous_value_is_refused_at_the_write(self, tmp_path): + """A value column a table would retype could make a group mix kinds, + so the set refuses to write a store it would not read. + """ + frame = pd.DataFrame( + {"group": ["phase"] * 2, "value": ["P", "true"], "distance": [1.0, 2.0]} + ) + annotations = dc.AnnotationSet(frame, dims=("distance",)) + with pytest.raises(ParameterError, match="read back as a boolean"): + annotations.save(tmp_path / "picks") + + def test_an_unambiguous_value_still_writes(self, tmp_path): + """Only text a table would read as another kind is refused.""" + frame = pd.DataFrame( + {"group": ["phase"] * 2, "value": ["P", "S"], "distance": [1.0, 2.0]} + ) + annotations = dc.AnnotationSet(frame, dims=("distance",)) + assert dc.annotations(annotations.save(tmp_path / "picks")) == annotations + + def test_a_non_finite_looking_extra_stays_text(self, tmp_path): + """A cell reading 'nan' is text, not a value which then vanishes.""" + frame = pd.DataFrame({"group": ["a"], "distance": [1.0], "note": ["nan"]}) + annotations = dc.AnnotationSet(frame, dims=("distance",)) + loaded = dc.annotations(annotations.save(tmp_path / "picks")) + assert loaded[0].extra["note"] == "nan" + + def test_an_extra_some_rows_leave_blank(self, tmp_path): + """A blank cell is unset; the rows which state one still read.""" + frame = pd.DataFrame( + {"group": ["a", "b"], "distance": [1.0, 2.0], "note": ["seen", None]} + ) + annotations = dc.AnnotationSet(frame, dims=("distance",)) + loaded = dc.annotations(annotations.save(tmp_path / "picks")) + assert loaded[0].extra["note"] == "seen" + assert "note" not in loaded[1].extra + def test_a_numeric_looking_extra_reads_as_a_number(self, tmp_path): """A cell is read the way its own text states it, as every table is.""" frame = pd.DataFrame({"group": ["a"], "distance": [1.0], "zip": ["01234"]}) @@ -208,6 +244,48 @@ def test_a_numeric_looking_extra_reads_as_a_number(self, tmp_path): assert loaded[0].extra["zip"] == 1234 +class TestSavingOverASet: + """Writing states the whole directory, not only the parts it has.""" + + def test_a_stale_vertices_table_is_cleared(self, with_vertices, regions, tmp_path): + """A set without vertices leaves none behind for the next read.""" + directory = tmp_path / "picks" + with_vertices.save(directory) + regions.save(directory) + assert not (directory / "vertices.csv").exists() + assert dc.annotations(directory) == regions + + @pytest.mark.skipif(yaml is None, reason="pyyaml is not installed") + def test_a_hand_authored_yaml_is_superseded(self, tmp_path): + """Saving a set read from YAML does not leave two attrs files.""" + directory = tmp_path / "picks" + directory.mkdir() + (directory / "attrs.yaml").write_text(yaml.safe_dump({"dims": list(DIMS)})) + (directory / "annotations.csv").write_text("group,distance\nnoise,1.0\n") + loaded = dc.annotations(directory) + loaded.save(directory) + assert not (directory / "attrs.yaml").exists() + assert dc.annotations(directory) == loaded + + def test_a_file_owing_this_format_nothing_is_left(self, regions, tmp_path): + """Only the spellings a set claims are cleared.""" + directory = regions.save(tmp_path / "picks") + (directory / "attrs.bak").write_text("mine") + regions.save(directory) + assert (directory / "attrs.bak").read_text() == "mine" + + def test_a_shouted_spelling_is_read_and_superseded(self, regions, tmp_path): + """A file is matched by its name, not by its case.""" + directory = regions.save(tmp_path / "picks") + (directory / "attrs.json").rename(directory / "attrs.JSON") + loaded = dc.annotations(directory) + assert loaded == regions + loaded.save(directory) + assert {x.name for x in directory.iterdir() if x.stem == "attrs"} == { + "attrs.json" + } + + class TestTheDoor: """Everything a set may be loaded from goes through one function.""" @@ -215,6 +293,21 @@ def test_a_set_is_itself(self, regions): """Loading a set which is already loaded hands it back.""" assert dc.annotations(regions) is regions + def test_a_set_refuses_overrides(self, regions): + """Silently dropping them would make one door mean two things.""" + with pytest.raises(ParameterError, match="already built"): + dc.annotations(regions, dims=("time", "distance")) + with pytest.raises(ParameterError, match="already built"): + dc.annotations(regions, acquisition_key="N.A.00.das") + + def test_a_directory_refuses_what_it_states(self, regions, tmp_path): + """A directory holds its own attributes and vertices.""" + directory = regions.save(tmp_path / "picks") + with pytest.raises(InvalidAnnotationError, match="a set directory"): + dc.annotations(directory, attrs={"dims": DIMS}) + with pytest.raises(InvalidAnnotationError, match="a set directory"): + dc.annotations(directory, vertices=pd.DataFrame()) + def test_a_dataframe(self): """A frame becomes a set, as the constructor makes one.""" frame = pd.DataFrame({"group": ["a"], "distance": [1.0]}) @@ -270,14 +363,21 @@ def test_stated_by_neither(self, regions, tmp_path): with pytest.raises(InvalidAnnotationError, match="states no dimensions"): dc.annotations(path) - def test_stated_as_a_bare_string(self, tmp_path): - """A lone string is a sequence of its own letters, so it is refused.""" + @pytest.mark.parametrize("source", ["file", "caller"]) + def test_stated_as_a_bare_string(self, tmp_path, source): + """One dimension may be a lone string, as the constructor takes it, + rather than a sequence of its own letters. + """ directory = tmp_path / "picks" directory.mkdir() - (directory / "attrs.json").write_text('{"dims": "distance"}') + stated = '{"dims": "distance"}' if source == "file" else "{}" + (directory / "attrs.json").write_text(stated) (directory / "annotations.csv").write_text("group,distance\na,1.0\n") - with pytest.raises(InvalidAnnotationError, match="sequence of letters"): - dc.annotations(directory) + dims = None if source == "file" else "distance" + loaded = dc.annotations(directory, dims=dims) + assert loaded.dims == ("distance",) + # The cells were typed against one dimension, not eight letters. + assert loaded[0].region.bounds["distance"] == (1.0, 1.0) def test_a_document_which_does_not_build(self, tmp_path): """A bad stored document is named as a bad file, not as a bad call.""" @@ -432,6 +532,28 @@ def test_a_dimension_no_row_states(self, tmp_path): loaded = dc.annotations(path, dims=("time",)) assert "time" not in loaded[0].region.bounds + def test_a_minute_resolution_time(self, tmp_path): + """Numpy writes only the fields a unit carries, and they all read back.""" + frame = pd.DataFrame( + { + "group": ["a"], + "time_start": [np.datetime64("2020-01-01T12:30")], + "time_end": [np.datetime64("2020-01-01T12:35")], + } + ) + annotations = dc.AnnotationSet(frame, dims=("time",)) + loaded = dc.annotations(annotations.save(tmp_path / "picks")) + assert loaded == annotations + assert isinstance(loaded[0].region.bounds["time"][0], np.datetime64) + + def test_a_text_dimension_column_agrees_with_its_region(self): + """The frame and the geometry built from it say the same thing.""" + frame = pd.DataFrame({"time_start": ["2020-01-01"], "time_end": ["2020-01-02"]}) + out = dc.AnnotationSet(frame, dims=("time",)) + held = out.to_dataframe()["time_start"][0] + assert isinstance(held, pd.Timestamp | np.datetime64) + assert out[0].region.bounds["time"][0] == np.datetime64("2020-01-01") + def test_an_id_which_looks_like_a_number(self, tmp_path): """An id is the label its vertices name it by, never a number.""" frame = pd.DataFrame({"id": ["1"], "geometry": ["path"]}) @@ -441,6 +563,11 @@ def test_an_id_which_looks_like_a_number(self, tmp_path): annotations = dc.AnnotationSet(frame, dims=("distance",), vertices=vertices) loaded = dc.annotations(annotations.save(tmp_path / "picks")) assert loaded[0].id == "1" + # The frame too, not only the model: Annotation.id is typed str, so + # it would coerce an int back and hide the damage. + assert loaded.to_dataframe()["id"][0] == "1" + assert loaded.to_vertices()["id"][0] == "1" + assert loaded == annotations class TestWriting: @@ -488,6 +615,14 @@ def test_a_nested_extra_is_written_as_its_document(self, tmp_path): text = dc.AnnotationSet(frame, dims=("distance",)).to_csv() assert '{""n"": 1}' in text + def test_an_extra_json_cannot_spell(self, tmp_path): + """A nested value with no json type is written as its text rather + than dying as a circular reference. + """ + frame = pd.DataFrame({"group": ["a"], "distance": [1.0], "meta": [{"s": {1}}]}) + text = dc.AnnotationSet(frame, dims=("distance",)).to_csv() + assert '{""s"": ""1""}' in text + def test_the_attrs_name_their_model(self, regions, tmp_path): """The document says what it holds, as every stored object does.""" directory = regions.save(tmp_path / "picks") diff --git a/tests/test_core/test_annotations.py b/tests/test_core/test_annotations.py index 9b30551b2..9133b8d71 100644 --- a/tests/test_core/test_annotations.py +++ b/tests/test_core/test_annotations.py @@ -283,14 +283,24 @@ def test_category_needs_no_categories(self): == 2 ) - def test_datetime_unit_still_distinguished(self): - """Comparing by name keeps a nanosecond column from passing as microsecond.""" - frame = pd.DataFrame({"when": TIMES[:1]}) - with pytest.raises(ParameterError, match="states dtype"): + def test_datetime_unit_is_always_nanoseconds(self): + """A set holds times at nanoseconds, so another unit names no column + it could hold, and the error says so rather than blaming the data. + """ + frame = pd.DataFrame({"when": np.array(["2020-01-01"], dtype="datetime64[us]")}) + with pytest.raises(ParameterError, match="every time at nanoseconds"): AnnotationSet( frame, dims=DIMS, columns={"when": {"dtype": "datetime64[us]"}} ) + def test_a_declared_nanosecond_column(self): + """The unit a set does hold is the one which may be declared.""" + frame = pd.DataFrame({"when": np.array(["2020-01-01"], dtype="datetime64[us]")}) + out = AnnotationSet( + frame, dims=DIMS, columns={"when": {"dtype": "datetime64[ns]"}} + ) + assert out.to_dataframe()["when"].dtype == np.dtype("datetime64[ns]") + def test_unreadable_dtype_refused(self): """A dtype naming nothing says so, rather than raising numpy's error.""" with pytest.raises(ParameterError, match="declares the dtype"): @@ -380,6 +390,16 @@ def test_absent(self): """No tags is an empty tuple, not None.""" assert AnnotationSet(pd.DataFrame({"group": ["a"]}), dims=DIMS)[0].tags == () + def test_a_padded_tag_is_held_stripped(self): + """Tags are held as they read back, so padding does not survive.""" + out = AnnotationSet(pd.DataFrame({"tags": [(" a", "b ")]}), dims=DIMS) + assert out.to_dataframe()["tags"][0] == ("a", "b") + + def test_an_empty_tag_is_no_tag(self): + """A tag holding nothing cannot be written down, so it is not held.""" + out = AnnotationSet(pd.DataFrame({"tags": [("", "b")]}), dims=DIMS) + assert out[0].tags == ("b",) + def test_a_tag_holding_a_comma(self): """A comma separates tags, so a tag holding one would become two.""" frame = pd.DataFrame({"tags": [("a,b", "c")]}) @@ -1141,9 +1161,30 @@ def test_a_label_is_not_a_time(self): @pytest.mark.parametrize("model", [Region, Line]) def test_coordinates_which_are_not_a_mapping(self, model): """A coordinate map which is not a map is pydantic's to refuse.""" - with pytest.raises(ValidationError): + with pytest.raises(ValidationError, match=r"valid dictionary|Extra inputs"): model(bounds="everywhere", start="here", end="there") + @pytest.mark.parametrize( + "spelling", + ["2020-01-01", "2020-01-01T12", "2020-01-01T12:30", "2020-01-01T12:30:45"], + ) + def test_every_resolution_reads_back_as_a_time(self, spelling): + """Numpy writes only the fields a unit carries, and all of them read + back: an hour- or minute-resolution pick is an ordinary one. + """ + time = np.datetime64(spelling) + region = Region(bounds={"time": (time, time)}) + assert Region(**region.model_dump(mode="json")) == region + assert isinstance(region.bounds["time"][0], np.datetime64) + + @pytest.mark.parametrize("label", ["2020", "2020-01", "spring", "12:30"]) + def test_a_partial_date_is_not_a_time(self, label): + """A label which is not a whole date stays the label it was; nothing + distinguishes a bare year from a string spelled like one. + """ + region = Region(bounds={"stage": (label, label)}) + assert region.bounds["stage"] == (label, label) + def test_geometry_kinds_are_distinct(self): """A polygon is not a path which happens to close.""" assert not isinstance( From 2e32592ef82dcbf2b729c6ac875040439074ec79 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sun, 16 Aug 2026 21:11:49 +0200 Subject: [PATCH 4/5] Assert one attrs file rather than its spelling on disk A case-insensitive filesystem holds a shouted attrs.JSON and the written attrs.json in the same file, so the name it keeps is the platform's to decide. What this format states is that a directory holds one. --- tests/test_core/test_annotation_loader.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/test_core/test_annotation_loader.py b/tests/test_core/test_annotation_loader.py index 086094625..584cd30eb 100644 --- a/tests/test_core/test_annotation_loader.py +++ b/tests/test_core/test_annotation_loader.py @@ -281,9 +281,14 @@ def test_a_shouted_spelling_is_read_and_superseded(self, regions, tmp_path): loaded = dc.annotations(directory) assert loaded == regions loaded.save(directory) - assert {x.name for x in directory.iterdir() if x.stem == "attrs"} == { - "attrs.json" - } + # One attrs file, whatever it ends up called: a case-insensitive + # filesystem holds the shouted name and the written one in the + # same file, so the spelling on disk is the platform's to decide + # and only the count is this format's. + assert ( + len([x for x in directory.iterdir() if x.stem.casefold() == "attrs"]) == 1 + ) + assert dc.annotations(directory) == regions class TestTheDoor: From 8e674bb1204310a4d6eb7c4791ec9607afbca6c2 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sun, 16 Aug 2026 21:27:43 +0200 Subject: [PATCH 5/5] Answer the review of the store's write path - `save` spelled everything out before touching the directory. It cleared the superseded parts and replaced the attributes first, so a table refusing to be written -- which an ambiguous value now makes it do -- left the stale files gone, the attributes new and the annotations old: the half-stated directory the clearing exists to prevent. - A directory which states its own dimensions refuses others. Reading its cells against different dimensions types them differently and builds a set which is not the one stored, so the door now treats stored dims as it treats a stored attrs or vertices. - Only the suffix of an object file is matched without regard to case, never the stem; the comment claimed both. --- dascore/core/annotation_loader.py | 13 +++++-- dascore/core/annotations.py | 41 +++++++++++++++-------- tests/test_core/test_annotation_loader.py | 30 ++++++++++++++--- 3 files changed, 63 insertions(+), 21 deletions(-) diff --git a/dascore/core/annotation_loader.py b/dascore/core/annotation_loader.py index 999dc3a6b..a7dd255fd 100644 --- a/dascore/core/annotation_loader.py +++ b/dascore/core/annotation_loader.py @@ -69,9 +69,11 @@ def _read_object(path: Path) -> dict[str, Any]: except (OSError, UnicodeDecodeError) as error: msg = f"Could not read {quote_path(path)}: {error}." raise ParameterError(msg) from error - # Casefolded, as the inventory matches its object files: a shouted - # ATTRS.JSON is the file attrs.json would be, and reading it as YAML - # for its spelling would fail on a document which is not wrong. + # The suffix is casefolded, as the inventory casefolds its own: an + # attrs.JSON is the file attrs.json would be, and reading it as YAML + # for its spelling would fail on a document which is not wrong. The + # stem is not: the part names are exact, as every other name a format + # reserves is. if path.suffix.casefold() == OBJECT_SUFFIXES[0]: try: data = json.loads(text) @@ -304,6 +306,11 @@ def _load_directory(directory: Path, dims, **kwargs) -> AnnotationSet: vertices=kwargs.pop("vertices", None), ) attrs = _read_attrs(directory) + # Dimensions too, once the directory has stated them: reading the + # cells against other dimensions would type them differently and + # build a set which is not the one stored here. + if attrs.get("dims"): + _refuse_overrides("a directory stating its own dimensions", dims=dims) _refuse_stray_tables(directory) table = directory / f"{ANNOTATION_STEM}{TABLE_SUFFIX}" if not table.exists(): diff --git a/dascore/core/annotations.py b/dascore/core/annotations.py index cd8c4af3f..e040a3dc6 100644 --- a/dascore/core/annotations.py +++ b/dascore/core/annotations.py @@ -756,14 +756,24 @@ def save(self, path) -> pathlib.Path: ------- The directory written to, so a save reads straight back. """ + # Everything is spelled out before the directory is touched: a + # table which refuses to be written -- an ambiguous value does -- + # would otherwise raise with the stale parts already deleted and + # the attributes already replaced, leaving half a set behind. + # Defaults are dropped from the document, so it says what the set + # says; dims has no default, so it is always written, and the + # attributes name their own model, which is what the file holds. + document = self._attrs.model_dump(mode="json", exclude_defaults=True) + annotation_text = _write_table(self._df) + vertex_text = None if self._vertices.empty else _write_table(self._vertices) directory = pathlib.Path(path) directory.mkdir(parents=True, exist_ok=True) vertex_table = directory / f"{VERTEX_STEM}{TABLE_SUFFIX}" attrs_file = directory / f"{ATTRS_STEM}{OBJECT_SUFFIXES[0]}" - # Only the spellings this format claims, and matched the way the - # loader matches them: a notes.txt or an attrs.bak beside them - # participates in no convention and is not this function's to - # delete, while a shouted attrs.YAML is one the loader would read. + # Only the spellings this format claims: a notes.txt or an + # attrs.bak beside them participates in no convention and is not + # this function's to delete. The suffix is matched without regard + # to case, as the loader matches it. superseded = [ x for x in directory.iterdir() @@ -771,19 +781,15 @@ def save(self, path) -> pathlib.Path: and x.suffix.casefold() in OBJECT_SUFFIXES and x != attrs_file ] - if self._vertices.empty: + if vertex_text is None: superseded.append(vertex_table) for stale in superseded: stale.unlink(missing_ok=True) - # Defaults are dropped, so the document says what the set says; - # dims has no default, so it is always written. The attributes name - # their own model, which is what the file holds. - document = self._attrs.model_dump(mode="json", exclude_defaults=True) with open(attrs_file, "w") as stream: json.dump(document, stream, indent=2) - _write_table(self._df, directory / f"{ANNOTATION_STEM}{TABLE_SUFFIX}") - if not self._vertices.empty: - _write_table(self._vertices, vertex_table) + _write_text(annotation_text, directory / f"{ANNOTATION_STEM}{TABLE_SUFFIX}") + if vertex_text is not None: + _write_text(vertex_text, vertex_table) return directory # --- what the set holds @@ -1479,11 +1485,18 @@ def _write_table(frame: pd.DataFrame, path=None) -> str: spelled = pd.DataFrame({name: _writable(frame[name]) for name in frame.columns}) text = spelled.to_csv(index=False) if path is not None: - with open(path, "w", newline="", encoding="utf-8") as stream: - stream.write(text) + _write_text(text, path) return text +def _write_text(text: str, path) -> None: + """Write table text exactly as it was spelled.""" + # newline="" so the line terminators pandas wrote are the ones which + # land, rather than each one growing a carriage return on Windows. + with open(path, "w", newline="", encoding="utf-8") as stream: + stream.write(text) + + def _writable(series: pd.Series) -> pd.Series: """Return one column as the text a table states it with.""" return series.map(_writable_cell) diff --git a/tests/test_core/test_annotation_loader.py b/tests/test_core/test_annotation_loader.py index 584cd30eb..86dc3ab50 100644 --- a/tests/test_core/test_annotation_loader.py +++ b/tests/test_core/test_annotation_loader.py @@ -274,8 +274,8 @@ def test_a_file_owing_this_format_nothing_is_left(self, regions, tmp_path): regions.save(directory) assert (directory / "attrs.bak").read_text() == "mine" - def test_a_shouted_spelling_is_read_and_superseded(self, regions, tmp_path): - """A file is matched by its name, not by its case.""" + def test_a_shouted_suffix_is_read_and_superseded(self, regions, tmp_path): + """One data model stands behind a suffix however it is spelled.""" directory = regions.save(tmp_path / "picks") (directory / "attrs.json").rename(directory / "attrs.JSON") loaded = dc.annotations(directory) @@ -393,9 +393,18 @@ def test_a_document_which_does_not_build(self, tmp_path): with pytest.raises(InvalidAnnotationError, match="Extra inputs"): dc.annotations(directory) - def test_the_caller_wins(self, regions, tmp_path): - """A caller stating dimensions states them for the whole read.""" + def test_a_directory_which_states_them_refuses_others(self, regions, tmp_path): + """Reading the cells against other dimensions would type them + differently and build a set which is not the one stored. + """ + directory = regions.save(tmp_path / "picks") + with pytest.raises(InvalidAnnotationError, match="its own dimensions"): + dc.annotations(directory, dims=("time", "distance")) + + def test_a_directory_which_states_none_takes_them(self, regions, tmp_path): + """Where a directory states none, the caller's are the only ones.""" directory = regions.save(tmp_path / "picks") + (directory / "attrs.json").unlink() assert dc.annotations(directory, dims=("time", "distance")).dims == ( "time", "distance", @@ -551,6 +560,19 @@ def test_a_minute_resolution_time(self, tmp_path): assert loaded == annotations assert isinstance(loaded[0].region.bounds["time"][0], np.datetime64) + def test_a_dimension_some_rows_leave_blank(self): + """Times as text beside empty cells read as times and as unset.""" + frame = pd.DataFrame( + { + "group": ["a", "b"], + "time_start": ["2020-01-01", None], + "time_end": ["2020-01-02", None], + } + ) + out = dc.AnnotationSet(frame, dims=("time",)) + assert out[0].region.bounds["time"][0] == np.datetime64("2020-01-01") + assert "time" not in out[1].region.bounds + def test_a_text_dimension_column_agrees_with_its_region(self): """The frame and the geometry built from it say the same thing.""" frame = pd.DataFrame({"time_start": ["2020-01-01"], "time_end": ["2020-01-02"]})