From 2426bbcff6319a663875ee58fabfcb42d8c781d0 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Fri, 24 Jul 2026 19:01:01 +0200 Subject: [PATCH 01/12] Fix DASVader anonymous reference handling (#774) --- dascore/io/dasvader/core.py | 5 ++-- dascore/io/dasvader/utils.py | 8 ++--- tests/test_io/test_dasvader/test_dasvader.py | 31 +++++++++++++++++++- 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/dascore/io/dasvader/core.py b/dascore/io/dasvader/core.py index 5d5f72782..7114f22b3 100644 --- a/dascore/io/dasvader/core.py +++ b/dascore/io/dasvader/core.py @@ -24,8 +24,9 @@ class DASVaderV1(FiberIO): Notes ----- Legacy DASVader files may contain anonymous JLD2 object references. DASCore - detects those files and raises `DASVaderCompatibilityError` with compatibility - instructions instead of failing inside `h5py`. A known working stack for + reads these references when supported by HDF5 and raises + `DASVaderCompatibilityError` with compatibility instructions when + dereferencing fails. A known working stack for such legacy files is `h5py<3.16` with `HDF5 1.14.x`. """ diff --git a/dascore/io/dasvader/utils.py b/dascore/io/dasvader/utils.py index 174d221bb..f808a7d05 100644 --- a/dascore/io/dasvader/utils.py +++ b/dascore/io/dasvader/utils.py @@ -4,7 +4,6 @@ import h5py import numpy as np -from h5py import h5r from h5py.h5r import Reference import dascore as dc @@ -77,12 +76,13 @@ def _raise_legacy_ref_error(h5, field_name: str) -> None: def _dereference(h5, value, field_name: str): - """Resolve an HDF5 reference, rejecting legacy anonymous DASVader refs.""" + """Resolve an HDF5 reference or raise a clear compatibility error.""" if not isinstance(value, Reference): return value - if h5r.get_name(value, h5.id) is None: + try: + return h5[value] + except KeyError: _raise_legacy_ref_error(h5, field_name) - return h5[value] # --- Metadata parsing diff --git a/tests/test_io/test_dasvader/test_dasvader.py b/tests/test_io/test_dasvader/test_dasvader.py index 02b9ba1d2..99561b02b 100644 --- a/tests/test_io/test_dasvader/test_dasvader.py +++ b/tests/test_io/test_dasvader/test_dasvader.py @@ -13,7 +13,7 @@ from h5py.h5r import Reference import dascore as dc -from dascore.exceptions import DependencyError +from dascore.exceptions import DASVaderCompatibilityError, DependencyError from dascore.io.dasvader.utils import _dereference, _julia_ms_to_datetime64 from dascore.utils.downloader import fetch @@ -239,3 +239,32 @@ def test_dereference_returns_non_reference(self): """Non-reference values should be returned unchanged.""" value = np.float64(5_000.0) assert _dereference(None, value, "PulseRateFreq") == value + + def test_dereference_anonymous_reference(self, tmp_path): + """Anonymous references should be read when supported by HDF5.""" + path = tmp_path / "anonymous_reference.h5" + with h5py.File(path, "w") as resource: + target = resource.create_dataset("target", data=np.array([1])) + resource.create_dataset("reference", data=target.ref, dtype=h5py.ref_dtype) + del resource["target"] + reference = resource["reference"][()] + + assert h5py.h5r.get_name(reference, resource.id) is None + resolved = _dereference(resource, reference, "htime") + + assert resolved[0] == 1 + + def test_dereference_failure(self): + """Failed references should raise a clear compatibility error.""" + + class BrokenResource: + """Minimal HDF5 resource whose references cannot be resolved.""" + + filename = "legacy.jld2" + + def __getitem__(self, value): + raise KeyError(value) + + match = r"legacy\.jld2.*'htime'.*h5py<3\.16" + with pytest.raises(DASVaderCompatibilityError, match=match): + _dereference(BrokenResource(), Reference(), "htime") From 38f43ed8382b4a9f8ae2556d3912861bcf616707 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sat, 25 Jul 2026 10:01:51 +0200 Subject: [PATCH 02/12] Fix rank-0 coord select guard and fbe doc typo (#771) --- dascore/core/coords.py | 2 +- dascore/transform/fbe.py | 4 ++-- tests/test_core/test_coords.py | 7 +++++++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/dascore/core/coords.py b/dascore/core/coords.py index cf1a2636f..e4c23e655 100644 --- a/dascore/core/coords.py +++ b/dascore/core/coords.py @@ -347,7 +347,7 @@ def _select_by_sample_array(self, array): msg = "Using an array input for select with samples requires integer dtype." raise CoordError(msg) # Filter out bad indices - if self.ndim > 1: + if self.ndim != 1: msg = "Select only works on 1D coords." raise CoordError(msg) inds = np.arange(len(self)) diff --git a/dascore/transform/fbe.py b/dascore/transform/fbe.py index 401bfbd34..48ffaf992 100644 --- a/dascore/transform/fbe.py +++ b/dascore/transform/fbe.py @@ -41,8 +41,8 @@ def fbe( can be used for downsampling of the resulting patch. See also [rolling](`dascore.Patch.rolling`) db - Return patch data in decibel [dB] instead of orginal units. - Decibel is calculated as 20 * log10( sqrt(mean(x^2))) ). + Return patch data in decibel [dB] instead of original units. + Decibel is calculated as 20 * log10( sqrt(mean(x^2)) ). **kwargs Used to specify the dimension and asociated frequency, wavelength, or equivalent limits. For example time=(1, 100) applies a time-dimension bandpass diff --git a/tests/test_core/test_coords.py b/tests/test_core/test_coords.py index 9031795c6..7e4a6e956 100644 --- a/tests/test_core/test_coords.py +++ b/tests/test_core/test_coords.py @@ -2104,6 +2104,13 @@ def test_select_sample_array_2d_raises(self, coord_2d): with pytest.raises(CoordError, match="1D coords"): coord_2d.select(np.array([0, 1]), samples=True) + def test_select_sample_array_0d_raises(self): + """A rank-0 coord must also be rejected, not just >1D.""" + coord_0d = CoordPartial(shape=()) + assert coord_0d.ndim == 0 + with pytest.raises(CoordError, match="1D coords"): + coord_0d.select(np.array([0, 1]), samples=True) + def test_get_sample_count_2d_raises(self, coord_2d): """get_sample_count requires a 1D coord.""" with pytest.raises(CoordError, match="1D coords"): From 357968b928e61f25f1a8d22db5b216ec70db72d7 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sat, 25 Jul 2026 10:39:20 +0200 Subject: [PATCH 03/12] Streamline exact rolling output construction (#775) --- benchmarks/test_patch_benchmarks.py | 5 +++ dascore/proc/rolling.py | 65 +++++++++++++++++++---------- tests/test_proc/test_rolling.py | 64 ++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 22 deletions(-) diff --git a/benchmarks/test_patch_benchmarks.py b/benchmarks/test_patch_benchmarks.py index 748d10f73..1f47d31d7 100644 --- a/benchmarks/test_patch_benchmarks.py +++ b/benchmarks/test_patch_benchmarks.py @@ -353,6 +353,11 @@ def test_rolling_large_roller_mean(self, big_roller): """Time rolling mean calculation.""" big_roller.mean() + @pytest.mark.benchmark + def test_rolling_mean_full_call(self, example_patch): + """Time a complete rolling mean, including roller construction.""" + example_patch.rolling(time=5, samples=True, center=True).mean() + class TestAlignBenchmarks: """Benchmarks for align_to_coord operation.""" diff --git a/dascore/proc/rolling.py b/dascore/proc/rolling.py index b6b11feb7..5a6503ea2 100644 --- a/dascore/proc/rolling.py +++ b/dascore/proc/rolling.py @@ -2,6 +2,7 @@ from __future__ import annotations +from dataclasses import dataclass from typing import Any, Literal import numpy as np @@ -11,7 +12,6 @@ from dascore.constants import samples_arg_description from dascore.exceptions import ParameterError from dascore.utils.docs import compose_docstring -from dascore.utils.models import DascoreBaseModel from dascore.utils.patch import get_dim_axis_value, get_window_axis_step from dascore.utils.pd import rolling_df @@ -37,11 +37,14 @@ """ -class _PatchRollerInfo(DascoreBaseModel): +@dataclass(frozen=True, slots=True) +class _PatchRollerInfo: """ A dataclass for storing info on rolling operation. - Should be subclassed to implement rolling methods. + Should be subclassed to implement rolling methods. This is an ephemeral + internal object created on every rolling call, so it is a plain dataclass + rather than a validated model. """ patch: Any # cant set to patch due to circular import @@ -59,9 +62,10 @@ def get_coords(self): Accounts for centered or non-centered coordinates. If the window length is even, the first half value is used. """ - coord = self.patch.get_coord(self.dim) - if self.step > 1: - coord = coord[:: self.step] + # Without a step the dimension is unchanged; reuse the coord manager. + if self.step == 1: + return self.patch.coords + coord = self.patch.get_coord(self.dim)[:: self.step] return self.patch.coords.update(**{self.dim: coord}) def _get_attrs_with_apply_history(self, func_or_str): @@ -76,6 +80,16 @@ def _get_attrs_with_apply_history(self, func_or_str): attrs = self.patch.attrs.update(history=new_history, coords={}) return attrs + def _new_patch(self, data, attrs): + """ + Create the output patch from rolled data. + + The coordinates and attrs are both built here, so the Patch is + created directly rather than through `Patch.update`, which would + reconcile the attrs against the coords a second time. + """ + return self.patch.__class__(data=data, coords=self.get_coords(), attrs=attrs) + class _NumpyPatchRoller(_PatchRollerInfo): """A class to apply roller operations to patches.""" @@ -91,17 +105,26 @@ def get_start_index(self): return int(out) def _pad_roll_array(self, data): - """Pad.""" + """ + Pad the reduced array with NaNs and align it to the output coordinate. + + The NaNs go at the start of the axis, except when centering, which + moves `num_nans // 2` of them to the end. This is done with a single + allocation rather than a pad followed by a roll. + """ num_nans = 1 + (self.window - 2) // self.step - pad_width = [(0, 0)] * len(data.shape) - pad_width[self.axis] = (num_nans, 0) - padded = np.pad(data, pad_width, constant_values=np.nan) + if not num_nans: # window of one sample; nothing to pad. + return data + shape = list(data.shape) + shape[self.axis] += num_nans + out = np.full(shape, np.nan, dtype=data.dtype) + start = num_nans - num_nans // 2 if self.center else num_nans + slicer = [slice(None, None)] * len(shape) + slicer[self.axis] = slice(start, start + data.shape[self.axis]) + out[tuple(slicer)] = data if self.step == 1: - assert padded.shape == self.patch.data.shape - if self.center: - # roll array along axis to center - padded = np.roll(padded, -(num_nans // 2), axis=self.axis) - return padded + assert out.shape == self.patch.data.shape + return out @compose_docstring(apply_description=rolling_apply_description) def apply(self, function, *args, **kwargs): @@ -127,11 +150,10 @@ def apply(self, function, *args, **kwargs): step_slice[self.axis] = slice(start, None, self.step) # apply function, then pad with NaNs and roll trimmed_slide_view = slide_view[tuple(step_slice)] - raw = function(trimmed_slide_view, *args, axis=-1, **kwargs).astype(np.float64) - out = self._pad_roll_array(raw) - new_coords = self.get_coords() + raw = function(trimmed_slide_view, *args, axis=-1, **kwargs) + out = self._pad_roll_array(np.asarray(raw, dtype=np.float64)) attrs = self._get_attrs_with_apply_history(function) - return self.patch.update(data=out, coords=new_coords, attrs=attrs) + return self._new_patch(out, attrs) def mean(self): """Apply mean to moving window.""" @@ -181,14 +203,13 @@ def _get_rolling(self): ) return roll - def _repack_patch(self, df, attrs=None): + def _repack_patch(self, df, attrs): """Repack patch into dataframe.""" data = df.values if not self.axis else df.T.values # get rid of extra dims if original data doesn't have them. if len(data.shape) != len(self.patch.data.shape): data = np.squeeze(data) - coords = self.get_coords() - return self.patch.update(data=data, coords=coords, attrs=attrs) + return self._new_patch(data, attrs) def _call_rolling_func(self, name, *args, **kwargs): """Helper function for calling a rolling function.""" diff --git a/tests/test_proc/test_rolling.py b/tests/test_proc/test_rolling.py index b55b7bbea..3c72f4779 100644 --- a/tests/test_proc/test_rolling.py +++ b/tests/test_proc/test_rolling.py @@ -243,6 +243,70 @@ def percentile_plus(frame, q, offset=0, axis=None): assert all_close(out, expected) +class TestRollingMetadata: + """Ensure rolling output carries over the input patch's metadata.""" + + @pytest.fixture(scope="class") + def unit_patch(self, random_patch_with_lat_lon): + """A patch with units set on its data and all of its coords.""" + patch = random_patch_with_lat_lon + return patch.set_units("strain", distance="m", time="s", latitude="deg") + + @pytest.mark.parametrize("engine", ("numpy", "pandas")) + def test_coords_unchanged_without_step(self, random_patch, engine): + """Without a step the rolling dim is unchanged, so are the coords.""" + out = random_patch.rolling(time=10, samples=True, engine=engine).mean() + assert out.coords == random_patch.coords + + @pytest.mark.parametrize("engine", ("numpy", "pandas")) + def test_units_preserved(self, unit_patch, engine): + """Data units and units of every coord should survive rolling.""" + out = unit_patch.rolling(time=10, samples=True, engine=engine).mean() + assert out.attrs.data_units == unit_patch.attrs.data_units + for name in unit_patch.coords.coord_map: + assert out.get_coord(name).units == unit_patch.get_coord(name).units + + @pytest.mark.parametrize("engine", ("numpy", "pandas")) + def test_attrs_preserved(self, random_patch, engine): + """Non-coordinate attrs should be unaffected by rolling.""" + patch = random_patch.update_attrs(tag="bob", station="wat") + out = patch.rolling(time=10, samples=True, engine=engine).mean() + assert out.attrs.tag == "bob" + assert out.attrs.station == "wat" + + # The numpy engine routes its reductions through apply, pandas does not. + @pytest.mark.parametrize( + "engine,suffix", (("numpy", ".apply(mean)"), ("pandas", ".mean()")) + ) + def test_history_appended(self, random_patch, engine, suffix): + """A single history entry naming the operation should be added.""" + out = random_patch.rolling(time=10, samples=True, engine=engine).mean() + history = list(out.attrs.history) + assert len(history) == len(random_patch.attrs.history) + 1 + expected = ( + f"rolling(time=10, step=1, overlap=None, " + f"center=False, engine={engine}){suffix}" + ) + assert history[-1] == expected + + @pytest.mark.parametrize("engine", ("numpy", "pandas")) + def test_non_dim_coords_preserved(self, random_patch_with_lat_lon, engine): + """Coords which don't change shape should be kept as they were.""" + patch = random_patch_with_lat_lon + out = patch.rolling(time=10, samples=True, engine=engine).mean() + assert set(out.coords.coord_map) == set(patch.coords.coord_map) + for name in set(patch.coords.coord_map) - {"time"}: + assert out.get_coord(name) == patch.get_coord(name) + assert np.array_equal(out.get_array(name), patch.get_array(name)) + + @pytest.mark.parametrize("engine", ("numpy", "pandas")) + def test_attrs_conform_to_coords(self, random_patch, engine): + """The attrs coord summaries should match the output coords.""" + out = random_patch.rolling(time=10, samples=True, step=3, engine=engine).mean() + assert out.attrs.coords == out.coords.to_summary_dict() + assert out.attrs.dim_tuple == out.dims + + class TestNumpyVsPandasRolling: """Ensure numpy rolling return the same results as pandas rolling.""" From 052cc294e7943ac81f5d8ba7f2fad6dcb9df1a8a Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sat, 25 Jul 2026 12:01:56 +0200 Subject: [PATCH 04/12] Add Patch.from_parts and stop reconciling attrs twice (#777) --- benchmarks/test_patch_benchmarks.py | 48 +++++++ dascore/core/attrs.py | 16 +++ dascore/core/patch.py | 84 ++++++++++++- dascore/proc/basic.py | 20 ++- dascore/proc/filter.py | 4 +- dascore/proc/rolling.py | 41 +++--- dascore/transform/differentiate.py | 3 +- dascore/transform/strain.py | 6 +- dascore/utils/array.py | 6 +- tests/test_core/test_patch.py | 189 +++++++++++++++++++++++++++- tests/test_proc/test_rolling.py | 24 +++- 11 files changed, 401 insertions(+), 40 deletions(-) diff --git a/benchmarks/test_patch_benchmarks.py b/benchmarks/test_patch_benchmarks.py index 1f47d31d7..6c01e9254 100644 --- a/benchmarks/test_patch_benchmarks.py +++ b/benchmarks/test_patch_benchmarks.py @@ -190,6 +190,54 @@ def test_slope_mute(self, example_patch): patch.slope_mute(slopes=(1000, 3000)) +class TestPatchConstructionBenchmarks: + """Benchmarks for the patch construction paths.""" + + @pytest.fixture(scope="module") + def new_data(self, example_patch): + """Data for constructing new patches; built outside the timed call.""" + return np.asarray(example_patch.data) * 2 + + @pytest.fixture(scope="module") + def decimated(self, example_patch): + """A coord manager with one dimension shortened.""" + coord = example_patch.get_coord("time")[::2] + return example_patch.coords.update(time=coord) + + @pytest.mark.benchmark + def test_new_data_only(self, example_patch, new_data): + """Time new when only data changes; coords and attrs are reused.""" + example_patch.new(data=new_data) + + @pytest.mark.benchmark + def test_new_with_coords(self, example_patch, decimated): + """Time new when the coords change, so attrs must be rebuilt.""" + example_patch.new(data=example_patch.data[:, ::2], coords=decimated) + + @pytest.mark.benchmark + def test_new_with_coords_and_attrs(self, example_patch, new_data): + """Time new when both coords and attrs are passed.""" + patch = example_patch + patch.new(data=new_data, coords=patch.coords, attrs=patch.attrs) + + @pytest.mark.benchmark + def test_from_parts(self, example_patch, new_data): + """Time the fast constructor for already conforming parts.""" + patch = example_patch + dc.Patch.from_parts(new_data, patch.coords, patch.attrs) + + @pytest.mark.benchmark + def test_patch_init(self, example_patch, new_data): + """ + Time the normal constructor. + + This is a control; it should not move, since the strict path is + deliberately left alone. + """ + patch = example_patch + dc.Patch(data=new_data, coords=patch.coords, dims=patch.dims, attrs=patch.attrs) + + class TestTransformBenchmarks: """Benchmarks for patch transform operations.""" diff --git a/dascore/core/attrs.py b/dascore/core/attrs.py index 1160ee405..815fdc421 100644 --- a/dascore/core/attrs.py +++ b/dascore/core/attrs.py @@ -254,6 +254,22 @@ def update(self, **kwargs) -> Self: out["coords"] = {} return self.__class__(**out) + def _conform_to(self, coords: dc.CoordManager) -> Self: + """ + Return attrs whose coord summaries and dims come from coords. + + Equivalent to `self.update(coords=coords)` but skips a full model + dump and re-validation. This uses model_copy, which does no type + coercion, so the values assigned here must already be in their + final form. + """ + return self.model_copy( + update={ + "coords": FrozenDict(coords.to_summary_dict()), + "dims": ",".join(coords.dims), + } + ) + def drop(self, *args): """Drop specific keys if they exist.""" contents = dict(self) diff --git a/dascore/core/patch.py b/dascore/core/patch.py index 470037903..360db1a8f 100644 --- a/dascore/core/patch.py +++ b/dascore/core/patch.py @@ -16,6 +16,7 @@ from dascore.core.attrs import PatchAttrs from dascore.core.coordmanager import CoordManager, get_coord_manager from dascore.core.coords import BaseCoord +from dascore.exceptions import PatchCoordinateError from dascore.utils.array import PatchUFunc, patch_array_function, patch_array_ufunc from dascore.utils.deprecate import deprecate from dascore.utils.display import array_to_text, attrs_to_text, get_dascore_text @@ -25,6 +26,16 @@ from dascore.utils.time import to_float +def _check_dims_match(coords: CoordManager, attrs: PatchAttrs) -> None: + """Raise if the coord manager and attrs disagree about dimensions.""" + if coords.dims != attrs.dim_tuple: + msg = ( + f"Dimension mismatch between coords ({coords.dims}) and " + f"attrs ({attrs.dim_tuple})." + ) + raise PatchCoordinateError(msg) + + class Patch(NamespaceOwner): """ A Class for managing data and metadata. @@ -60,6 +71,11 @@ class Patch(NamespaceOwner): - If coords and attrs are provided, attrs will have priority. This means if there is a conflict between information contained in both, the coords will be recalculated. + + - Because the constructor always reconciles the two, every Patch has attrs + whose coordinate summaries match its coords. Code which already holds a + conforming pair can skip that work with + [`Patch.from_parts`](`dascore.core.patch.Patch.from_parts`). """ data: ArrayLike @@ -98,11 +114,77 @@ def __init__( else: # ensure attrs conforms to coords attrs = dc.PatchAttrs.from_dict(attrs).update(coords=coords) - assert coords.dims == attrs.dim_tuple, "dim mismatch on coords and attrs" + _check_dims_match(coords, attrs) self._coords = coords self._attrs = attrs self._data = array(self.coords.validate_data(data)) + @classmethod + def from_parts( + cls, + data: ArrayLike, + coords: CoordManager, + attrs: PatchAttrs, + ) -> Self: + """ + Create a patch from parts which already conform to each other. + + This is a fast alternative to the normal constructor for code which + has already built a coordinate manager and matching attributes. It + skips recomputing the coordinate summaries stored on attrs, which + the constructor does unconditionally. + + Parameters + ---------- + data + The array data. Its shape must match coords. + coords + A CoordManager describing the data's dimensions. + attrs + A PatchAttrs whose coordinate summaries were derived from coords. + + Raises + ------ + PatchCoordinateError + If coords and attrs disagree about dimension names. + CoordDataError + If the data shape does not match coords. + + Notes + ----- + The data shape and dimension names are validated, but the coordinate + summaries on attrs are trusted. Passing attrs whose summaries came + from a different coordinate manager creates a Patch whose attrs + disagree with its coords. + + Every Patch conforms by construction, so `patch.attrs` is always safe + to pass alongside `patch.coords`. If the coords have changed, rebuild + the attrs first with `patch.attrs.update(coords=new_coords)`. Note + that attrs built with an empty `coords` (a common way to let the + constructor repopulate them) do *not* conform, and the dimension + check will not catch it, since clearing coords leaves dims in place. + + Examples + -------- + >>> import dascore as dc + >>> patch = dc.get_example_patch() + >>> + >>> # Data of the same shape reuses the existing coords and attrs. + >>> out = dc.Patch.from_parts(patch.data * 2, patch.coords, patch.attrs) + >>> + >>> # If the coords change, the attrs must be rebuilt from them. + >>> coords = patch.coords.update(time=patch.get_coord("time")[::2]) + >>> out = dc.Patch.from_parts( + ... patch.data[..., ::2], coords, patch.attrs.update(coords=coords) + ... ) + """ + _check_dims_match(coords, attrs) + new = cls.__new__(cls) + new._coords = coords + new._attrs = attrs + new._data = array(coords.validate_data(data)) + return new + _namespace_entry_point_group = "dascore.patch_namespace" def __eq__(self, other): diff --git a/dascore/proc/basic.py b/dascore/proc/basic.py index f5e540195..82d1d6753 100644 --- a/dascore/proc/basic.py +++ b/dascore/proc/basic.py @@ -10,7 +10,7 @@ from scipy.fft import next_fast_len import dascore as dc -from dascore.compat import array +from dascore.compat import DataArray, array from dascore.constants import PatchType from dascore.core.attrs import PatchAttrs from dascore.core.coordmanager import CoordManager, get_coord_manager @@ -239,6 +239,16 @@ def update( ----- - If both coords and attrs are defined, attrs will have priority. """ + # A Patch or DataArray passed as data brings its own coords and attrs, + # which the constructor unpacks, so it can't use the fast paths here. + data_has_meta = isinstance(data, DataArray | dc.Patch) + # When nothing that can affect coords or attrs changed, the reconciling + # done when self was created still holds and needn't be repeated. + if not data_has_meta and attrs is None and dims is None: + # Identity, not equality; CoordManager equality compares arrays. + if coords is None or coords is self.coords: + new_data = self.data if data is None else data + return self.from_parts(new_data, self.coords, self.attrs) data = data if data is not None else self.data coords = coords if coords is not None else self.coords if dims is None: @@ -247,9 +257,11 @@ def update( if attrs is not None: coords, attrs = coords.update_from_attrs(attrs) else: - _attrs = dc.PatchAttrs.from_dict(attrs or self.attrs) - attrs = _attrs.update(coords=coords) - return self.__class__(data=data, coords=coords, attrs=attrs) + attrs = dc.PatchAttrs.from_dict(self.attrs)._conform_to(coords) + if data_has_meta: # let the constructor unpack the patch/DataArray. + return self.__class__(data=data, coords=coords, attrs=attrs) + # coords and attrs were just reconciled; the constructor would redo it. + return self.from_parts(data, coords, attrs) @patch_function() diff --git a/dascore/proc/filter.py b/dascore/proc/filter.py index 53957b1d4..51c9508e5 100644 --- a/dascore/proc/filter.py +++ b/dascore/proc/filter.py @@ -190,7 +190,7 @@ def sobel_filter( dim, mode, cval = _check_sobel_args(dim, mode, cval) axis = patch.get_axis(dim) out = ndimage.sobel(patch.data, axis=axis, mode=mode, cval=cval) - return dc.Patch(data=out, coords=patch.coords, attrs=patch.attrs, dims=patch.dims) + return dc.Patch.from_parts(out, patch.coords, patch.attrs) def _create_size_and_axes(patch, kwargs, samples): @@ -331,7 +331,7 @@ def notch_filter(patch: PatchType, q: float, **kwargs) -> PatchType: raise FilterValueError(msg) b, a = iirnotch(w0, Q=q, fs=sr) data = filtfilt(b, a, data, axis=axis) - return dc.Patch(data=data, coords=patch.coords, attrs=patch.attrs, dims=patch.dims) + return dc.Patch.from_parts(data, patch.coords, patch.attrs) @patch_function() diff --git a/dascore/proc/rolling.py b/dascore/proc/rolling.py index 5a6503ea2..30b9af6ee 100644 --- a/dascore/proc/rolling.py +++ b/dascore/proc/rolling.py @@ -68,27 +68,31 @@ def get_coords(self): coord = self.patch.get_coord(self.dim)[:: self.step] return self.patch.coords.update(**{self.dim: coord}) - def _get_attrs_with_apply_history(self, func_or_str): - """Get new attrs that has history from apply attached.""" - new_history = list(self.patch.attrs.history) + def _get_attrs(self, func_or_str, coords): + """Get attrs, with the apply history attached, conforming to coords.""" if callable(func_or_str): func_name = getattr(func_or_str, "__name__", "") hist_str = f"{self.roll_hist}.apply({func_name})" else: hist_str = f"{self.roll_hist}.{func_or_str}()" - new_history.append(hist_str) - attrs = self.patch.attrs.update(history=new_history, coords={}) - return attrs - - def _new_patch(self, data, attrs): + # Must be a tuple; model_copy below does no type coercion. + history = (*self.patch.attrs.history, hist_str) + if coords is self.patch.coords: + # The coords didn't change, so their summaries still hold. + return self.patch.attrs.model_copy(update={"history": history}) + return self.patch.attrs.update(history=history, coords=coords) + + def _new_patch(self, data, func_or_str): """ Create the output patch from rolled data. - The coordinates and attrs are both built here, so the Patch is - created directly rather than through `Patch.update`, which would - reconcile the attrs against the coords a second time. + The coords and matching attrs are both built here, so the patch is + made with `Patch.from_parts` rather than the normal constructor, + which would reconcile the two a second time. """ - return self.patch.__class__(data=data, coords=self.get_coords(), attrs=attrs) + coords = self.get_coords() + attrs = self._get_attrs(func_or_str, coords) + return self.patch.from_parts(data, coords, attrs) class _NumpyPatchRoller(_PatchRollerInfo): @@ -152,8 +156,7 @@ def apply(self, function, *args, **kwargs): trimmed_slide_view = slide_view[tuple(step_slice)] raw = function(trimmed_slide_view, *args, axis=-1, **kwargs) out = self._pad_roll_array(np.asarray(raw, dtype=np.float64)) - attrs = self._get_attrs_with_apply_history(function) - return self._new_patch(out, attrs) + return self._new_patch(out, function) def mean(self): """Apply mean to moving window.""" @@ -203,20 +206,19 @@ def _get_rolling(self): ) return roll - def _repack_patch(self, df, attrs): + def _repack_patch(self, df, func_or_str): """Repack patch into dataframe.""" data = df.values if not self.axis else df.T.values # get rid of extra dims if original data doesn't have them. if len(data.shape) != len(self.patch.data.shape): data = np.squeeze(data) - return self._new_patch(data, attrs) + return self._new_patch(data, func_or_str) def _call_rolling_func(self, name, *args, **kwargs): """Helper function for calling a rolling function.""" rolling = self._get_rolling() df = getattr(rolling, name)(*args, **kwargs) - attrs = self._get_attrs_with_apply_history(name) - return self._repack_patch(df, attrs=attrs) + return self._repack_patch(df, name) @compose_docstring(apply_description=rolling_apply_description) def apply(self, function, *args, **kwargs): @@ -224,8 +226,7 @@ def apply(self, function, *args, **kwargs): {apply_description} """ df = self._get_rolling().apply(function, args=args, kwargs=kwargs) - attrs = self._get_attrs_with_apply_history(function) - return self._repack_patch(df, attrs=attrs) + return self._repack_patch(df, function) def mean(self): """Apply mean.""" diff --git a/dascore/transform/differentiate.py b/dascore/transform/differentiate.py index e9954dbe0..c758e6a0f 100644 --- a/dascore/transform/differentiate.py +++ b/dascore/transform/differentiate.py @@ -145,4 +145,5 @@ def differentiate( # update units data_units = _get_data_units_from_dims(patch, dims, truediv) attrs = patch.attrs.update(data_units=data_units) - return patch.new(data=new_data, attrs=attrs) + # Coords are unchanged, so attrs still conform to them. + return patch.from_parts(new_data, patch.coords, attrs) diff --git a/dascore/transform/strain.py b/dascore/transform/strain.py index 45d090be7..e23bcb164 100644 --- a/dascore/transform/strain.py +++ b/dascore/transform/strain.py @@ -118,7 +118,8 @@ def velocity_to_strain_rate( new_attrs = patch.attrs.update( data_type="strain_rate", gauge_length=step * step_multiple ) - return patch.update(attrs=new_attrs) + # Coords are unchanged, so attrs still conform to them. + return patch.from_parts(patch.data, patch.coords, new_attrs) @patch_function( @@ -273,4 +274,5 @@ def radians_to_strain( # Build output patch new_attrs = patch.attrs.update(data_units=new_units) new_data = patch.data * const * d_factor - return patch.update(data=new_data, attrs=new_attrs) + # Coords are unchanged, so attrs still conform to them. + return patch.from_parts(new_data, patch.coords, new_attrs) diff --git a/dascore/utils/array.py b/dascore/utils/array.py index 02b1bd39c..c7fd41c30 100644 --- a/dascore/utils/array.py +++ b/dascore/utils/array.py @@ -310,10 +310,8 @@ def _apply_aggregator(patch, dim, func, dim_reduce="empty"): else: coords = patch.coords.update(**{dim: new_coord}) data = np.expand_dims(func(data, axis=axis), axis) - attrs = patch.attrs.model_dump(exclude={"coords", "dims"}, exclude_unset=True) - attrs["coords"] = coords.to_summary_dict() - attrs["dims"] = coords.dims - patch = patch.new(data=data, coords=coords, attrs=attrs) + # Attrs are rebuilt from the new coords, so they conform to them. + patch = patch.from_parts(data, coords, patch.attrs._conform_to(coords)) return patch diff --git a/tests/test_core/test_patch.py b/tests/test_core/test_patch.py index 6abf6d3ad..3c68d19eb 100644 --- a/tests/test_core/test_patch.py +++ b/tests/test_core/test_patch.py @@ -4,6 +4,7 @@ import operator import weakref +from typing import ClassVar import numpy as np import pandas as pd @@ -14,7 +15,12 @@ from dascore.compat import random_state from dascore.core import Patch from dascore.core.coords import BaseCoord, CoordRange -from dascore.exceptions import CoordError, ParameterError +from dascore.exceptions import ( + CoordDataError, + CoordError, + ParameterError, + PatchCoordinateError, +) from dascore.proc.basic import apply_operator from dascore.utils.misc import suppress_warnings @@ -91,9 +97,8 @@ def patch_complex_coords(self): out = dict(data=array, coords=coords, attrs=attrs, dims=dims) return Patch(**out) - @pytest.fixture(scope="class") def test_conflicting_attrs_coords_raises(self): - """Patch for testing conflicting coordinates/attributes.""" + """Conflicting coordinate info in attrs and coords should raise.""" array = random_state.random((10, 10)) # create attrs, these should all get overwritten by coords. attrs = dict( @@ -112,7 +117,7 @@ def test_conflicting_attrs_coords_raises(self): # assemble and output. dims = ("distance", "time") out = dict(data=array, coords=coords, attrs=attrs, dims=dims) - msg = "Coords and attrs are incompatible." + msg = "At most one parameter can be specified in update_limits" with pytest.raises(ValueError, match=msg): dc.Patch(**out) @@ -346,6 +351,182 @@ def test_new_dims_renames_dims(self, random_patch): out = random_patch.new(dims=dims) assert out.dims == dims + def test_new_data_only_validates_shape(self, random_patch): + """New should still reject data which doesn't match the coords.""" + data = random_patch.data[:-1] + with pytest.raises(CoordDataError): + random_patch.new(data=data) + + def test_new_coords_validates_shape(self, random_patch): + """New should reject data which doesn't match passed coords.""" + coords = random_patch.coords + with pytest.raises(CoordDataError): + random_patch.new(data=random_patch.data[:, :-1], coords=coords) + + def test_new_same_coords_matches_default(self, random_patch): + """Passing the patch's own coords should match passing nothing.""" + data = random_patch.data * 2 + out1 = random_patch.new(data=data) + out2 = random_patch.new(data=data, coords=random_patch.coords) + assert out1.equals(out2, only_required_attrs=False) + + def test_new_no_args_equals_input(self, random_patch): + """New with no arguments should reproduce the patch.""" + assert random_patch.new().equals(random_patch, only_required_attrs=False) + + def test_new_from_patch_takes_over_metadata(self, random_patch): + """Passing a patch as data should adopt its coords and attrs.""" + other = random_patch.decimate(time=2) + out = random_patch.new(data=other) + assert out.coords == other.coords + assert out.shape == other.shape + + +class TestFromParts: + """Tests for the `Patch.from_parts` fast constructor.""" + + def test_round_trip(self, patch): + """Rebuilding a patch from its own parts should reproduce it.""" + out = dc.Patch.from_parts(patch.data, patch.coords, patch.attrs) + assert out.equals(patch, only_required_attrs=False) + + def test_matches_normal_constructor(self, patch): + """from_parts should match what the constructor produces.""" + data = patch.data * 2 + fast = dc.Patch.from_parts(data, patch.coords, patch.attrs) + slow = dc.Patch( + data=data, coords=patch.coords, dims=patch.dims, attrs=patch.attrs + ) + assert fast.equals(slow, only_required_attrs=False) + assert fast.coords == slow.coords + assert fast.attrs == slow.attrs + + def test_data_not_writeable(self, random_patch): + """Data should be read-only, just as with the normal constructor.""" + out = dc.Patch.from_parts( + np.array(random_patch.data), random_patch.coords, random_patch.attrs + ) + assert not out.data.flags.writeable + + def test_changed_coords_need_rebuilt_attrs(self, random_patch): + """The documented recipe for changed coords should conform.""" + patch = random_patch + coords = patch.coords.update(time=patch.get_coord("time")[::2]) + attrs = patch.attrs.update(coords=coords) + out = dc.Patch.from_parts(patch.data[:, ::2], coords, attrs) + assert dict(out.attrs.coords) == out.coords.to_summary_dict() + + def test_bad_shape_raises(self, random_patch): + """Data which doesn't match the coords should raise.""" + with pytest.raises(CoordDataError): + dc.Patch.from_parts( + random_patch.data[:-1], random_patch.coords, random_patch.attrs + ) + + def test_dim_mismatch_raises(self, random_patch): + """Attrs which disagree with coords about dims should raise.""" + attrs = random_patch.attrs.update(dims="jazz,hands") + with pytest.raises(PatchCoordinateError, match="Dimension mismatch"): + dc.Patch.from_parts(random_patch.data, random_patch.coords, attrs) + + def test_subclass_preserved(self, random_patch): + """from_parts should honor the class it is called on.""" + + class SubPatch(dc.Patch): + """A patch subclass.""" + + out = SubPatch.from_parts( + random_patch.data, random_patch.coords, random_patch.attrs + ) + assert isinstance(out, SubPatch) + + +class TestAttrsConformTo: + """Tests for the cheap `PatchAttrs._conform_to` rebuild.""" + + @pytest.fixture(scope="class") + def coord_managers(self, random_patch_with_lat_lon): + """Coord managers covering the interesting kinds of change.""" + patch = random_patch_with_lat_lon + coords = patch.coords + return { + "same": coords, + "decimated": coords.update(time=patch.get_coord("time")[::2]), + "dropped": coords.drop_coords("latitude")[0], + "transposed": coords.transpose(), + } + + def test_matches_update(self, random_patch_with_lat_lon, coord_managers): + """_conform_to should match the slower attrs.update(coords=...).""" + attrs = random_patch_with_lat_lon.attrs + for name, cm in coord_managers.items(): + fast, slow = attrs._conform_to(cm), attrs.update(coords=cm) + assert fast == slow, f"mismatch for {name}" + assert fast.model_dump() == slow.model_dump(), f"dump differs for {name}" + + def test_result_conforms(self, random_patch_with_lat_lon, coord_managers): + """The rebuilt attrs should agree with the coords they came from.""" + attrs = random_patch_with_lat_lon.attrs + for name, cm in coord_managers.items(): + out = attrs._conform_to(cm) + assert dict(out.coords) == cm.to_summary_dict(), name + assert out.dim_tuple == cm.dims, name + + def test_model_copy_does_not_coerce(self, random_patch): + """ + Document why _conform_to must pass final types. + + model_copy skips validation, so a list where a tuple is expected + produces an object which is not equal to the validated one. + """ + attrs = random_patch.attrs + history = ["a", "b"] + coerced = attrs.model_copy(update={"history": tuple(history)}) + uncoerced = attrs.model_copy(update={"history": history}) + assert coerced == attrs.update(history=history) + assert uncoerced != attrs.update(history=history) + + +class TestAttrsCoordsInvariant: + """ + Every Patch should have attrs whose coord summaries match its coords. + + This is what makes `Patch.from_parts` safe; the constructor establishes + it unconditionally. If this breaks, the fast paths built on it are wrong. + """ + + ops: ClassVar = { + "abs": lambda p: p.abs(), + "add": lambda p: p + 1, + # Differing history makes _merge_models drop the coord summaries, + # so this is the binary-op case most likely to break the invariant. + "add_patch": lambda p: p + p.update_attrs(history=["boo"]), + "select": lambda p: p.select(time=(1, 20), samples=True), + "decimate": lambda p: p.decimate(time=2), + "transpose": lambda p: p.transpose(), + "squeeze": lambda p: p.select(distance=0, samples=True).squeeze(), + "pad": lambda p: p.pad(time=2), + "dropna": lambda p: p.dropna("time"), + "sobel": lambda p: p.sobel_filter("time"), + "differentiate": lambda p: p.differentiate("time"), + "integrate": lambda p: p.integrate("time"), + "aggregate": lambda p: p.mean("time"), + "aggregate_squeeze": lambda p: p.mean("time", dim_reduce="squeeze"), + "rolling": lambda p: p.rolling(time=5, samples=True).mean(), + "rolling_step": lambda p: p.rolling(time=5, samples=True, step=3).mean(), + "dft": lambda p: p.dft("time"), + "idft": lambda p: p.dft("time").idft(), + "set_units": lambda p: p.set_units("strain", time="s"), + "update_attrs": lambda p: p.update_attrs(tag="bob"), + } + + @pytest.mark.parametrize("name", list(ops)) + def test_invariant_holds(self, random_patch_with_lat_lon, name): + """Output attrs should always agree with output coords.""" + out = self.ops[name](random_patch_with_lat_lon) + assert dict(out.attrs.coords) == out.coords.to_summary_dict() + assert out.attrs.dim_tuple == out.coords.dims + class TestDisplay: """Tests for displaying patches.""" diff --git a/tests/test_proc/test_rolling.py b/tests/test_proc/test_rolling.py index 3c72f4779..1f24d03dc 100644 --- a/tests/test_proc/test_rolling.py +++ b/tests/test_proc/test_rolling.py @@ -299,13 +299,33 @@ def test_non_dim_coords_preserved(self, random_patch_with_lat_lon, engine): assert out.get_coord(name) == patch.get_coord(name) assert np.array_equal(out.get_array(name), patch.get_array(name)) + # step=1 reuses the patch's coords; step>1 rebuilds them. The attrs are + # built differently in each case, so both need covering. @pytest.mark.parametrize("engine", ("numpy", "pandas")) - def test_attrs_conform_to_coords(self, random_patch, engine): + @pytest.mark.parametrize("step", (1, 3)) + def test_attrs_conform_to_coords(self, random_patch, engine, step): """The attrs coord summaries should match the output coords.""" - out = random_patch.rolling(time=10, samples=True, step=3, engine=engine).mean() + out = random_patch.rolling( + time=10, samples=True, step=step, engine=engine + ).mean() assert out.attrs.coords == out.coords.to_summary_dict() assert out.attrs.dim_tuple == out.dims + @pytest.mark.parametrize("engine", ("numpy", "pandas")) + @pytest.mark.parametrize("step", (1, 3)) + def test_history_is_tuple(self, random_patch, engine, step): + """ + History must stay a tuple. + + The step==1 path uses model_copy, which does no type coercion, so a + list would silently produce attrs unequal to the validated form. + """ + out = random_patch.rolling( + time=10, samples=True, step=step, engine=engine + ).mean() + assert isinstance(out.attrs.history, tuple) + assert out.attrs == out.attrs.update(history=out.attrs.history) + class TestNumpyVsPandasRolling: """Ensure numpy rolling return the same results as pandas rolling.""" From 2d58caa6d6102b3f24cc97bfaefc2580dbbec697 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sun, 26 Jul 2026 19:59:35 +0200 Subject: [PATCH 05/12] Add benchmark for importing dascore (#789) --- benchmarks/readme.md | 2 ++ benchmarks/test_import_benchmarks.py | 53 ++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 benchmarks/test_import_benchmarks.py diff --git a/benchmarks/readme.md b/benchmarks/readme.md index df439b850..c27086abe 100644 --- a/benchmarks/readme.md +++ b/benchmarks/readme.md @@ -17,6 +17,7 @@ pytest benchmarks/ --codspeed pytest benchmarks/test_patch_benchmarks.py --codspeed pytest benchmarks/test_io_benchmarks.py --codspeed pytest benchmarks/test_spool_benchmarks.py --codspeed +pytest benchmarks/test_import_benchmarks.py --codspeed ``` ## Benchmark Structure @@ -26,6 +27,7 @@ Benchmarks are now organized as pytest tests in the `benchmarks/` directory: - `test_patch_benchmarks.py` - Core Patch processing, transform, and visualization benchmarks - `test_io_benchmarks.py` - File I/O operations benchmarks - `test_spool_benchmarks.py` - Spool chunking and selection benchmarks +- `test_import_benchmarks.py` - Import benchmarks (dascore's own modules; third party dependencies stay warm) Each benchmark uses the `@pytest.mark.benchmark` decorator to automatically measure performance. diff --git a/benchmarks/test_import_benchmarks.py b/benchmarks/test_import_benchmarks.py new file mode 100644 index 000000000..8047af4d3 --- /dev/null +++ b/benchmarks/test_import_benchmarks.py @@ -0,0 +1,53 @@ +"""Benchmarks for importing dascore using pytest-codspeed.""" + +from __future__ import annotations + +import importlib +import sys +import warnings + +import pint +import pytest + + +def _dascore_module_names(): + """Get the names of all currently imported dascore modules.""" + return [x for x in sys.modules if x == "dascore" or x.startswith("dascore.")] + + +class TestImportBenchmarks: + """ + Benchmarks for re-executing dascore's modules. + + Only dascore's own modules are removed from the module cache, so these + measure the cost of executing dascore's module bodies, not the one-time + cost of importing third party dependencies. A fresh interpreter (eg a + CLI call) also pays the latter, but it cannot be measured here; a + subprocess falls outside the region CodSpeed instruments. Instead, see + tests/test_imports.py for the guards which keep slow dependencies out of + the import chain entirely. + """ + + @pytest.fixture() + def restore_dascore_modules(self): + """Put the original dascore modules back after re-importing them.""" + # Import here so third party dependencies are warm before timing, + # even when this file is the only one collected. + importlib.import_module("dascore") + saved = {x: sys.modules[x] for x in _dascore_module_names()} + # Re-importing dascore makes (and installs) a new pint registry. + registry = pint.get_application_registry().get() + # dascore adds a warning filter on import; catch_warnings undoes that. + with warnings.catch_warnings(): + yield + for name in _dascore_module_names(): + del sys.modules[name] + sys.modules.update(saved) + pint.set_application_registry(registry) + + @pytest.mark.benchmark + def test_reimport_dascore(self, restore_dascore_modules): + """Time re-importing the top-level dascore module.""" + for name in _dascore_module_names(): + del sys.modules[name] + importlib.import_module("dascore") From 6cee97cf3977c05fcdab0845e50593df8f6c24c3 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sun, 26 Jul 2026 20:15:59 +0200 Subject: [PATCH 06/12] Validate the requested length in BaseCoord.change_length (#787) --- dascore/core/coords.py | 27 +++++++++++++++++++++++++-- tests/test_core/test_coords.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/dascore/core/coords.py b/dascore/core/coords.py index e4c23e655..5e494e62e 100644 --- a/dascore/core/coords.py +++ b/dascore/core/coords.py @@ -129,6 +129,18 @@ def _reduce_time_like(func, data): return np.atleast_1d(out) +def _validate_new_length(length) -> int: + """Ensure a requested coordinate length is a non-negative integer.""" + # bool is an int subclass; True/False are never a sensible length. + if isinstance(length, bool) or not isinstance(length, int | np.integer): + msg = f"change_length requires an integer length, not {length!r}." + raise ParameterError(msg) + if length < 0: + msg = f"change_length requires a non-negative length, not {length}." + raise ParameterError(msg) + return int(length) + + def _get_dtype(value, dtype): """Get the data type based on the first argument.""" if dtype is not None and dtype != "": @@ -967,7 +979,12 @@ def change_length(self, length: int) -> Self: Parameters ---------- length - The output length. + The output length. Must be a non-negative integer. + + Raises + ------ + ParameterError + If length is not a non-negative integer. """ msg = f"Coordinate type {self.__class__} does not implement change_length" raise NotImplementedError(msg) @@ -1109,7 +1126,7 @@ def change_length(self, length: int) -> Self: if self.ndim != 1: msg = "change_length only works on 1D coords." raise CoordError(msg) - return get_coord(shape=(length,)) + return get_coord(shape=(_validate_new_length(length),)) def to_summary(self, dims=()) -> CoordSummary: """Get the summary info about the coord.""" @@ -1373,8 +1390,14 @@ def change_length(self, length: int) -> Self: """ # CoordRange is always 1D by construction; keep as an internal invariant. assert self.ndim == 1, "Can only change length for 1D coords." + length = _validate_new_length(length) if (current := len(self)) == length: return self + # A CoordRange always has at least one sample (start == stop has a + # length of 1) so an empty coord is the only way to represent 0. + if length == 0: + data = np.empty(0, dtype=self.dtype) + return get_coord(data=data, step=self.step, units=self.units) diff = length - current stop, step = self.stop, self.step out = self.update(stop=stop + step * diff) diff --git a/tests/test_core/test_coords.py b/tests/test_core/test_coords.py index 7e4a6e956..ea97e4e84 100644 --- a/tests/test_core/test_coords.py +++ b/tests/test_core/test_coords.py @@ -2005,6 +2005,38 @@ def test_not_implemented_in_baseclass(self, evenly_sampled_coord): with pytest.raises(NotImplementedError): BaseCoord.change_length(coord, 10) + @pytest.mark.parametrize("length", [-1, -10]) + def test_negative_length_raises( + self, evenly_sampled_coord, basic_non_coord, length + ): + """Ensure a negative length is rejected rather than making a bad coord.""" + for coord in (evenly_sampled_coord, basic_non_coord): + with pytest.raises(ParameterError, match="non-negative"): + coord.change_length(length) + + @pytest.mark.parametrize("length", [2.5, 3.0, "3", None, True, False]) + def test_non_integer_length_raises( + self, evenly_sampled_coord, basic_non_coord, length + ): + """Ensure non integer lengths are rejected.""" + for coord in (evenly_sampled_coord, basic_non_coord): + with pytest.raises(ParameterError, match="integer length"): + coord.change_length(length) + + def test_zero_length(self, evenly_sampled_coord, basic_non_coord): + """Ensure a length of zero produces an empty coord.""" + for coord in (evenly_sampled_coord, basic_non_coord): + assert len(coord.change_length(0)) == 0 + + def test_zero_length_keeps_metadata(self, evenly_sampled_float_coord_with_units): + """Ensure an emptied coord keeps its units, step and dtype.""" + coord = evenly_sampled_float_coord_with_units + out = coord.change_length(0) + assert len(out) == 0 + assert out.units == coord.units + assert out.step == coord.step + assert out.dtype == coord.dtype + class TestIssues: """Tests for special issues related to coords.""" From 9a0d9b6985b91ff88e463f7a80b418ff6aca8ad5 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Sat, 1 Aug 2026 07:54:49 +0200 Subject: [PATCH 07/12] Make demean, demedian, standardize, and normalize NaN aware (#793) --- dascore/proc/basic.py | 45 ++++++++++++------- tests/test_proc/test_basic.py | 85 +++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 16 deletions(-) diff --git a/dascore/proc/basic.py b/dascore/proc/basic.py index 82d1d6753..d41888df1 100644 --- a/dascore/proc/basic.py +++ b/dascore/proc/basic.py @@ -346,6 +346,10 @@ def normalize( """ Normalize a patch along a specified dimension. + NaN values are ignored when computing the norm. They remain NaN in the + output but do not affect any other sample. Slices with a norm of zero, + meaning they contain nothing but zeros and NaN, are returned unscaled. + Parameters ---------- dim @@ -376,26 +380,24 @@ def normalize( data = self.data if norm in {"l1", "l2"}: order = int(norm[-1]) - norm_values = np.linalg.norm(self.data, axis=axis, ord=order) + # Equivalent to np.linalg.norm, but skips NaN rather than letting a + # single null blank every sample sharing its slice. The float exponent + # promotes ints so the powers cannot overflow a narrow dtype. + norm_values = np.nansum(np.abs(data) ** float(order), axis=axis) ** (1 / order) + divisor = np.expand_dims(norm_values, axis=axis) elif norm == "max": - norm_values = np.max(np.abs(data), axis=axis) + divisor = np.expand_dims(np.nanmax(np.abs(data), axis=axis), axis=axis) elif norm == "bit": - pass + divisor = np.abs(data) else: msg = ( f"Norm value of {norm} is not supported. " f"Supported values are {('l1', 'l2', 'max', 'bit')}" ) raise ValueError(msg) - if norm == "bit": - new_data = np.divide( - data, np.abs(data), out=np.zeros_like(data), where=np.abs(data) != 0 - ) - else: - expanded_norm = np.expand_dims(norm_values, axis=axis) - new_data = np.divide( - data, expanded_norm, out=np.zeros_like(data), where=expanded_norm != 0 - ) + # A zero divisor means there is nothing but zeros and nulls to scale, so + # divide those by one; the zeros stay zero and the nulls stay null. + new_data = data / np.where(divisor == 0, 1, divisor) return self.new(data=new_data) @@ -413,6 +415,9 @@ def standardize( where u is the mean of the training samples or zero if with_mean=False, and s is the standard deviation of the training samples or one if with_std=False. + NaN values are ignored when computing the mean and standard deviation. They + remain NaN in the output but do not affect any other sample. + Parameters ---------- dim @@ -434,8 +439,8 @@ def standardize( """ axis = self.get_axis(dim) data = self.data - mean = np.mean(data, axis=axis, keepdims=True) - std = np.std(data, axis=axis, keepdims=True) + mean = np.nanmean(data, axis=axis, keepdims=True) + std = np.nanstd(data, axis=axis, keepdims=True) new_data = (data - mean) / std return self.new(data=new_data) @@ -838,6 +843,10 @@ def demedian(patch, dim: str = "time"): """ Remove the median along a given dimension of a DASCore patch. + NaN values are ignored when computing the median, consistent with + [Patch.median](`dascore.proc.aggregate.median`). They remain NaN in the + output but do not affect any other sample. + Parameters ---------- patch : @@ -884,7 +893,7 @@ def demedian(patch, dim: str = "time"): data = patch.data # Compute median along axis, keep dims for broadcasting - med = np.median(data, axis=axis, keepdims=True) + med = np.nanmedian(data, axis=axis, keepdims=True) new_data = data - med @@ -897,6 +906,10 @@ def demean(patch, dim: str = "time"): """ Remove the mean along a given dimension of a DASCore patch. + NaN values are ignored when computing the mean, consistent with + [Patch.mean](`dascore.proc.aggregate.mean`). They remain NaN in the output + but do not affect any other sample. + Parameters ---------- patch : @@ -943,7 +956,7 @@ def demean(patch, dim: str = "time"): data = patch.data # Compute mean along axis, keep dims for broadcasting - mea = np.mean(data, axis=axis, keepdims=True) + mea = np.nanmean(data, axis=axis, keepdims=True) new_data = data - mea diff --git a/tests/test_proc/test_basic.py b/tests/test_proc/test_basic.py index daa90b27c..de7af2e48 100644 --- a/tests/test_proc/test_basic.py +++ b/tests/test_proc/test_basic.py @@ -18,6 +18,13 @@ TEST_OPS = tuple(getattr(operator, x) for x in OP_NAMES) +def _patch_with_nan(patch, index=(0, 0)): + """Return a float patch with a single NaN so slice contamination shows.""" + data = np.asarray(patch.data, dtype=np.float64).copy() + data[index] = np.nan + return patch.new(data=data) + + @pytest.fixture(scope="session") def random_complex_patch(random_patch): """Swap out data for complex data.""" @@ -198,6 +205,56 @@ def test_zero_channels(self, random_patch): assert np.all(norm.data[0, :] == 0.0) assert np.all(norm.data[:, 0] == 0.0) + @pytest.mark.parametrize("norm", ["l1", "l2", "max"]) + @pytest.mark.parametrize("dim", ["time", "distance"]) + def test_nan_does_not_contaminate_slice(self, random_patch, dim, norm): + """A single NaN should not blank every value sharing its slice.""" + patch = _patch_with_nan(random_patch) + out = patch.normalize(dim, norm=norm) + assert np.isnan(out.data).sum() == 1 + + @pytest.mark.filterwarnings("ignore:All-NaN slice encountered") + @pytest.mark.parametrize("norm", ["l1", "l2", "max"]) + @pytest.mark.parametrize("dim", ["time", "distance"]) + def test_all_nan_slice_stays_null(self, random_patch, dim, norm): + """A completely null slice should stay null rather than become zeros.""" + data = np.asarray(random_patch.data, dtype=np.float64).copy() + # Null the first slice reduced by norm (patch is 2D, so the other axis). + other_axis = 1 - random_patch.get_axis(dim) + null_slice = tuple(0 if i == other_axis else slice(None) for i in range(2)) + data[null_slice] = np.nan + patch = random_patch.new(data=data) + + out = patch.normalize(dim, norm=norm) + + assert np.all(np.isnan(out.data[null_slice])) + assert not np.any(np.isnan(np.delete(out.data, 0, axis=other_axis))) + + @pytest.mark.parametrize("norm", ["l1", "l2", "max", "bit"]) + def test_null_in_zero_slice_stays_null(self, random_patch, norm): + """A null in an otherwise zero slice should stay null, not become zero.""" + data = np.zeros(random_patch.shape, dtype=np.float64) + data[0, 0] = np.nan + patch = random_patch.new(data=data) + + out = patch.normalize("time", norm=norm) + + assert np.isnan(out.data[0, 0]) + assert np.all(out.data[0, 1:] == 0) + + @pytest.mark.parametrize("norm", ["l1", "l2", "max", "bit"]) + def test_int_data(self, random_patch, norm): + """Integer data should normalize to floats without overflowing.""" + # 100 is large enough that squaring it overflows int8. + data = np.full(random_patch.shape, 100, dtype=np.int8) + samples = data.shape[random_patch.get_axis("time")] + expected = {"l1": 1 / samples, "l2": 1 / np.sqrt(samples), "max": 1, "bit": 1} + + out = random_patch.new(data=data).normalize("time", norm=norm) + + assert np.issubdtype(out.data.dtype, np.floating) + assert np.allclose(out.data, expected[norm]) + class TestStandardize: """Tests for standardization.""" @@ -211,6 +268,16 @@ def test_base_case(self, random_patch): out = random_patch.standardize("time") assert not np.any(pd.isnull(out.data)) + @pytest.mark.parametrize("dim", ["time", "distance"]) + def test_nan_does_not_contaminate_slice(self, random_patch, dim): + """A single NaN should not blank every value sharing its slice.""" + patch = _patch_with_nan(random_patch) + out = patch.standardize(dim) + axis = out.get_axis(dim) + assert np.isnan(out.data).sum() == 1 + assert np.allclose(np.nanmean(out.data, axis=axis), 0) + assert np.allclose(np.nanstd(out.data, axis=axis), 1) + def test_std(self, random_patch): """Ensure after operation standard deviations are 1.""" dims = random_patch.dims @@ -860,6 +927,15 @@ def test_demedian(self, random_patch, dim): medians = np.median(dem.data, axis=dem.get_axis(dim)) assert np.allclose(medians, 0) + @pytest.mark.parametrize("dim", ["time", "distance"]) + def test_nan_does_not_contaminate_slice(self, random_patch, dim): + """A single NaN should not blank every value sharing its slice.""" + patch = _patch_with_nan(random_patch) + out = patch.demedian(dim=dim) + assert np.isnan(out.data).sum() == 1 + medians = np.nanmedian(out.data, axis=out.get_axis(dim)) + assert np.allclose(medians, 0) + class TestDemean: """Tests for demean of data.""" @@ -872,3 +948,12 @@ def test_demean(self, random_patch, dim): dem = new.demean(dim=dim) means = np.mean(dem.data, axis=dem.get_axis(dim)) assert np.allclose(means, 0) + + @pytest.mark.parametrize("dim", ["time", "distance"]) + def test_nan_does_not_contaminate_slice(self, random_patch, dim): + """A single NaN should not blank every value sharing its slice.""" + patch = _patch_with_nan(random_patch) + out = patch.demean(dim=dim) + assert np.isnan(out.data).sum() == 1 + means = np.nanmean(out.data, axis=out.get_axis(dim)) + assert np.allclose(means, 0) From 265e51d1a5a570f4ebe69767d1774f87af08e8ba Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Tue, 4 Aug 2026 12:22:16 +0200 Subject: [PATCH 08/12] Validate the conflict argument of Spool.chunk (#806) --- dascore/utils/attrs.py | 13 ++++++++++++- dascore/utils/chunk.py | 3 ++- tests/test_core/test_patch_chunk.py | 10 ++++++++++ tests/test_utils/test_attrs_utils.py | 8 +++++++- tests/test_utils/test_chunk.py | 5 +++++ 5 files changed, 36 insertions(+), 3 deletions(-) diff --git a/dascore/utils/attrs.py b/dascore/utils/attrs.py index 73ea67183..dfc1fb983 100644 --- a/dascore/utils/attrs.py +++ b/dascore/utils/attrs.py @@ -14,7 +14,7 @@ import dascore as dc from dascore.constants import attr_conflict_description -from dascore.exceptions import AttributeMergeError +from dascore.exceptions import AttributeMergeError, ParameterError from dascore.utils.docs import compose_docstring from dascore.utils.misc import ( _dict_list_diffs, @@ -24,6 +24,16 @@ iterate, ) +_VALID_CONFLICT_VALUES = ("drop", "raise", "keep_first") + + +def validate_conflict(conflict: str) -> Literal["drop", "raise", "keep_first"]: + """Ensure a conflict(s) argument is a supported value.""" + if conflict not in _VALID_CONFLICT_VALUES: + msg = f"conflict must be one of {_VALID_CONFLICT_VALUES}, got {conflict!r}." + raise ParameterError(msg) + return conflict + @compose_docstring(conflict_desc=attr_conflict_description) def combine_patch_attrs( @@ -51,6 +61,7 @@ def combine_patch_attrs( shortcut if it has already been computed. """ # TODO this is a monstrosity! need to refactor. + validate_conflict(conflicts) model_fields = dc.core.CoordSummary.model_fields eq_coord_fields = set(model_fields) - {"min", "max", "step"} diff --git a/dascore/utils/chunk.py b/dascore/utils/chunk.py index 029de29f3..f5d54d6f7 100644 --- a/dascore/utils/chunk.py +++ b/dascore/utils/chunk.py @@ -12,6 +12,7 @@ from dascore.constants import attr_conflict_description, numeric_types, timeable_types from dascore.exceptions import ChunkError, CoordMergeError, ParameterError +from dascore.utils.attrs import validate_conflict from dascore.utils.docs import compose_docstring from dascore.utils.misc import get_middle_value from dascore.utils.pd import ( @@ -168,7 +169,7 @@ def __init__( self._snap_coords = snap_coords self._tolerance = tolerance self._name, self._value = self._validate_kwargs(kwargs) - self._attr_conflict = conflict + self._attr_conflict = validate_conflict(conflict) self._validate_chunker() def _validate_kwargs(self, kwargs): diff --git a/tests/test_core/test_patch_chunk.py b/tests/test_core/test_patch_chunk.py index 73bbcd327..f4c7d567a 100644 --- a/tests/test_core/test_patch_chunk.py +++ b/tests/test_core/test_patch_chunk.py @@ -473,6 +473,16 @@ def test_attrs_conflict(self, adjacent_spool_different_attrs): patch = out[0] assert isinstance(patch, dc.Patch) + def test_invalid_conflict_raises(self, adjacent_spool_different_attrs): + """ + An unrecognized conflict value should raise rather than silently + selecting undocumented behavior. See #804. + """ + spool = adjacent_spool_different_attrs + for bad_value in ("banana", "", None): + with pytest.raises(ParameterError, match="conflict must be one of"): + spool.chunk(time=..., conflict=bad_value) + def test_chunk_patches_with_non_coord(self, random_patch): """Tests for chunking when some patches have non coordinate dimensions.""" patches = [random_patch.mean("time") for _ in range(3)] diff --git a/tests/test_utils/test_attrs_utils.py b/tests/test_utils/test_attrs_utils.py index ba7af0f90..ac266a0ec 100644 --- a/tests/test_utils/test_attrs_utils.py +++ b/tests/test_utils/test_attrs_utils.py @@ -7,7 +7,7 @@ import pytest from dascore import PatchAttrs -from dascore.exceptions import AttributeMergeError +from dascore.exceptions import AttributeMergeError, ParameterError from dascore.utils.attrs import combine_patch_attrs, separate_coord_info @@ -50,6 +50,12 @@ def test_drop(self): out = combine_patch_attrs([pa1, pa2], drop_attrs="history") assert isinstance(out, PatchAttrs) + def test_invalid_conflicts_raises(self): + """An unsupported conflicts value should raise. See #804.""" + pa1, pa2 = PatchAttrs(tag="bob"), PatchAttrs(tag="bill") + with pytest.raises(ParameterError, match="conflict must be one of"): + combine_patch_attrs([pa1, pa2], conflicts="banana") + def test_conflicts(self): """Ensure when non-dim fields aren't equal merge raises.""" pa1 = PatchAttrs(tag="bob", another=2, same=42) diff --git a/tests/test_utils/test_chunk.py b/tests/test_utils/test_chunk.py index acf9b689c..1b8d51847 100644 --- a/tests/test_utils/test_chunk.py +++ b/tests/test_utils/test_chunk.py @@ -259,6 +259,11 @@ def test_raises_zero_length_chunk(self): with pytest.raises(ParameterError, match="must be greater than 0"): ChunkManager(time=0) + def test_raises_invalid_conflict(self): + """An unsupported conflict value should raise. See #804.""" + with pytest.raises(ParameterError, match="conflict must be one of"): + ChunkManager(time=None, conflict="banana") + def test_raises_invalid_key_in_kwargs(self, contiguous_df): """Ensure an invalid key in kwargs raises an error.""" chunk_manager = ChunkManager(Time=10) From 5cdf79341329b9aa5b6707f1660d8e8768b8ca83 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Tue, 4 Aug 2026 12:24:25 +0200 Subject: [PATCH 09/12] Wire Spool.chunk's snap_coords argument to the merge path (#805) --- dascore/core/spool.py | 20 +++++--- dascore/utils/chunk.py | 34 ++++++++++++- dascore/utils/patch.py | 11 ++-- tests/test_core/test_patch_chunk.py | 78 +++++++++++++++++++++++++++++ 4 files changed, 130 insertions(+), 13 deletions(-) diff --git a/dascore/core/spool.py b/dascore/core/spool.py index de616893b..406c42619 100644 --- a/dascore/core/spool.py +++ b/dascore/core/spool.py @@ -187,7 +187,9 @@ def chunk( This often occurs because of data gaps or at end of chunks. snap_coords If True, snap the coords on joined patches such that the spacing - remains constant. + remains constant. If False, keep the original coordinate values + of the joined patches, which can result in unevenly sampled + patches. tolerance The maximum number of samples a block of data can be spaced (gap) and still be considered contiguous. @@ -630,13 +632,17 @@ def _merge_patches_streaming(self, joined, df_dict_list, merge_dim, samples): f"{merge_dim} but found {found_dim}." ) raise CoordMergeError(msg) - conf = self._merge_kwargs.get("conflicts", None) + merge_kwargs = dict(self._merge_kwargs) + # snap_coords controls coordinate merging, not attr merging, so it + # must not be passed on to combine_patch_attrs. + snap = merge_kwargs.pop("snap_coords", True) + conf = merge_kwargs.get("conflicts", None) drop_conflicting = conf in {"drop", "keep_first"} - new_coord = _get_merged_coord(summary_df, merge_dim, coords, drop_conflicting) - coord = new_coord.coord_map[merge_dim] - new_attrs = combine_patch_attrs( - attrs, merge_dim, coord=coord, **self._merge_kwargs + new_coord = _get_merged_coord( + summary_df, merge_dim, coords, drop_conflicting, snap=snap ) + coord = new_coord.coord_map[merge_dim] + new_attrs = combine_patch_attrs(attrs, merge_dim, coord=coord, **merge_kwargs) return dc.Patch(data=buffer, coords=new_coord, attrs=new_attrs, dims=list(dims)) def _get_dummy_dataframes(self, current): @@ -703,7 +709,7 @@ def chunk( out_df, source_df=self._source_df, instruction_df=instructions, - merge_kwargs={"conflicts": conflict}, + merge_kwargs={"conflicts": conflict, "snap_coords": snap_coords}, ) def new_from_df( diff --git a/dascore/utils/chunk.py b/dascore/utils/chunk.py index f5d54d6f7..25ad36f15 100644 --- a/dascore/utils/chunk.py +++ b/dascore/utils/chunk.py @@ -14,7 +14,7 @@ from dascore.exceptions import ChunkError, CoordMergeError, ParameterError from dascore.utils.attrs import validate_conflict from dascore.utils.docs import compose_docstring -from dascore.utils.misc import get_middle_value +from dascore.utils.misc import all_diffs_close_enough, get_middle_value from dascore.utils.pd import ( _instructions_modified, _remove_overlaps, @@ -136,6 +136,11 @@ class ChunkManager: keep_partial If True, keep segments which are shorter than chunk size (at end of contiguous blocks) + snap_coords + If True, joined coordinates are snapped to a constant step, so the + chunked segments advertise a single step. If False, segments whose + sources don't line up on a regular grid have no single step and + get a NaN step. tolerance The upper limit of a gap to tolerate in terms of the sampling along the desired dimension. E.G., the default value means entities @@ -252,11 +257,36 @@ def _get_duration_overlap(self, duration, start, step, overlap=None): overlap = np.asarray([0], dtype=step.dtype)[0] return duration, overlap + def _get_group_step(self, df): + """ + Get the sampling step to advertise for a group of source rows. + + When snapping is disabled and the sources don't line up on a + regular grid, the merged coordinate keeps its uneven values, so no + single step describes the output; return NaN in that case. + """ + steps = df[f"{self._name}_step"].values + step = get_middle_value(steps) + if self._snap_coords or len(df) < 2: + return step + mins = df[f"{self._name}_min"].values + maxs = df[f"{self._name}_max"].values + order = np.argsort(mins) + # The merged coord's diffs are each source's step plus the gaps + # between sources; apply the same evenly-sampled test get_coord + # uses on the concatenated values. + boundary_diffs = mins[order][1:] - maxs[order][:-1] + if all_diffs_close_enough(np.concatenate([steps, boundary_diffs])): + return step + if np.issubdtype(np.asarray(steps).dtype, np.timedelta64): + return np.timedelta64("NaT", "ns") + return np.nan + def _create_df(self, df, name, start_stop, gnum): """Reconstruct the dataframe.""" cols = f"{name}_min", f"{name}_max" out = pd.DataFrame(start_stop, columns=list(cols)) - out[f"{name}_step"] = get_middle_value(df[f"{name}_step"].values) + out[f"{name}_step"] = self._get_group_step(df) merger = df.drop(columns=out.columns) # get dims to determine which columns are still compared. Some test # dfs don't have dims though, so it should still work without dims col. diff --git a/dascore/utils/patch.py b/dascore/utils/patch.py index 74da3c6e1..b1abfd742 100644 --- a/dascore/utils/patch.py +++ b/dascore/utils/patch.py @@ -416,13 +416,13 @@ def _maybe_expected_step(df, dim): return None -def _get_merged_coord(df, merge_dim, coords, drop_conflicting=False): +def _get_merged_coord(df, merge_dim, coords, drop_conflicting=False, snap=True): """Get merged coordinates, also validate anticipated sampling.""" new_coord = merge_coord_managers( coords, dim=merge_dim, drop_conflicting=drop_conflicting ) expected_step = _maybe_expected_step(df, merge_dim) - if not pd.isnull(expected_step): + if snap and not pd.isnull(expected_step): new_coord = new_coord.snap(merge_dim)[0] # TODO slightly different dt can be produced, let pass for now # need to think more about how the merging should work. @@ -438,7 +438,10 @@ def _force_patch_merge(patch_dict_list, merge_kwargs, **kwargs): """ df = pd.DataFrame(patch_dict_list) merge_dim = _get_merge_dim(df) - merge_kwargs = merge_kwargs if merge_kwargs is not None else {} + merge_kwargs = dict(merge_kwargs) if merge_kwargs is not None else {} + # snap_coords controls coordinate merging, not attr merging, so it + # must not be passed on to combine_patch_attrs. + snap = merge_kwargs.pop("snap_coords", True) if merge_dim is None: # nothing to merge, complete overlap return [patch_dict_list[0]] dims = df["dims"].iloc[0].split(",") @@ -454,7 +457,7 @@ def _force_patch_merge(patch_dict_list, merge_kwargs, **kwargs): # Determine if conflicting non-dimensional coords should be dropped. conf = merge_kwargs.get("conflicts", None) drop_conf_coords = True if conf in {"drop", "keep_first"} else False - new_coord = _get_merged_coord(df, merge_dim, coords, drop_conf_coords) + new_coord = _get_merged_coord(df, merge_dim, coords, drop_conf_coords, snap=snap) coord = new_coord.coord_map[merge_dim] if merge_dim in dims else None new_attrs = combine_patch_attrs(attrs, merge_dim, coord=coord, **merge_kwargs) patch = dc.Patch(data=new_data, coords=new_coord, attrs=new_attrs, dims=dims) diff --git a/tests/test_core/test_patch_chunk.py b/tests/test_core/test_patch_chunk.py index f4c7d567a..0474c551b 100644 --- a/tests/test_core/test_patch_chunk.py +++ b/tests/test_core/test_patch_chunk.py @@ -628,6 +628,84 @@ def test_chunk_non_adjacent_within_tolerance_warns(self, random_patch): assert len(out) == 1 +class TestChunkSnapCoords: + """Tests for the snap_coords argument of chunk. See #803.""" + + @pytest.fixture(scope="class") + def spool_irregular_boundary(self, random_patch): + """ + Two patches whose boundary is 1.4 samples apart: within the default + tolerance of 1.5, so chunk merges them. + """ + p1 = random_patch + time = p1.get_coord("time") + p2 = p1.update_attrs(time_min=time.max() + time.step * 1.4) + return dc.spool([p1, p2]) + + def test_snap_false_preserves_values(self, spool_irregular_boundary): + """snap_coords=False must keep the original coordinate values.""" + p1, p2 = spool_irregular_boundary[0], spool_irregular_boundary[1] + merged = spool_irregular_boundary.chunk(time=None, snap_coords=False) + assert len(merged) == 1 + expected = np.concatenate([p1.get_array("time"), p2.get_array("time")]) + assert np.array_equal(merged[0].get_array("time"), expected) + + def test_snap_true_evenly_samples(self, spool_irregular_boundary): + """The default (snap_coords=True) still re-grids to a constant step.""" + merged = spool_irregular_boundary.chunk(time=None) + assert len(merged) == 1 + time = merged[0].get_array("time") + assert len(np.unique(np.diff(time))) == 1 + + def test_snap_false_materialized_merge(self, spool_irregular_boundary, monkeypatch): + """snap_coords=False must also work on the non-streaming merge path.""" + import dascore.core.spool as spool_module + + p1, p2 = spool_irregular_boundary[0], spool_irregular_boundary[1] + monkeypatch.setattr( + spool_module, "_estimate_merge_samples", lambda df, dim: None + ) + merged = spool_irregular_boundary.chunk(time=None, snap_coords=False) + expected = np.concatenate([p1.get_array("time"), p2.get_array("time")]) + assert np.array_equal(merged[0].get_array("time"), expected) + + def test_snap_false_contents_step( + self, spool_irregular_boundary, adjacent_spool_no_overlap + ): + """ + The advertised step must be NaN when a no-snap merge leaves the + coordinate uneven, and stay concrete for contiguous or snapped merges. + """ + merged = spool_irregular_boundary.chunk(time=None, snap_coords=False) + assert pd.isnull(merged.get_contents()["time_step"].iloc[0]) + assert pd.isnull(merged[0].attrs["time_step"]) + # Snapping produces an even coordinate, so the step remains. + snapped = spool_irregular_boundary.chunk(time=None) + assert not pd.isnull(snapped.get_contents()["time_step"].iloc[0]) + # So does merging contiguous patches without snapping. + contiguous = adjacent_spool_no_overlap.chunk(time=None, snap_coords=False) + assert not pd.isnull(contiguous.get_contents()["time_step"].iloc[0]) + + def test_snap_false_distance_merge(self, random_patch): + """Float-typed coords also keep values and get a NaN step. See #803.""" + p1 = random_patch.update_coords( + distance=random_patch.get_array("distance").astype(np.float64) + ) + dist = p1.get_coord("distance") + p2 = p1.update_attrs(distance_min=dist.max() + dist.step * 1.4) + spool = dc.spool([p1, p2]).chunk(distance=None, snap_coords=False) + assert np.isnan(spool.get_contents()["distance_step"].iloc[0]) + expected = np.concatenate([p1.get_array("distance"), p2.get_array("distance")]) + assert np.array_equal(spool[0].get_array("distance"), expected) + + def test_snap_false_contiguous_unchanged(self, adjacent_spool_no_overlap): + """Truly contiguous patches merge identically with snapping disabled.""" + merged = adjacent_spool_no_overlap.chunk(time=None, snap_coords=False) + snapped = adjacent_spool_no_overlap.chunk(time=None) + assert len(merged) == len(snapped) == 1 + assert np.array_equal(merged[0].get_array("time"), snapped[0].get_array("time")) + + class TestStreamingMerge: """ Tests for the streaming merge path, which copies each patch into a From f54196118514c9211ee78b1127c39122692fd6bc Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Tue, 4 Aug 2026 14:03:43 +0200 Subject: [PATCH 10/12] Fix incorrect code and prose in tutorial docs (#808) --- docs/index.qmd | 2 +- docs/recipes/smoothing.qmd | 2 +- docs/tutorial/coords.qmd | 26 +++++++++++++++++--------- docs/tutorial/patch.qmd | 6 +++--- docs/tutorial/processing.qmd | 2 +- docs/tutorial/spool.qmd | 6 +++--- 6 files changed, 26 insertions(+), 18 deletions(-) diff --git a/docs/index.qmd b/docs/index.qmd index 0d4c823d5..da9cc4b03 100644 --- a/docs/index.qmd +++ b/docs/index.qmd @@ -59,7 +59,7 @@ spool = ( # Index the directory contents .update() # Sub-select a specific time range - .select(time_min=('2020-01-01', ...)) + .select(time=('2020-01-01', ...)) # Specify chunk of the output patches .chunk(time=60, overlap=10) ) diff --git a/docs/recipes/smoothing.qmd b/docs/recipes/smoothing.qmd index c5e507a96..ac62bcdf7 100644 --- a/docs/recipes/smoothing.qmd +++ b/docs/recipes/smoothing.qmd @@ -1,7 +1,7 @@ --- title: "Patch Smoothing" execute: - warn: false + warning: false --- This recipe compares several smoothing strategies. diff --git a/docs/tutorial/coords.qmd b/docs/tutorial/coords.qmd index 31cc8ffea..f5d9b7c03 100644 --- a/docs/tutorial/coords.qmd +++ b/docs/tutorial/coords.qmd @@ -120,21 +120,27 @@ sorted_data = data[indexer, :] ``` ### Snap -['snap'](`dascore.core.coords.BaseCoord.snap`) is used to calculate an average spacing between samples and "snap" all values to that spacing. If the coordinate is not sorted, it will be sorted in the process. This method should be used with care since it causes some loss in precision and can introduce inaccuracies in down-stream calculations. The min and max of the coordinate remain unchanged. +[`snap`](`dascore.core.coords.BaseCoord.snap`) is used to calculate an average spacing between samples and "snap" all values to that spacing. If the coordinate is not sorted, it will be sorted in the process. This method should be used with care since it causes some loss in precision and can introduce inaccuracies in down-stream calculations. The min and max of the coordinate remain unchanged. + +Unlike [`sort`](`dascore.core.coords.BaseCoord.sort`), `snap` returns only a new coordinate, not an indexer. Since snapping an unsorted coordinate also reorders it, use [`CoordManager.snap`](`dascore.core.coordmanager.CoordManager.snap`) instead when an associated data array needs to stay aligned. ```{python} import numpy as np from dascore.core import get_coord -random_array = np.random.rand(10) +random_array = np.random.rand(10) random_coord = get_coord(data=random_array) -sorted_coord, indexer = random_coord.sort() +# Snap returns a single, evenly sampled (and sorted) coordinate. +snapped_coord = random_coord.snap() -# The data array can be updated like so: -data = np.random.rand(10, 20) -sorted_data = data[indexer, :] +print(f"before snapping, evenly sampled: {random_coord.evenly_sampled}") +print(f"after snapping, evenly sampled: {snapped_coord.evenly_sampled}") + +# The min and max are unchanged (up to floating point precision). +assert np.isclose(random_coord.min(), snapped_coord.min()) +assert np.isclose(random_coord.max(), snapped_coord.max()) ``` @@ -254,7 +260,7 @@ coord_dict = { } cm_many_coords = get_coord_manager(coords=coord_dict, dims=("dim1", "dim2")) -print(cm) +print(cm_many_coords) ``` ### Update @@ -298,8 +304,10 @@ and drop or disassociate coordinates. # Disassociate "new_coord" from dimension distance. new_cm_3 = new_cm_1.update(new_coord=(None, new_coord)) -# Drop coordinate "new_coord". -new_cm_4 = cm.update(new_coord=None) +# Drop coordinate "new_coord". Note this must be done on a coord manager +# which actually has "new_coord"; dropping a missing coord is a no-op. +new_cm_4 = new_cm_1.update(new_coord=None) +assert "new_coord" not in new_cm_4.coord_map # Drop dimension "time". new_cm_5 = cm.update(time=None) diff --git a/docs/tutorial/patch.qmd b/docs/tutorial/patch.qmd index c0be96a37..96f033e97 100644 --- a/docs/tutorial/patch.qmd +++ b/docs/tutorial/patch.qmd @@ -784,9 +784,9 @@ patch_reduced = patch.add.reduce("distance") ``` -```{.note} -PatchUFunc.reduce/accumulate accept dim (mapped internally to axis), while numpy ufuncs accept axis. -``` +:::{.callout-note} +`PatchUFunc.reduce`/`accumulate` accept `dim` (mapped internally to `axis`), while numpy ufuncs accept `axis`. +::: For more advanced usage, you can create patch ufuncs. diff --git a/docs/tutorial/processing.qmd b/docs/tutorial/processing.qmd index 62cd3c53c..34e728655 100644 --- a/docs/tutorial/processing.qmd +++ b/docs/tutorial/processing.qmd @@ -161,7 +161,7 @@ Here is an example of using a rolling mean to smooth along the time axis: import dascore as dc patch = dc.get_example_patch("example_event_1") -# Apply moving mean window over every 10 samples +# Apply moving mean window over every 50 samples dt = patch.get_coord('time').step smoothed = patch.rolling(time=50*dt).mean() diff --git a/docs/tutorial/spool.qmd b/docs/tutorial/spool.qmd index 54e19acc1..79a57065d 100644 --- a/docs/tutorial/spool.qmd +++ b/docs/tutorial/spool.qmd @@ -126,10 +126,10 @@ import numpy as np spool = dc.get_example_spool() # Get an array of integers which indicate the index of included patches -bool_array = np.array([2, 0]) +index_array = np.array([2, 0]) # create a new spool with patch 2 and patch 0. -new = spool[bool_array] +new = spool[index_array] ``` # get_contents @@ -223,7 +223,7 @@ print(merged[0].coords) The [`map`](`dascore.core.spool.BaseSpool.map`) method applies a function to all patches in the spool. It provides an efficient way to process large datasets, especially when combined with clients (aka executors). -For example, calculating the maximum value for each channel (distance) for 4 second increments with 1 second overlap can be done like so: +For example, calculating the maximum value for each channel (distance) for 5 second increments with 1 second overlap can be done like so: ```{python} import dascore as dc From 26008f10d4bbd9b6b4e6a5b4a7c5d8fa59ad1f7a Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Tue, 4 Aug 2026 15:01:34 +0200 Subject: [PATCH 11/12] Don't let range queries on columns silently return nothing (#810) --- dascore/utils/pd.py | 39 ++++++++++++++++++++-- tests/test_utils/test_pd.py | 66 +++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/dascore/utils/pd.py b/dascore/utils/pd.py index 400980c76..05e1ad9e1 100644 --- a/dascore/utils/pd.py +++ b/dascore/utils/pd.py @@ -134,9 +134,34 @@ def _filter_equality(query_dict, df, bool_index): return bool_index +def _check_misdirected_range_query(key, val, df): + """ + Raise if a range query was aimed at an interval column. + + Columns like time_min/time_max hold the limits of each row, and a + collection of values applied to one is a membership (isin) check. An open + bound (... or None) in such a collection is meaningless, and signals a + range query which belongs on the dimension instead, eg + time_min=(t1, ...) should be time=(t1, ...). Without this the query would + silently match nothing. + """ + base = key[:-4] if key.endswith(("_min", "_max")) else None + if base is None or not {f"{base}_min", f"{base}_max"}.issubset(set(df.columns)): + return + if not any(x is ... or x is None for x in val): + return + msg = ( + f"An open bound (... or None) is not valid in the query for column " + f"'{key}'; a collection of values is an isin check, not a range. " + f"Use {base}=(min, max) to query a range of {base} values." + ) + raise ParameterError(msg) + + def _filter_contains(query_dict, df, bool_index): """Filter based on rows containing specified values.""" for key, val in query_dict.items(): + _check_misdirected_range_query(key, val, df) bool_index = np.logical_and(bool_index, df[key].isin(val)) return bool_index @@ -170,6 +195,16 @@ def _filter_multicolumn_range(query_dict, df, bool_index): return bool_index +def _convert_range_bounds(range_tuple, func): + """ + Apply a time conversion to each bound of a range. + + Unbounded (None) ends are left alone; converting them would produce NaT, + which compares False against everything and would silently empty the query. + """ + return tuple(None if x is None else func(x) for x in range_tuple) + + def _convert_times(df, some_dict): """Convert query values to datetime/timedelta values.""" if not some_dict: @@ -181,12 +216,12 @@ def _convert_times(df, some_dict): non_min_max_cols & set(some_dict) ) for key in datetime_keys: - some_dict[key] = to_datetime64(some_dict[key]) + some_dict[key] = _convert_range_bounds(some_dict[key], to_datetime64) # convert queries related to time delta into timedelta64 timedelta_cols = set(df.select_dtypes(include=np.timedelta64).columns) timedelta_keys = timedelta_cols & set(some_dict) for key in timedelta_keys: - some_dict[key] = to_timedelta64(some_dict[key]) + some_dict[key] = _convert_range_bounds(some_dict[key], to_timedelta64) return some_dict diff --git a/tests/test_utils/test_pd.py b/tests/test_utils/test_pd.py index 2ebeb3292..4e792c5d8 100644 --- a/tests/test_utils/test_pd.py +++ b/tests/test_utils/test_pd.py @@ -227,6 +227,72 @@ def test_timedelta_columns(self, example_df_timedeltas): out = filter_df(df, time_step_min=0.5, time_step_max=2) assert out.all() + def test_open_ended_datetime_range(self, example_df_2): + """ + An unbounded end of a range on a datetime column should not exclude + everything. See #808. + """ + # 'time' must be a plain column, ie not paired with time_min/time_max. + df = pd.DataFrame({"time": example_df_2["time_min"]}) + cutoff = df["time"].iloc[2] + assert np.all(filter_df(df, time_min=cutoff) == (df["time"] >= cutoff)) + assert np.all(filter_df(df, time_max=cutoff) == (df["time"] <= cutoff)) + + def test_open_ended_timedelta_range(self, example_df_2): + """Same as above, but for timedelta columns.""" + df = example_df_2.assign(step=example_df_2["time_step"] * range(5)) + cutoff = df["step"].iloc[2] + assert np.all(filter_df(df, step_min=cutoff) == (df["step"] >= cutoff)) + assert np.all(filter_df(df, step_max=cutoff) == (df["step"] <= cutoff)) + + @pytest.mark.parametrize( + "val", + [ + ("2020-01-03", ...), + (..., "2020-01-03"), + ("2020-01-03", None), + (None, "2020-01-03"), + ], + ) + def test_open_bound_on_interval_column_raises(self, example_df_2, val): + """ + An open bound in a membership query on an interval column is a range + query aimed at the wrong key; it should raise rather than silently + return an empty result. See #808. + """ + with pytest.raises(ParameterError, match=r"Use time=\(min, max\)"): + filter_df(example_df_2, time_min=val) + + def test_closed_range_on_interval_column_still_isin(self, example_df_2): + """ + Without an open bound the query is ambiguous, so the documented isin + behavior is kept. + """ + vals = list(example_df_2["bp_min"].iloc[:2]) + out = filter_df(example_df_2, bp_min=tuple(vals)) + assert np.all(out == example_df_2["bp_min"].isin(vals)) + + def test_ellipsis_kept_for_non_interval_column(self, example_df_2): + """ + Columns with no min/max pair are plain isin checks; an ellipsis there + contributes nothing but must not break the rest of the collection. + """ + out = filter_df(example_df_2, first_name=("Jason", ...)) + assert np.all(out == example_df_2["first_name"].isin(["Jason"])) + + def test_open_bound_ignored_for_unknown_column(self, example_df_2): + """ + Unknown columns are forwarded to patch level select, so an open bound + in one of them must not raise here. + """ + out = filter_df(example_df_2, not_a_column=(1, ...), ignore_bad_kwargs=True) + assert out.all() + + def test_spool_select_open_bound_on_interval_column(self, random_spool): + """The user facing path which prompted this: spool.select(time_min=...).""" + with pytest.raises(ParameterError, match=r"Use time=\(min, max\)"): + random_spool.select(time_min=("2020-01-01", ...)) + class TestAdjustSegments: """Tests for adjusting segments of dataframes.""" From 37a03456278c507f360bf65969ee0a95927f543b Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Tue, 4 Aug 2026 16:53:36 +0200 Subject: [PATCH 12/12] Drop the from_parts construction benchmark The benchmark went with Patch.from_parts, which this merge removes. --- benchmarks/test_patch_benchmarks.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/benchmarks/test_patch_benchmarks.py b/benchmarks/test_patch_benchmarks.py index 27d045249..525fb1bbe 100644 --- a/benchmarks/test_patch_benchmarks.py +++ b/benchmarks/test_patch_benchmarks.py @@ -220,12 +220,6 @@ def test_new_with_coords_and_attrs(self, example_patch, new_data): patch = example_patch patch.new(data=new_data, coords=patch.coords, attrs=patch.attrs) - @pytest.mark.benchmark - def test_from_parts(self, example_patch, new_data): - """Time the fast constructor for already conforming parts.""" - patch = example_patch - dc.Patch.from_parts(new_data, patch.coords, patch.attrs) - @pytest.mark.benchmark def test_patch_init(self, example_patch, new_data): """