From 929eb979b8822684f0c454c3290f5338d436c3cf Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Mon, 6 Jul 2026 23:03:06 +0200 Subject: [PATCH 1/7] Improve draft-release skill effectiveness (#745) --- .agents/skills/draft-release/SKILL.md | 47 ++++++++++++++++++++------- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/.agents/skills/draft-release/SKILL.md b/.agents/skills/draft-release/SKILL.md index 585592d12..f6eea595b 100644 --- a/.agents/skills/draft-release/SKILL.md +++ b/.agents/skills/draft-release/SKILL.md @@ -14,13 +14,19 @@ Draft the next release version and changelog from merged PRs. ## Workflow -0. Ask for elevated permissions with network access to run `git fetch --all --tags` and the GitHub CLI PR commands used to collect merged PRs (for example `gh pr list`, `gh pr view`, or `gh api`). -1. Fetch the latest refs and tags: +0. Ask for elevated permissions with network access to run the `git fetch` and GitHub CLI PR commands used to collect merged PRs (for example `gh pr list`, `gh pr view`, or `gh api`). +1. Fetch the latest refs and tags from `origin` only. Do **not** use + `git fetch --all` — it also fetches unrelated remotes (e.g. a personal fork or + another contributor's remote) and can fail on those or clobber local tags, + aborting the whole fetch: ```bash -git fetch --all --tags +git fetch --tags origin ``` + If you only need to read the tags without touching local state, use + `git ls-remote --tags origin` instead. + 2. Determine the next version number: - Consider only tags that start with `v` and match strict semver: `^v[0-9]+\.[0-9]+\.[0-9]+$` (ignore pre-release/build suffixes). @@ -45,18 +51,32 @@ git fetch --all --tags - `Bug Fixes` - `Breaking Changes` -5. Classify PRs deterministically: -- `Breaking Changes` if any of: - - title contains `!` in conventional-commit style segment, or - - label indicates breaking change (e.g., `breaking`), or - - body contains `BREAKING CHANGE`. -- Otherwise `New Features` if labels/titles indicate feature work - (e.g., `feature`, `enhancement`, `feat`). +5. Drop reverted pairs first. If a PR in scope reverts another PR that is also + in scope (revert PRs usually say "Revert ..." and name the reverted PR or + commit in the title/body), the two cancel out to no net user-facing change. + Omit both from the sections and instead list them under a short + `Reverted (no net change)` note at the end, so the reader knows why those PR + numbers are absent. + +6. Classify the remaining PRs. This repo does not use conventional-commit + markers, and its labels are topical (`proc`, `viz`, `spool`, `IO`, + `transform`, `bug`, ...) rather than semantic, so labels alone are not + enough — read each PR's title and body and use judgment: +- `Breaking Changes` if the change removes or alters existing public API, + defaults, or behavior in a way that can break callers — regardless of whether + any `!`, `breaking` label, or `BREAKING CHANGE` text is present. A signature + or keyword change to a documented `Patch`/`dc` method is breaking even when + unlabeled; when unsure, list it here with a one-line note on what changed. +- Otherwise `New Features` if the PR adds a capability, option, or notable + performance improvement (judge from the title/body, not just a + `feature`/`enhancement` label, which is often missing). - Otherwise `Bug Fixes`. -- Sort entries by PR number ascending. +- Prefer user-facing behavior over internal implementation when deciding and + when summarizing. +- Sort entries within each section by PR number ascending. - Include a link to the PR in the changelog. -6. Print to screen: +7. Print to screen: - The new version tag. - The drafted changelog. @@ -73,6 +93,8 @@ Next Version: vX.Y.Z ## Breaking Changes - #125: Short summary (https://github.com/OWNER/REPO/pull/125) + +Reverted (no net change): #126 reverted by #127 ``` ## Notes @@ -81,3 +103,4 @@ Next Version: vX.Y.Z - Prefer explicit, user-facing PR summaries over internal implementation details. - If no merged PRs are found in scope, still print the next version and include all sections with `- None`. +- Omit the `Reverted (no net change)` line when no reverted pairs exist. From eeb9be4d67809099bff7e4bf44f46b8be3b4dd24 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 9 Jul 2026 22:56:40 +0200 Subject: [PATCH 2/7] Raise CoordError instead of assert for non-1D coord operations (#747) * Raise CoordError instead of assert for non-1D coord operations Several coordinate operations that only support 1D coords guarded their input with `assert`, which raises a bare AssertionError and is stripped entirely under `python -O` (so the check silently vanishes in optimized runs). Convert the user-reachable ones -- select-by-sample-array, align_to, get_sample_count, CoordPartial.change_length, and CoordRange construction -- to raise CoordError. Genuine internal invariants that are impossible by construction (CoordRange.change_length) stay as asserts. Add tests covering each new error path. * Extend assert->raise cleanup to proc, viz, and wav IO Apply the same treatment repo-wide to user-reachable asserts that validate caller input, converting them to ParameterError: - proc/taper: taper window must be a length-2 sequence - proc/detrend: dim must be in the patch - proc/correlate: patch must be 2D - viz/map_fiber: x/y/color must be existing coords; scale_type and scale validated - io/wav: only single-patch spools can be written to wav Internal invariants (impossible-by-construction shape/postcondition checks, binary-format parser consistency) are left as asserts. Adds tests for every new error path; the five changed modules keep 100% line coverage. --- dascore/core/coords.py | 21 ++++++++++---- dascore/io/wav/core.py | 5 +++- dascore/proc/correlate.py | 5 +++- dascore/proc/detrend.py | 5 +++- dascore/proc/taper.py | 4 ++- dascore/viz/map_fiber.py | 21 ++++++++++---- tests/test_core/test_coords.py | 46 ++++++++++++++++++++++++++++++ tests/test_io/test_wav/test_wav.py | 8 ++++++ tests/test_proc/test_correlate.py | 13 ++++++++- tests/test_proc/test_detrend.py | 8 ++++++ tests/test_proc/test_taper.py | 9 ++++++ tests/test_viz/test_map_fiber.py | 30 +++++++++++++++++++ 12 files changed, 160 insertions(+), 15 deletions(-) diff --git a/dascore/core/coords.py b/dascore/core/coords.py index 56bfe386b..cf1a2636f 100644 --- a/dascore/core/coords.py +++ b/dascore/core/coords.py @@ -347,7 +347,9 @@ 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 - assert self.ndim <= 1, "Select only works on 1D coords." + if self.ndim > 1: + msg = "Select only works on 1D coords." + raise CoordError(msg) inds = np.arange(len(self)) valid_values = np.isin(inds, array) return self[valid_values], valid_values @@ -430,7 +432,9 @@ def valid_non_coord(coord1, coord2): if self == other: return self, other, slice(None), slice(None) - assert self.ndim == 1, "can only align 1D arrays." + if self.ndim != 1: + msg = "can only align 1D coords." + raise CoordError(msg) if isinstance(self, CoordPartial) or isinstance(other, CoordPartial): valid_non_coord(self, other) return self, other, slice(None), slice(None) @@ -821,7 +825,9 @@ def get_sample_count(self, value, samples=False, enforce_lt_coord=False) -> int: If True, raise an error if the number of samples obtained exceeds the length of the coordinate. """ - assert self.ndim == 1, "get sample count only works for 1D coords." + if self.ndim != 1: + msg = "get sample count only works for 1D coords." + raise CoordError(msg) if not self.evenly_sampled: msg = "Coordinate is not evenly sampled, can't get sample count." raise CoordError(msg) @@ -1100,7 +1106,9 @@ def change_length(self, length: int) -> Self: """ {doc} """ - assert self.ndim == 1, "change_length only works on 1D coords." + if self.ndim != 1: + msg = "change_length only works on 1D coords." + raise CoordError(msg) return get_coord(shape=(length,)) def to_summary(self, dims=()) -> CoordSummary: @@ -1162,7 +1170,9 @@ def _maybe_unbox_scalar(value): start, stop, step, shape = _attrs if not pd.isnull(shape): shape = tuple(iterate(shape)) - assert len(shape) == 1, "Coord range only works for 1D coords." + if len(shape) != 1: + msg = "Coord range only works for 1D coords." + raise CoordError(msg) length = shape[0] if pd.isnull(start): start = stop - step * length @@ -1361,6 +1371,7 @@ def change_length(self, length: int) -> Self: """ {doc} """ + # CoordRange is always 1D by construction; keep as an internal invariant. assert self.ndim == 1, "Can only change length for 1D coords." if (current := len(self)) == length: return self diff --git a/dascore/io/wav/core.py b/dascore/io/wav/core.py index c6d6fd927..92f25b3bc 100644 --- a/dascore/io/wav/core.py +++ b/dascore/io/wav/core.py @@ -8,6 +8,7 @@ from scipy.io.wavfile import write from dascore.constants import ONE_SECOND, SpoolType +from dascore.exceptions import ParameterError from dascore.io.core import FiberIO from dascore.utils.patch import check_patch_coords @@ -54,7 +55,9 @@ def write( see if this fixes the issue. """ resource = Path(resource) - assert len(spool) == 1, "Only single patch spools can be written to wav" + if len(spool) != 1: + msg = "Only single patch spools can be written to wav" + raise ParameterError(msg) patch = spool[0] # write a single wav file, maybe multi-channeled. data, sr = self._get_wav_data(patch, resample_frequency) diff --git a/dascore/proc/correlate.py b/dascore/proc/correlate.py index fb74f97a5..0d63fec18 100644 --- a/dascore/proc/correlate.py +++ b/dascore/proc/correlate.py @@ -8,6 +8,7 @@ import dascore as dc from dascore.constants import PatchType +from dascore.exceptions import ParameterError from dascore.utils.patch import ( get_dim_axis_value, patch_function, @@ -196,7 +197,9 @@ def correlate( "(e.g., select(lag_time=(...)))" ) warnings.warn(msg, DeprecationWarning) - assert len(patch.dims) == 2, "must be a 2D patch." + if len(patch.dims) != 2: + msg = "must be a 2D patch." + raise ParameterError(msg) dim, source_axis, source = get_dim_axis_value(patch, kwargs=kwargs)[0] # Get the axis and coord over which fft should be calculated. fft_axis = next(iter(set(range(len(patch.dims))) - {source_axis})) diff --git a/dascore/proc/detrend.py b/dascore/proc/detrend.py index 45eb19ac5..44577fe6f 100644 --- a/dascore/proc/detrend.py +++ b/dascore/proc/detrend.py @@ -5,6 +5,7 @@ from typing import Literal from dascore.constants import PatchType +from dascore.exceptions import ParameterError from dascore.utils.imports import lazy_import from dascore.utils.patch import patch_function @@ -36,7 +37,9 @@ def detrend( >>> pa = dascore.get_example_patch() # generate example patch >>> out = pa.detrend("time") # detrend along the time dimension """ - assert dim in patch.dims + if dim not in patch.dims: + msg = f"dim '{dim}' is not in patch dimensions {patch.dims}" + raise ParameterError(msg) axis = patch.get_axis(dim) out = scipy_detrend(patch.data, axis=axis, type=type) return patch.new(data=out) diff --git a/dascore/proc/taper.py b/dascore/proc/taper.py index e454ace98..7edf936ec 100644 --- a/dascore/proc/taper.py +++ b/dascore/proc/taper.py @@ -23,7 +23,9 @@ def _get_taper_slices(patch, kwargs): dim, axis, value = get_dim_axis_value(patch, kwargs=kwargs)[0] coord = patch.coords.coord_map[dim] if isinstance(value, Sequence | np.ndarray): - assert len(value) == 2, "Length 2 sequence required." + if len(value) != 2: + msg = "Length 2 sequence required." + raise ParameterError(msg) start, stop = value[0], value[1] else: start, stop = value, value diff --git a/dascore/viz/map_fiber.py b/dascore/viz/map_fiber.py index 41259d768..98bbca488 100644 --- a/dascore/viz/map_fiber.py +++ b/dascore/viz/map_fiber.py @@ -9,6 +9,7 @@ import numpy as np from dascore.constants import PatchType +from dascore.exceptions import ParameterError from dascore.utils.patch import patch_function from dascore.utils.plotting import ( _get_ax, @@ -20,8 +21,12 @@ def _set_scale(im, scale, scale_type, color_coords): """Set the scale of the color bar based on scale and scale_type.""" # check scale parameters - assert scale_type in {"absolute", "relative"} - assert isinstance(scale, float | int) or len(scale) == 2 + if scale_type not in {"absolute", "relative"}: + msg = f"scale_type must be 'absolute' or 'relative', got {scale_type!r}" + raise ParameterError(msg) + if not (isinstance(scale, float | int) or len(scale) == 2): + msg = "scale must be a number or a length-2 sequence" + raise ParameterError(msg) # make sure we have a len two array modifier = 1 if scale_type == "relative": @@ -88,15 +93,21 @@ def map_fiber( """ dims = [] if isinstance(x, str): - assert x in patch.coords, f"{x} not found in patch coordinates" + if x not in patch.coords: + msg = f"{x} not found in patch coordinates" + raise ParameterError(msg) dims.append(x) x = patch.coords.get_array(x) if isinstance(y, str): - assert y in patch.coords, f"{y} not found in patch coordinates" + if y not in patch.coords: + msg = f"{y} not found in patch coordinates" + raise ParameterError(msg) dims.append(y) y = patch.coords.get_array(y) if isinstance(color, str): - assert color in patch.coords, f"{color} not found in patch coordinates" + if color not in patch.coords: + msg = f"{color} not found in patch coordinates" + raise ParameterError(msg) data_type = color data_units = patch.attrs.coords[color].units color = patch.coords.get_array(color) diff --git a/tests/test_core/test_coords.py b/tests/test_core/test_coords.py index 9e8717424..9031795c6 100644 --- a/tests/test_core/test_coords.py +++ b/tests/test_core/test_coords.py @@ -2078,3 +2078,49 @@ def test_2d_date_time(self, evenly_sampled_date_coord): data = np.stack([dates, dates], axis=-1) coord = get_coord(data=data) assert isinstance(coord, CoordArray) + + +class TestDimensionalityErrors: + """ + Operations that only support 1D coordinates should raise CoordError on + multi-dim input rather than a bare AssertionError (which is also stripped + under `python -O`). + """ + + @pytest.fixture(scope="class") + def coord_2d(self): + """A simple 2D (non-dimensional) coordinate.""" + return get_coord(values=np.arange(12).reshape(3, 4)) + + @pytest.fixture(scope="class") + def partial_2d(self): + """A 2D partial coordinate.""" + coord = get_coord(start=0, step=1, shape=(2, 3)) + assert isinstance(coord, CoordPartial) and coord.ndim == 2 + return coord + + def test_select_sample_array_2d_raises(self, coord_2d): + """Selecting with a sample array requires a 1D coord.""" + with pytest.raises(CoordError, match="1D coords"): + coord_2d.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"): + coord_2d.get_sample_count(2) + + def test_align_to_2d_raises(self, coord_2d): + """align_to requires 1D coords.""" + other = get_coord(values=np.arange(6).reshape(2, 3)) + with pytest.raises(CoordError, match="1D coords"): + coord_2d.align_to(other) + + def test_change_length_2d_partial_raises(self, partial_2d): + """change_length requires a 1D coord.""" + with pytest.raises(CoordError, match="1D coords"): + partial_2d.change_length(5) + + def test_coord_range_requires_1d_shape(self): + """Constructing a CoordRange with a 2D shape must be rejected.""" + with pytest.raises(ValidationError, match="only works for 1D coords"): + CoordRange(start=0, step=1, shape=(2, 3)) diff --git a/tests/test_io/test_wav/test_wav.py b/tests/test_io/test_wav/test_wav.py index 1366a6f1c..0c702bd36 100644 --- a/tests/test_io/test_wav/test_wav.py +++ b/tests/test_io/test_wav/test_wav.py @@ -9,6 +9,7 @@ import dascore as dc from dascore.constants import ONE_SECOND +from dascore.exceptions import ParameterError class TestWriteWav: @@ -68,3 +69,10 @@ def test_write_non_distance_dims( # Verify content of first file sr, data = read_wav(str(wavs[0])) assert sr == int(ONE_SECOND / patch.get_coord("time").step) + + def test_multi_patch_spool_raises(self, audio_patch, tmp_path_factory): + """Writing a spool with more than one patch to wav should raise.""" + path = tmp_path_factory.mktemp("wave_multi") / "temp.wav" + spool = dc.spool([audio_patch, audio_patch]) + with pytest.raises(ParameterError, match="single patch spools"): + dc.write(spool, path, "wav") diff --git a/tests/test_proc/test_correlate.py b/tests/test_proc/test_correlate.py index 16618d05a..7d4dd1f04 100644 --- a/tests/test_proc/test_correlate.py +++ b/tests/test_proc/test_correlate.py @@ -4,7 +4,7 @@ import pytest import dascore as dc -from dascore.exceptions import UnitError +from dascore.exceptions import ParameterError, UnitError from dascore.units import m from dascore.utils.time import to_float @@ -186,3 +186,14 @@ def test_lag_deprecated(self, corr_patch): """Ensure the lag parameter is deprecated.""" with pytest.warns(DeprecationWarning): corr_patch.correlate(time=1, lag=10, samples=True) + + +class TestCorrelateErrors: + """Tests for correlate input validation.""" + + def test_non_2d_patch_raises(self): + """Correlate requires a 2D patch.""" + coord = dc.get_coord(start=0, step=1, stop=10) + patch_1d = dc.Patch(np.arange(10.0), coords={"time": coord}, dims=("time",)) + with pytest.raises(ParameterError, match="2D patch"): + patch_1d.correlate(time=0, samples=True) diff --git a/tests/test_proc/test_detrend.py b/tests/test_proc/test_detrend.py index b32d5596f..7ff3233d3 100644 --- a/tests/test_proc/test_detrend.py +++ b/tests/test_proc/test_detrend.py @@ -3,6 +3,9 @@ from __future__ import annotations import numpy as np +import pytest + +from dascore.exceptions import ParameterError class TestDetrend: @@ -15,3 +18,8 @@ def test_detrend(self, random_patch): det = new.detrend(dim="time", type="linear") means = np.mean(det.data, axis=det.get_axis("time")) assert np.allclose(means, 0) + + def test_bad_dim_raises(self, random_patch): + """A dim not in the patch should raise ParameterError.""" + with pytest.raises(ParameterError, match="not in patch dim"): + random_patch.detrend(dim="not_a_dim") diff --git a/tests/test_proc/test_taper.py b/tests/test_proc/test_taper.py index 62ff61527..c6153cba8 100644 --- a/tests/test_proc/test_taper.py +++ b/tests/test_proc/test_taper.py @@ -314,3 +314,12 @@ def test_two_value_invert_mutes_to_last_sample(self, patch_ones): # Values from start_idx to end should be 0 (muted) assert np.allclose(out.data[start_idx, :], 1) assert np.allclose(out.data[end_idx - 1, :], 1) + + +class TestTaperErrors: + """Tests for taper input validation.""" + + def test_non_length_2_sequence_raises(self, random_patch): + """A sequence taper value must have exactly two entries.""" + with pytest.raises(ParameterError, match="Length 2 sequence"): + random_patch.taper(time=(0.1, 0.2, 0.3)) diff --git a/tests/test_viz/test_map_fiber.py b/tests/test_viz/test_map_fiber.py index e678231dc..f8e5e4bab 100644 --- a/tests/test_viz/test_map_fiber.py +++ b/tests/test_viz/test_map_fiber.py @@ -6,6 +6,7 @@ import pytest import dascore as dc +from dascore.exceptions import ParameterError from dascore.utils.time import is_datetime64 @@ -116,3 +117,32 @@ def test_show(self, random_patch, monkeypatch): """Ensure show path is callable.""" monkeypatch.setattr(plt, "show", lambda: None) random_patch.viz.map_fiber(show=True) + + +class TestMapFiberErrors: + """Tests for map_fiber input validation.""" + + def test_bad_x_coord(self, random_patch): + """A non-existent x coordinate name should raise.""" + with pytest.raises(ParameterError, match="not found in patch"): + random_patch.viz.map_fiber("not_a_coord", "time") + + def test_bad_y_coord(self, random_patch): + """A non-existent y coordinate name should raise.""" + with pytest.raises(ParameterError, match="not found in patch"): + random_patch.viz.map_fiber("distance", "not_a_coord") + + def test_bad_color_coord(self, random_patch): + """A non-existent color coordinate name should raise.""" + with pytest.raises(ParameterError, match="not found in patch"): + random_patch.viz.map_fiber("distance", "time", "not_a_coord") + + def test_bad_scale_type(self, random_patch): + """An unknown scale_type should raise.""" + with pytest.raises(ParameterError, match="scale_type"): + random_patch.viz.map_fiber(scale_type="nope", scale=10) + + def test_bad_scale_length(self, random_patch): + """A scale that is neither a number nor a length-2 sequence should raise.""" + with pytest.raises(ParameterError, match="scale must be"): + random_patch.viz.map_fiber(scale=(1, 2, 3)) From 98f73a82e966d47bf19f7cf6c782b9265eba95e6 Mon Sep 17 00:00:00 2001 From: Andreas Wuestefeld <115324323+andreas-wuestefeld@users.noreply.github.com> Date: Fri, 17 Jul 2026 18:47:52 +0200 Subject: [PATCH 3/7] Fix/fbe decibel (#755) * fixed decibel scaling factore to 20 (was 10) * fixed test to match new decibel factor --- dascore/transform/fbe.py | 5 +++-- tests/test_transform/test_fbe.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/dascore/transform/fbe.py b/dascore/transform/fbe.py index 439c1c6d1..401bfbd34 100644 --- a/dascore/transform/fbe.py +++ b/dascore/transform/fbe.py @@ -41,7 +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 + Return patch data in decibel [dB] instead of orginal 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 @@ -92,7 +93,7 @@ def fbe( ) if db: - fbe = (10 * fbe.log10()).update( + fbe = (20 * fbe.log10()).update( attrs={"data_type": "frequency_band_energy", "data_units": "dB"} ) diff --git a/tests/test_transform/test_fbe.py b/tests/test_transform/test_fbe.py index c422184f0..cc3785452 100644 --- a/tests/test_transform/test_fbe.py +++ b/tests/test_transform/test_fbe.py @@ -45,7 +45,7 @@ def test_db_true_matches_expected_db(self, random_patch): filtered = random_patch.pass_filter(**kwargs) rms = (filtered**2).rolling(time=0.01, step=0.01).mean() ** 0.5 - expected = 10 * rms.log10() + expected = 20 * rms.log10() assert np.allclose(out.data, expected.data, equal_nan=True) From 28640327c84027a575dc5dcfb2c23e125047cca6 Mon Sep 17 00:00:00 2001 From: Andreas Wuestefeld <115324323+andreas-wuestefeld@users.noreply.github.com> Date: Sun, 19 Jul 2026 22:45:20 +0200 Subject: [PATCH 4/7] implemented gap-sensistive waterfall plot (#753) * implemented gap-sensistive waterfall plot * added 3 more tests * refactor gap_detection and mesh-coordinates; handle datetime64 natively --- dascore/utils/gaps.py | 104 +++++++++++++++++++++++ dascore/viz/waterfall.py | 119 ++++++++++++++++++++++++--- tests/test_utils/test_gaps.py | 83 +++++++++++++++++++ tests/test_viz/test_waterfall.py | 137 +++++++++++++++++++++++++++++++ 4 files changed, 430 insertions(+), 13 deletions(-) create mode 100644 dascore/utils/gaps.py create mode 100644 tests/test_utils/test_gaps.py diff --git a/dascore/utils/gaps.py b/dascore/utils/gaps.py new file mode 100644 index 000000000..a78a5e1a7 --- /dev/null +++ b/dascore/utils/gaps.py @@ -0,0 +1,104 @@ +"""Utilities for detecting coordinate gaps and constructing cell edges.""" + +from __future__ import annotations + +import warnings + +import numpy as np + +from dascore.utils.time import is_datetime64, is_timedelta64, to_float + + +def _to_numeric(values): + """Convert timedeltas to seconds while retaining other numeric values.""" + values = np.asarray(values) + return to_float(values) if is_timedelta64(values) else values + + +def _normalize_coord_values(values): + """Normalize coordinate values for gap and edge calculations.""" + values = np.asarray(values) + if is_datetime64(values): + return values.astype("datetime64[ns]") + return _to_numeric(values) + + +def is_monotonic_and_finite(values) -> bool: + """Return True when values are finite and strictly monotonic.""" + values = _normalize_coord_values(values) + if not np.all(np.isfinite(values)): + return False + diffs = _to_numeric(np.diff(values)) + return bool(not len(diffs) or np.all(diffs > 0) or np.all(diffs < 0)) + + +def get_gap_edges(values, gap_factor: float | None = None): + """ + Return cell edges and coordinate gap locations. + + Timedelta coordinates are converted to seconds. Datetime coordinates are + retained so plotting libraries with datetime support can convert them. + When ``gap_factor`` is None, adjacent cell edges meet halfway between + coordinate centers. Otherwise, intervals larger than ``gap_factor`` times + the median interval are expanded into a gap. + + Parameters + ---------- + values + One-dimensional, monotonic coordinate centers. + gap_factor + Factor of the median interval above which an interval is a gap. If + None, no intervals are considered gaps. + + Returns + ------- + tuple[np.ndarray, np.ndarray] + Cell edges and a Boolean array marking gaps after each input value. + """ + values = _normalize_coord_values(values) + if len(values) == 1: + msg = "Singleton coordinate has no inferred cell width; using a default width." + warnings.warn(msg, UserWarning, stacklevel=2) + if is_datetime64(values): + step = np.asarray(np.timedelta64(1, "D")).astype("timedelta64[ns]")[()] + else: + step = 1 + return np.asarray([values[0] - step / 2, values[0] + step / 2]), np.zeros( + 0, dtype=bool + ) + + diffs = np.diff(values) + numeric_diffs = _to_numeric(diffs) + gap_mask = np.zeros(len(diffs), dtype=bool) + if gap_factor is not None: + representative_interval = np.median(np.abs(numeric_diffs)) + gap_mask = np.abs(numeric_diffs) > representative_interval * gap_factor + + if not np.any(gap_mask): + edges = np.concatenate( + ( + [values[0] - diffs[0] / 2], + values[:-1] + diffs / 2, + [values[-1] + diffs[-1] / 2], + ) + ) + return edges, gap_mask + + representative_step = np.median(np.abs(diffs)) + direction = 1 if numeric_diffs[0] > 0 else -1 + signed_step = direction * representative_step + first_step = signed_step if gap_mask[0] else diffs[0] + last_step = signed_step if gap_mask[-1] else diffs[-1] + edges = [values[0] - first_step / 2] + for index, diff in enumerate(diffs): + if gap_mask[index]: + edges.extend( + [ + values[index] + signed_step / 2, + values[index + 1] - signed_step / 2, + ] + ) + else: + edges.append(values[index] + diff / 2) + edges.append(values[-1] + last_step / 2) + return np.asarray(edges), gap_mask diff --git a/dascore/viz/waterfall.py b/dascore/viz/waterfall.py index 7e137abee..bf1196527 100644 --- a/dascore/viz/waterfall.py +++ b/dascore/viz/waterfall.py @@ -12,6 +12,7 @@ from dascore.constants import DEFAULT_COLORMAPS, PatchType from dascore.exceptions import ParameterError from dascore.units import get_quantity_str, maybe_convert_percent_to_fraction +from dascore.utils.gaps import get_gap_edges, is_monotonic_and_finite from dascore.utils.misc import tukey_fence from dascore.utils.patch import patch_function from dascore.utils.plotting import ( @@ -28,7 +29,14 @@ def _validate_scale_type(scale_type): """Validate that scale_type is either 'relative' or 'absolute'.""" valid_types = {"absolute", "relative"} if scale_type not in valid_types: - msg = f"scale_type must be one of {valid_types}, " f"but got '{scale_type}'" + msg = f"scale_type must be one of {valid_types}, but got '{scale_type}'" + raise ParameterError(msg) + + +def _validate_gap_factor(gap_factor): + """Validate the factor used to identify coordinate gaps.""" + if not np.isfinite(gap_factor) or gap_factor <= 1: + msg = "gap_factor must be a finite number greater than 1" raise ParameterError(msg) @@ -157,6 +165,49 @@ def _get_waterfall_colormap(patch, cmap=None): return _get_cmap(cmap) +def _insert_gap_bands(data, gap_mask, axis): + """Insert masked bands into an array at each coordinate gap.""" + if not np.any(gap_mask): + return data + old_size = data.shape[axis] + new_size = old_size + np.count_nonzero(gap_mask) + new_shape = list(data.shape) + new_shape[axis] = new_size + out = np.ma.masked_all(new_shape, dtype=data.dtype) + new_indices = np.arange(old_size) + np.cumsum( + np.concatenate(([0], gap_mask.astype(int))) + ) + indexer = [slice(None)] * data.ndim + indexer[axis] = new_indices + out[tuple(indexer)] = data + return out + + +def _plot_with_mesh(ax, data, dims, coords, cmap, gap_color, gap_factor): + """Plot irregularly sampled data using a quadrilateral mesh.""" + mesh_data = np.ma.asarray(data) + edges = {} + mesh_gap_factor = gap_factor if gap_color is not None else None + for axis, dim in enumerate(dims): + dim_edges, gap_mask = get_gap_edges(coords[dim], mesh_gap_factor) + if gap_color is not None: + mesh_data = _insert_gap_bands(mesh_data, gap_mask, axis) + edges[dim] = dim_edges + + if gap_color is not None: + cmap = cmap.with_extremes(bad=gap_color) + return ax.pcolormesh( + edges[dims[1]], + edges[dims[0]], + mesh_data, + cmap=cmap, + shading="flat", + edgecolors="none", + linewidth=0, + antialiased=False, + ) + + @patch_function() def waterfall( patch: PatchType, @@ -166,6 +217,8 @@ def waterfall( scale_type: Literal["relative", "absolute"] = "relative", interpolation: str | None = "antialiased", interpolation_stage: str = "auto", + gap_color: str | Sequence[float] | None = None, + gap_factor: float = 1.5, log: bool = False, cbar: bool = True, show: bool = False, @@ -173,6 +226,12 @@ def waterfall( """ Create a waterfall plot of the Patch data. + Evenly sampled dimension coordinates are rendered with ``imshow`` for + efficient display and image interpolation. Finite, monotonic irregular + coordinates are rendered with ``pcolormesh`` so cell geometry follows the + coordinate values. Incomplete or nonmonotonic coordinates fall back to + ``imshow`` with index-based or minimum/maximum extents. + Parameters ---------- patch @@ -199,12 +258,33 @@ def waterfall( which is relevant for DAS. Usually, "antialiased" works well, but if the data look smeared disabling interpolation with None might help. Other options are available, see matplotlib's documentation for more details. + This option does not apply when irregular coordinates select the + ``pcolormesh`` renderer. interpolation_stage If 'data', interpolation is carried out on the data provided by the user. If 'rgba', the interpolation is carried out after the colormapping has been applied (visual interpolation). 'auto' (default) selects a suitable interpolation stage automatically. - See matplotlib's imshow documentation for more details. + See matplotlib's imshow documentation for more details. This option + does not apply when ``pcolormesh`` is used. + gap_color + Matplotlib color used to display gaps in irregular dimension + coordinates. When a color is provided, a masked row or column is + inserted for each detected gap and displayed with this color. The + default of None bridges gaps by extending adjacent cells across them + without expanding the data matrix. This option only applies when + ``pcolormesh`` is used. Existing masked or NaN data receive the same + color as coordinate gaps. + gap_factor + When ``gap_color`` is provided, coordinate intervals larger than this + factor times the median interval are displayed as gaps. With the + default ``gap_color=None``, cells bridge intervals and this parameter + has no visual effect. Gap detection assumes the median interval + represents the sampling interval, so coordinates containing contiguous + regions with different sampling rates may classify the more coarsely + sampled region as gaps. For such data, use the default + ``gap_color=None``, increase ``gap_factor``, or plot/resample the + regions separately. Must be greater than 1. log If True, visualize the common logarithm of the absolute values of patch data. To avoid log(0), the abs(array) is cast to float64 and a small value @@ -272,6 +352,7 @@ def waterfall( """ # Validate inputs patch = _validate_patch_dims(patch) + _validate_gap_factor(gap_factor) # Setup axes and data ax = _get_ax(ax) if log: @@ -280,20 +361,32 @@ def waterfall( data = patch.data dims = patch.dims dims_r = tuple(reversed(dims)) - coords = {dim: patch.coords.get_array(dim) for dim in dims} - # Plot using imshow and set colorbar limits - extents = _get_extents(dims_r, coords) + dim_coords = {dim: patch.get_coord(dim) for dim in dims} + coords = {dim: np.asarray(coord) for dim, coord in dim_coords.items()} cmap = _get_waterfall_colormap(patch, cmap) scale = _get_scale(scale, scale_type, data) - with mpl.rc_context({"image.resample": True}): - im = ax.imshow( + use_image = all(coord.evenly_sampled for coord in dim_coords.values()) + if use_image or not all(is_monotonic_and_finite(x) for x in coords.values()): + extents = _get_extents(dims_r, coords) + with mpl.rc_context({"image.resample": True}): + im = ax.imshow( + data, + extent=extents, + aspect="auto", + cmap=cmap, + origin="lower", + interpolation=interpolation, + interpolation_stage=interpolation_stage, + ) + else: + im = _plot_with_mesh( + ax, data, - extent=extents, - aspect="auto", - cmap=cmap, - origin="lower", - interpolation=interpolation, - interpolation_stage=interpolation_stage, + dims, + coords, + cmap, + gap_color=gap_color, + gap_factor=gap_factor, ) if scale is not None and len(scale) == 2 and np.all(np.isfinite(scale)): im.set_clim(np.asarray(scale)) diff --git a/tests/test_utils/test_gaps.py b/tests/test_utils/test_gaps.py new file mode 100644 index 000000000..bda3b0445 --- /dev/null +++ b/tests/test_utils/test_gaps.py @@ -0,0 +1,83 @@ +"""Tests for coordinate gap utilities.""" + +from __future__ import annotations + +import numpy as np +import pytest + +from dascore.utils.gaps import get_gap_edges, is_monotonic_and_finite + + +class TestGetGapEdges: + """Tests for constructing coordinate cell edges.""" + + def test_numeric_without_gaps(self): + """Numeric coordinates produce midpoint cell edges.""" + edges, gaps = get_gap_edges([0, 1, 2]) + np.testing.assert_allclose(edges, [-0.5, 0.5, 1.5, 2.5]) + assert not np.any(gaps) + + def test_numeric_with_gap(self): + """Large numeric intervals are expanded into a gap.""" + edges, gaps = get_gap_edges([0, 1, 5, 6], gap_factor=1.5) + np.testing.assert_allclose(edges, [-0.5, 0.5, 1.5, 4.5, 5.5, 6.5]) + np.testing.assert_array_equal(gaps, [False, True, False]) + + def test_timedelta(self): + """Timedeltas are converted to seconds before constructing edges.""" + values = np.array([0, 1, 2], dtype="timedelta64[s]") + edges, gaps = get_gap_edges(values) + np.testing.assert_allclose(edges, [-0.5, 0.5, 1.5, 2.5]) + assert not np.any(gaps) + + def test_datetime(self): + """Datetime edges retain datetime dtype and support gaps.""" + values = np.array( + ["2020-01-01", "2020-01-02", "2020-01-06"], + dtype="datetime64[D]", + ) + edges, gaps = get_gap_edges(values, gap_factor=1.5) + expected = np.array( + [ + "2019-12-31T12:00:00", + "2020-01-01T12:00:00", + "2020-01-03T06:00:00", + "2020-01-04T18:00:00", + "2020-01-07T06:00:00", + ], + dtype="datetime64[ns]", + ) + assert np.issubdtype(edges.dtype, np.datetime64) + np.testing.assert_array_equal(edges, expected) + np.testing.assert_array_equal(gaps, [False, True]) + + def test_singleton_datetime(self): + """Singleton datetimes warn and receive a one-day default cell width.""" + with pytest.warns(UserWarning, match="Singleton coordinate"): + edges, gaps = get_gap_edges(np.array(["2020-01-01"], dtype="datetime64[D]")) + expected = np.array( + ["2019-12-31T12:00:00", "2020-01-01T12:00:00"], dtype="datetime64[ns]" + ) + np.testing.assert_array_equal(edges, expected) + assert not len(gaps) + + def test_singleton_numeric(self): + """Singleton numeric coordinates warn and receive a unit cell width.""" + with pytest.warns(UserWarning, match="Singleton coordinate"): + edges, gaps = get_gap_edges([10]) + np.testing.assert_allclose(edges, [9.5, 10.5]) + assert not len(gaps) + + +class TestIsMonotonicAndFinite: + """Tests for validating mesh coordinate centers.""" + + @pytest.mark.parametrize("values", [[0, 1, 2], [2, 1, 0]]) + def test_valid(self, values): + """Ascending and descending finite values are valid.""" + assert is_monotonic_and_finite(values) + + @pytest.mark.parametrize("values", [[0, 2, 1], [0, np.nan, 2]]) + def test_invalid(self, values): + """Nonmonotonic and nonfinite values are invalid.""" + assert not is_monotonic_and_finite(values) diff --git a/tests/test_viz/test_waterfall.py b/tests/test_viz/test_waterfall.py index 100556f44..4f3a3378a 100644 --- a/tests/test_viz/test_waterfall.py +++ b/tests/test_viz/test_waterfall.py @@ -5,6 +5,8 @@ import matplotlib.pyplot as plt import numpy as np import pytest +from matplotlib.collections import QuadMesh +from matplotlib.image import AxesImage import dascore as dc from dascore.exceptions import ParameterError @@ -65,6 +67,141 @@ def timedelta_patch(self, random_patch): new_time = to_timedelta64(np.arange(len(old_coord))) return random_patch.update_coords(time=new_time) + @pytest.fixture() + def distance_gap_patch(self, random_patch): + """Create a patch with one large gap in its distance coordinate.""" + coord = random_patch.get_coord("distance") + values = np.asarray(coord).copy() + split = len(values) // 2 + values[split:] += coord.step * 10 + return random_patch.update_coords(distance=values), split + + @pytest.fixture() + def time_gap_patch(self, random_patch): + """Create a patch with one large gap in its time coordinate.""" + coord = random_patch.get_coord("time") + values = np.asarray(coord).copy() + split = len(values) // 2 + values[split:] += coord.step * 10 + return random_patch.update_coords(time=values), split + + def test_even_coordinates_use_image(self, random_patch): + """Evenly sampled coordinates retain the fast image renderer.""" + ax = random_patch.viz.waterfall(cbar=False) + assert isinstance(ax.images[0], AxesImage) + assert not any(isinstance(x, QuadMesh) for x in ax.collections) + + def test_irregular_timedelta_coordinates_use_mesh(self, timedelta_patch): + """Irregular timedelta coordinates are converted to seconds for meshes.""" + values = np.asarray(timedelta_patch.get_coord("time")).copy() + split = len(values) // 2 + values[split:] += np.timedelta64(10, "s") + patch = timedelta_patch.update_coords(time=values) + ax = patch.viz.waterfall(cbar=False) + mesh = ax.collections[0] + assert isinstance(mesh, QuadMesh) + assert np.all(np.isfinite(mesh.get_coordinates())) + + def test_singleton_irregular_coordinate_uses_mesh(self, random_patch): + """A singleton irregular coordinate receives finite cell edges.""" + patch = random_patch.select(distance=0, samples=True) + distance = np.asarray(patch.get_coord("distance")) + patch = patch.update_coords(distance=distance) + assert not patch.get_coord("distance").evenly_sampled + with pytest.warns(UserWarning, match="Singleton coordinate"): + ax = patch.viz.waterfall(cbar=False) + mesh = ax.collections[0] + assert isinstance(mesh, QuadMesh) + assert mesh.get_coordinates().shape[:2] == tuple(x + 1 for x in patch.shape) + + def test_nonmonotonic_coordinate_uses_image(self, random_patch): + """Nonmonotonic coordinates retain the image-rendering fallback.""" + distance = np.asarray(random_patch.get_coord("distance")).copy() + distance[[1, 2]] = distance[[2, 1]] + patch = random_patch.update_coords(distance=distance) + ax = patch.viz.waterfall(cbar=False) + assert isinstance(ax.images[0], AxesImage) + assert not any(isinstance(x, QuadMesh) for x in ax.collections) + + def test_gap_uses_masked_mesh(self, distance_gap_patch): + """A gap color adds one masked mesh band.""" + patch, split = distance_gap_patch + ax = patch.viz.waterfall(gap_color="white", cbar=False) + mesh = ax.collections[0] + array = mesh.get_array() + mask = np.ma.getmaskarray(array) + assert isinstance(mesh, QuadMesh) + assert not ax.images + assert array.shape == (patch.shape[0] + 1, patch.shape[1]) + assert np.all(mask[split, :]) + assert mesh.get_coordinates().shape[:2] == ( + patch.shape[0] + 2, + patch.shape[1] + 1, + ) + assert np.allclose(mesh.cmap.get_bad(), [1, 1, 1, 1]) + + def test_gap_mesh_colorbar_and_scale(self, distance_gap_patch): + """Mesh plots retain waterfall colorbar and scaling behavior.""" + patch, _ = distance_gap_patch + ax = patch.viz.waterfall( + scale=(-1, 1), scale_type="absolute", gap_color="white" + ) + mesh = ax.collections[0] + assert mesh.colorbar is not None + assert mesh.get_clim() == (-1, 1) + + def test_bridge_gap_doesnt_expand_data(self, distance_gap_patch): + """The default extends cells across a gap without adding a band.""" + patch, _ = distance_gap_patch + ax = patch.viz.waterfall(cbar=False) + mesh = ax.collections[0] + assert mesh.get_array().shape == patch.shape + assert mesh.get_coordinates().shape[:2] == tuple(x + 1 for x in patch.shape) + + def test_gap_color(self, distance_gap_patch): + """A specified gap color is assigned to masked mesh cells.""" + patch, _ = distance_gap_patch + ax = patch.viz.waterfall(gap_color="white", cbar=False) + assert np.allclose(ax.collections[0].cmap.get_bad(), [1, 1, 1, 1]) + + def test_gaps_in_both_axes(self, distance_gap_patch): + """Gap bands can be inserted along both dimensions.""" + patch, distance_split = distance_gap_patch + time = patch.get_coord("time") + values = np.asarray(time).copy() + time_split = len(values) // 2 + values[time_split:] += time.step * 10 + patch = patch.update_coords(time=values) + ax = patch.viz.waterfall(gap_color="white", cbar=False) + mesh_array = ax.collections[0].get_array() + mask = np.ma.getmaskarray(mesh_array) + assert mesh_array.shape == tuple(x + 1 for x in patch.shape) + assert np.all(mask[distance_split, :]) + assert np.all(mask[:, time_split]) + + def test_time_gap_keeps_regular_ticks(self, time_gap_patch): + """Time-axis ticks remain monotonic and evenly spaced across gaps.""" + patch, split = time_gap_patch + ax = patch.viz.waterfall(gap_color="white", cbar=False) + mask = np.ma.getmaskarray(ax.collections[0].get_array()) + ax.get_figure().canvas.draw() + tick_diffs = np.diff(ax.get_xticks()) + assert np.all(mask[:, split]) + assert np.all(tick_diffs > 0) + assert np.allclose(tick_diffs, tick_diffs[0]) + + @pytest.mark.parametrize( + ("kwargs", "match"), + [ + ({"gap_factor": 1}, "gap_factor"), + ({"gap_factor": np.inf}, "gap_factor"), + ], + ) + def test_bad_gap_options_raise(self, random_patch, kwargs, match): + """Invalid gap display options raise an informative error.""" + with pytest.raises(ParameterError, match=match): + random_patch.viz.waterfall(**kwargs) + def test_returns_axes(self, random_patch): """Call waterfall plot, return.""" # modify patch to include line at start From af83f767ae1a2c1f1ccb9d4a3d00f1ffb38b95e3 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 23 Jul 2026 10:39:49 +0200 Subject: [PATCH 5/7] Add no-op fast paths for transpose/squeeze and idempotent coordinate snapping (#765) --- benchmarks/test_patch_benchmarks.py | 18 +++++++ dascore/core/coordmanager.py | 22 ++++++-- dascore/proc/coords.py | 12 +++++ tests/test_proc/test_proc_coords.py | 78 +++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+), 3 deletions(-) diff --git a/benchmarks/test_patch_benchmarks.py b/benchmarks/test_patch_benchmarks.py index 842f10364..70c03f40f 100644 --- a/benchmarks/test_patch_benchmarks.py +++ b/benchmarks/test_patch_benchmarks.py @@ -113,6 +113,18 @@ def test_transpose(self, example_patch): dims = patch.dims[::-1] patch.transpose(*dims) + @pytest.mark.benchmark + def test_transpose_noop(self, example_patch): + """Time a no-op transpose (same dimension order).""" + patch = example_patch + patch.transpose(*patch.dims) + + @pytest.mark.benchmark + def test_squeeze_noop(self, example_patch): + """Time a no-op squeeze (no length-1 dimensions).""" + patch = example_patch + patch.squeeze() + @pytest.mark.benchmark def test_roll(self, example_patch): """Time roll/shift operations.""" @@ -125,6 +137,12 @@ def test_snap_coords(self, patch_uneven_time): patch = patch_uneven_time patch.snap_coords("time") + @pytest.mark.benchmark + def test_snap_coords_already_even(self, example_patch): + """Time snapping an already even/sorted patch (no-op fast path).""" + patch = example_patch + patch.snap_coords("time", "distance") + @pytest.mark.benchmark def test_hampel_filter_non_approximate(self, example_patch): """Time the Hampel filter.""" diff --git a/dascore/core/coordmanager.py b/dascore/core/coordmanager.py index 040f4d768..141a6011b 100644 --- a/dascore/core/coordmanager.py +++ b/dascore/core/coordmanager.py @@ -418,6 +418,9 @@ def _sort_related(name, dim, indexer, new_coords): cmap = self.coord_map assert set(coords).issubset(set(cmap)) coords2sort = {x: cmap[x] for x in coords if not getattr(cmap[x], attr)} + # Nothing to sort; return self to avoid rebuilding an identical manager. + if not coords2sort: + return self, array new_coords, indexers = _get_dimensional_sorts(coords2sort) if array is not None: for index in indexers: @@ -447,10 +450,17 @@ def snap( coords = self.dims if len(coords) == 0 else coords cm, array = self.sort(*coords, array=array, reverse=reverse) # now the arrays are sorted it should be correct to snap dimensions. - cmap = dict(cm.coord_map) + # Only collect coords whose snap actually changes them so an already + # even manager is returned unchanged (snap returns self when even). + updates = {} for coord_name in coords: - cmap[coord_name] = cmap[coord_name].snap() - out = cm.new(coord_map=cmap) + current = cm.coord_map[coord_name] + snapped = current.snap() + if snapped is not current: + updates[coord_name] = snapped + if not updates: + return cm, array + out = cm.new(coord_map={**cm.coord_map, **updates}) assert out.shape == self.shape return out, array @@ -898,6 +908,9 @@ def _get_transpose_dims(new, old): return tuple(new_list) dims = _get_transpose_dims(new=dims or self.dims[::-1], old=self.dims) + # No-op transpose; return self rather than rebuilding an equal manager. + if dims == self.dims: + return self return self.new(dims=dims) def rename_coord(self, **kwargs) -> Self: @@ -966,6 +979,9 @@ def squeeze(self, dim: Sequence[str] | None = None) -> Self: msg = f"cant squeeze dim {name} because it has non-zero length" raise CoordError(msg) to_drop.append(name) + # Nothing to squeeze; return self rather than rebuilding an equal manager. + if not to_drop: + return self return self.drop_coords(*to_drop)[0] def decimate(self, **kwargs) -> tuple[Self, tuple[slice, ...]]: diff --git a/dascore/proc/coords.py b/dascore/proc/coords.py index 7e5a2f117..e64748c18 100644 --- a/dascore/proc/coords.py +++ b/dascore/proc/coords.py @@ -55,6 +55,9 @@ def snap_coords(patch: PatchType, *coords, reverse: bool = False) -> PatchType: >>> dist_snap = patch.snap_coords("distance") """ cman, data = patch.coords.snap(*coords, array=patch.data, reverse=reverse) + # Nothing changed; return the original patch to avoid a rebuild. + if cman is patch.coords and data is patch.data: + return patch return patch.new(data=data, coords=cman) @@ -90,6 +93,9 @@ def sort_coords(patch: PatchType, *coords, reverse: bool = False) -> PatchType: >>> assert dist_snap.coords.coord_map['distance'].reverse_sorted """ cman, data = patch.coords.sort(*coords, array=patch.data, reverse=reverse) + # Nothing changed; return the original patch to avoid a rebuild. + if cman is patch.coords and data is patch.data: + return patch return patch.new(data=data, coords=cman) @@ -617,6 +623,9 @@ def transpose(self: PatchType, *dims: str) -> PatchType: msg = f"Dimension(s) {invalid_list} not found in Patch dimensions: {valid_list}" raise ParameterError(msg) new_coord = self.coords.transpose(*dims) + # No-op transpose; the coord manager returned self, so reuse this patch. + if new_coord is self.coords: + return self new_dims = new_coord.dims axes = tuple(old_dims.index(x) for x in new_dims) new_data = np.transpose(self.data, axes) @@ -713,6 +722,9 @@ def squeeze(self: PatchType, dim=None) -> PatchType: >>> squeezed = single_time.squeeze(dim="time") """ coords = self.coords.squeeze(dim) + # Nothing to squeeze; the coord manager returned self, so reuse this patch. + if coords is self.coords: + return self if dim is None: axes = None else: diff --git a/tests/test_proc/test_proc_coords.py b/tests/test_proc/test_proc_coords.py index 86483a6bd..216700789 100644 --- a/tests/test_proc/test_proc_coords.py +++ b/tests/test_proc/test_proc_coords.py @@ -58,6 +58,14 @@ def test_data_sorted_correctly(self, wacky_dim_patch): data_along_slice = np.take(new.data, 0, ind) assert np.all(np.equal(arg_sort, data_along_slice)) + def test_noop_sort_returns_self(self, random_patch): + """Sorting already-sorted coords should return the same objects.""" + coords = random_patch.coords + new_coords, array = coords.sort(array=random_patch.data) + assert new_coords is coords + assert array is random_patch.data + assert random_patch.sort_coords() is random_patch + class TestSnapDims: """Tests for snapping dimensions.""" @@ -84,6 +92,53 @@ def test_snap_dims(self, wacky_dim_patch): assert coord.sorted assert coord.evenly_sampled + @pytest.fixture(scope="class") + def even_time_uneven_distance_patch(self): + """A patch with an even (CoordRange) time and monotonic-uneven distance.""" + time = dc.to_datetime64(np.arange(20)) + distance = np.cumsum(np.arange(1, 11) ** 1.5) + data = np.arange(len(time) * len(distance)).reshape(len(time), len(distance)) + patch = dc.Patch( + data=data.astype(np.float64), + coords={"time": time, "distance": distance}, + dims=("time", "distance"), + ) + # sanity: time is even, distance is monotonic but not evenly sampled + assert patch.coords.coord_map["time"].evenly_sampled + assert not patch.coords.coord_map["distance"].evenly_sampled + return patch + + def test_snap_already_even_returns_self(self, random_patch): + """Snapping an already-even, sorted patch returns the same objects.""" + coords = random_patch.coords + new_coords, array = coords.snap(array=random_patch.data) + assert new_coords is coords + assert array is random_patch.data + assert random_patch.snap_coords() is random_patch + assert random_patch.snap_coords("time", "distance") is random_patch + + def test_snap_changes_only_uneven_coord(self, even_time_uneven_distance_patch): + """Only the coordinate that must change is replaced; others are reused.""" + patch = even_time_uneven_distance_patch + out = patch.snap_coords() + # distance was uneven, so it should now be evenly sampled + assert out.coords.coord_map["distance"].evenly_sampled + # time was already even; its coordinate object should be reused as-is + assert out.coords.coord_map["time"] is patch.coords.coord_map["time"] + # data is unchanged because nothing needed reordering + assert np.array_equal(out.data, patch.data) + + def test_snap_reverse_sorted(self, even_time_uneven_distance_patch): + """Reverse snapping sorts descending and reorders data accordingly.""" + patch = even_time_uneven_distance_patch + out = patch.snap_coords("distance", reverse=True) + coord = out.coords.coord_map["distance"] + assert coord.reverse_sorted + assert coord.evenly_sampled + # data columns should be reversed relative to the ascending snap + ascending = patch.snap_coords("distance") + assert np.array_equal(out.data, ascending.data[:, ::-1]) + class TestDropCoords: """Tests for dropping coordinates.""" @@ -681,6 +736,16 @@ def test_coord_summary(self, flat_patch): if coords: assert set(coords) == set(patch.coords.coord_map) + def test_noop_squeeze_returns_self(self, random_patch): + """Squeeze on a patch with no length-1 dims returns the same patch.""" + assert 1 not in random_patch.shape + assert random_patch.squeeze() is random_patch + + def test_noop_coord_squeeze_returns_self(self, random_patch): + """CoordManager squeeze with no length-1 dims returns self.""" + coords = random_patch.coords + assert coords.squeeze() is coords + class TestGetCoord: """Tests for the get_coord convenience function.""" @@ -906,3 +971,16 @@ def test_transpose_5d_data_integrity(self, patch_5d): assert patch_5d.data.size == out.data.size # Data values should be the same, just rearranged assert np.array_equal(np.sort(patch_5d.data.flat), np.sort(out.data.flat)) + + def test_noop_transpose_returns_self(self, random_patch): + """Transposing to the current dim order returns the same patch.""" + assert random_patch.transpose(*random_patch.dims) is random_patch + + def test_noop_transpose_ellipsis_returns_self(self, patch_5d): + """A trailing ellipsis that resolves to the same order returns self.""" + assert patch_5d.transpose(*patch_5d.dims[:-1], ...) is patch_5d + + def test_noop_coord_transpose_returns_self(self, random_patch): + """CoordManager transpose to the current order returns self.""" + coords = random_patch.coords + assert coords.transpose(*coords.dims) is coords From 955643d4ca8cc680eaeca4d394066129a8f62c26 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 23 Jul 2026 11:00:53 +0200 Subject: [PATCH 6/7] CI: bump actions off deprecated Node.js 20 runtime (#766) --- .github/actions/mamba-install-dascore/action.yml | 6 +++--- .github/workflows/build_deploy_master_docs.yaml | 2 +- .github/workflows/build_deploy_stable_docs.yaml | 2 +- .github/workflows/get_coverage.yml | 4 ++-- .github/workflows/lint.yml | 2 +- .github/workflows/profile.yml | 2 +- .github/workflows/run_min_dep_tests.yml | 4 ++-- .github/workflows/runtests.yml | 6 +++--- .github/workflows/test_doc_build.yml | 2 +- .github/workflows/upload_pypi.yml | 2 +- 10 files changed, 16 insertions(+), 16 deletions(-) diff --git a/.github/actions/mamba-install-dascore/action.yml b/.github/actions/mamba-install-dascore/action.yml index 1b8879964..d2a849212 100644 --- a/.github/actions/mamba-install-dascore/action.yml +++ b/.github/actions/mamba-install-dascore/action.yml @@ -37,7 +37,7 @@ runs: shell: bash -l {0} run: echo "CURRENT_DATE=$(date '+%Y-%m-%d')" >> $GITHUB_ENV - - uses: mamba-org/setup-micromamba@v2 + - uses: mamba-org/setup-micromamba@v3 with: micromamba-version: '2.0.5-0' # versions: https://github.com/mamba-org/micromamba-releases environment-file: ${{ inputs.environment-file }} @@ -85,7 +85,7 @@ runs: - name: restore test data cache if: "${{ inputs.prepare-test-data == 'true' }}" id: restore-test-data - uses: actions/cache/restore@v4 + uses: actions/cache/restore@v5 with: path: ${{ env.DATA_CACHE_PATH }} key: ${{ env.DATA_CACHE_KEY }} @@ -97,7 +97,7 @@ runs: - name: save test data cache if: "${{ inputs.prepare-test-data == 'true' && steps.restore-test-data.outputs.cache-hit != 'true' }}" - uses: actions/cache/save@v4 + uses: actions/cache/save@v5 with: path: ${{ env.DATA_CACHE_PATH }} key: ${{ env.DATA_CACHE_KEY }} diff --git a/.github/workflows/build_deploy_master_docs.yaml b/.github/workflows/build_deploy_master_docs.yaml index 7feed3ba5..95fbc9c07 100644 --- a/.github/workflows/build_deploy_master_docs.yaml +++ b/.github/workflows/build_deploy_master_docs.yaml @@ -31,7 +31,7 @@ jobs: name: github-pages steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: fetch-tags: "true" fetch-depth: '0' diff --git a/.github/workflows/build_deploy_stable_docs.yaml b/.github/workflows/build_deploy_stable_docs.yaml index 4f8e12054..c423851fd 100644 --- a/.github/workflows/build_deploy_stable_docs.yaml +++ b/.github/workflows/build_deploy_stable_docs.yaml @@ -26,7 +26,7 @@ jobs: build: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: fetch-tags: "true" fetch-depth: '0' diff --git a/.github/workflows/get_coverage.yml b/.github/workflows/get_coverage.yml index e04fd13c4..2f20fba33 100644 --- a/.github/workflows/get_coverage.yml +++ b/.github/workflows/get_coverage.yml @@ -10,7 +10,7 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: fetch-tags: "true" fetch-depth: '0' @@ -29,7 +29,7 @@ jobs: run: | pytest -s --cov dascore --cov-report=xml - - uses: codecov/codecov-action@v4 + - uses: codecov/codecov-action@v5 with: fail_ci_if_error: true files: ./coverage.xml diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index b8c819fed..628528a0f 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -16,7 +16,7 @@ jobs: if: github.event_name == 'push' || !contains(github.event.pull_request.labels.*.name, 'no_ci') steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: fetch-tags: "true" fetch-depth: '0' diff --git a/.github/workflows/profile.yml b/.github/workflows/profile.yml index 1964c8c32..542ac8fb9 100644 --- a/.github/workflows/profile.yml +++ b/.github/workflows/profile.yml @@ -27,7 +27,7 @@ jobs: contains(github.event.pull_request.labels.*.name, 'benchmark') runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: fetch-tags: "true" fetch-depth: '0' diff --git a/.github/workflows/run_min_dep_tests.yml b/.github/workflows/run_min_dep_tests.yml index e8f823ae3..3416b2a77 100644 --- a/.github/workflows/run_min_dep_tests.yml +++ b/.github/workflows/run_min_dep_tests.yml @@ -33,7 +33,7 @@ jobs: # Shared values live in .github/actions/load-shared-vars/action.yml python-matrix: ${{ steps.load-vars.outputs.python-min-deps-matrix }} steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 - uses: ./.github/actions/load-shared-vars id: load-vars @@ -51,7 +51,7 @@ jobs: if: github.event_name == 'push' || !contains(github.event.pull_request.labels.*.name, 'no_ci') steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: fetch-tags: "true" fetch-depth: '0' diff --git a/.github/workflows/runtests.yml b/.github/workflows/runtests.yml index 885dc4400..492426a8c 100644 --- a/.github/workflows/runtests.yml +++ b/.github/workflows/runtests.yml @@ -35,7 +35,7 @@ jobs: # Shared values live in .github/actions/load-shared-vars/action.yml python-matrix: ${{ steps.load-vars.outputs.python-test-matrix }} steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 - uses: ./.github/actions/load-shared-vars id: load-vars @@ -57,7 +57,7 @@ jobs: env_file: 'environment.yml' steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: fetch-tags: 'true' fetch-depth: '0' @@ -79,7 +79,7 @@ jobs: run: ./.github/test_code.sh doctest # Upload coverage files - - uses: codecov/codecov-action@v4 + - uses: codecov/codecov-action@v5 with: fail_ci_if_error: false files: ./coverage.xml diff --git a/.github/workflows/test_doc_build.yml b/.github/workflows/test_doc_build.yml index eb66cb4bc..45a0e504e 100644 --- a/.github/workflows/test_doc_build.yml +++ b/.github/workflows/test_doc_build.yml @@ -15,7 +15,7 @@ jobs: || (github.event.action == 'synchronize' && contains(github.event.pull_request.labels.*.name, 'documentation')) runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: fetch-tags: "true" fetch-depth: '0' diff --git a/.github/workflows/upload_pypi.yml b/.github/workflows/upload_pypi.yml index 7da3fc8a2..ea64fde7b 100644 --- a/.github/workflows/upload_pypi.yml +++ b/.github/workflows/upload_pypi.yml @@ -9,7 +9,7 @@ jobs: upload: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v5 with: fetch-tags: "true" fetch-depth: '0' From 09aac2f4c1f5e5ac4be26f06a2ede8711edd5167 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 23 Jul 2026 13:02:49 +0200 Subject: [PATCH 7/7] Skip reparsing canonical CoordRange coords in CoordManager update/select (#768) --- benchmarks/test_patch_benchmarks.py | 10 ++++ dascore/core/coordmanager.py | 16 +++++- tests/test_core/test_coordmanager.py | 76 ++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 1 deletion(-) diff --git a/benchmarks/test_patch_benchmarks.py b/benchmarks/test_patch_benchmarks.py index 70c03f40f..748d10f73 100644 --- a/benchmarks/test_patch_benchmarks.py +++ b/benchmarks/test_patch_benchmarks.py @@ -88,6 +88,16 @@ def test_select(self, example_patch): patch.select(time=(t1, None)) patch.select(time=(t1, t2)) + @pytest.mark.benchmark + def test_update_existing_coords(self, example_patch): + """Time update_coords re-passing already-validated BaseCoords.""" + patch = example_patch + coords = patch.coords + patch.update_coords( + time=coords.coord_map["time"], + distance=coords.coord_map["distance"], + ) + @pytest.mark.benchmark def test_sobel_filter(self, example_patch): """Time the Sobel filter.""" diff --git a/dascore/core/coordmanager.py b/dascore/core/coordmanager.py index 141a6011b..87dabbf90 100644 --- a/dascore/core/coordmanager.py +++ b/dascore/core/coordmanager.py @@ -54,7 +54,12 @@ import dascore as dc from dascore.constants import dascore_styles, select_values_description -from dascore.core.coords import BaseCoord, CoordSummary, get_coord +from dascore.core.coords import ( + BaseCoord, + CoordRange, + CoordSummary, + get_coord, +) from dascore.exceptions import ( CoordDataError, CoordError, @@ -1272,6 +1277,15 @@ def _get_coord_dim_map(coords, dims): def _get_coord(coord): """Get a coordinate from various inputs.""" + # A CoordRange is already canonical (it is the evenly-sampled + # representation), so re-parsing it via model_dump -> get_coord is pure + # overhead; return it directly. Other coord types are NOT short-circuited: + # array coords (CoordArray/CoordMonotonicArray) can be left non-canonical + # by slicing (e.g. an evenly spaced subset that should collapse to a + # CoordRange), and a fully-specified CoordPartial should canonicalize to a + # CoordRange -- get_coord performs that inference. + if isinstance(coord, CoordRange): + return coord if hasattr(coord, "model_dump"): coord = coord.model_dump(exclude_defaults=True) if isinstance(coord, Mapping): # input is a dict diff --git a/tests/test_core/test_coordmanager.py b/tests/test_core/test_coordmanager.py index 93e47c663..5fd4d3197 100644 --- a/tests/test_core/test_coordmanager.py +++ b/tests/test_core/test_coordmanager.py @@ -17,6 +17,8 @@ ) from dascore.core.coords import ( BaseCoord, + CoordPartial, + CoordRange, get_coord, ) from dascore.exceptions import ( @@ -1084,6 +1086,80 @@ def test_update_with_units(self, coord_manager): assert dc.get_quantity(new_coord.units) == ft +class TestPreserveBaseCoord: + """Canonical coords should not be reparsed when passed through update/select. + + Only CoordRange (the canonical evenly-sampled representation) is returned + unchanged. Other coord types (CoordPartial, CoordArray, CoordMonotonicArray) + are still re-inferred so a fully-specified partial, or slicing that yields an + even/empty subset, is canonicalized. + """ + + def test_update_preserves_range_coord_identity(self, cm_basic): + """Updating a CoordRange dim with its own coord keeps the same object.""" + for name in cm_basic.dims: + coord = cm_basic.coord_map[name] + assert isinstance(coord, CoordRange) + out = cm_basic.update(**{name: coord}) + assert out.coord_map[name] is coord + + def test_full_partial_canonicalizes_to_range(self, cm_non_coord_dim): + """A fully-specified CoordPartial must still become a CoordRange. + + Regression guard: only CoordRange is short-circuited, so a CoordPartial + that carries complete start/stop/step is re-inferred into a CoordRange + (otherwise value-based selection on it would wrongly raise). + """ + cm = cm_non_coord_dim + assert isinstance(cm.coord_map["time"], CoordPartial) + size = cm.shape[cm.get_axis("time")] + full_partial = CoordPartial(shape=(size,), start=0, stop=size, step=1) + out = cm.update(time=full_partial) + assert isinstance(out.coord_map["time"], CoordRange) + # value-based selection must work on the canonicalized coord. + selected, _ = out.select(time=(0, size - 1)) + assert selected.shape[selected.get_axis("time")] <= size + + def test_update_array_coord_is_equivalent(self, cm_wacky_dims): + """Irregular array coords may be re-parsed but stay value-equal.""" + for name in cm_wacky_dims.dims: + coord = cm_wacky_dims.coord_map[name] + out = cm_wacky_dims.update(**{name: coord}) + assert out.coord_map[name] == coord + assert np.array_equal(out.get_array(name), cm_wacky_dims.get_array(name)) + + def test_select_preserves_untouched_range_coord(self, cm_multidim): + """Selecting distance leaves the independent CoordRange time unchanged.""" + original_time = cm_multidim.coord_map["time"] + assert isinstance(original_time, CoordRange) + new, _ = cm_multidim.select(distance=(100, 400)) + assert new.coord_map["time"] is original_time + # latitude is tied to distance, so it must be re-sliced (new object). + assert new.coord_map["latitude"] is not cm_multidim.coord_map["latitude"] + + def test_select_result_still_correct(self, cm_basic): + """The fast path must not change select results.""" + new, _ = cm_basic.select(distance=(100, 400)) + dist = new.get_array("distance") + full = cm_basic.get_array("distance") + expected = full[(full >= 100) & (full <= 400)] + assert np.array_equal(dist, expected) + + def test_even_subset_of_irregular_coord_canonicalizes(self): + """A sample-selected even subset of an irregular coord becomes a CoordRange. + + Regression guard: array coords must still be re-inferred so slicing that + happens to be evenly sampled is canonicalized rather than left as an + irregular array coordinate. + """ + coord = dc.get_coord(data=np.array([0.0, 1.0, 3.0])) + patch = dc.Patch(data=np.arange(3.0), coords={"x": coord}, dims=("x",)) + out = patch.select(x=(0, 2), samples=True) # indices 0..2 -> [0, 1] + new_coord = out.coords.coord_map["x"] + assert isinstance(new_coord, CoordRange) + assert new_coord.evenly_sampled + + class TestSqueeze: """Tests for squeezing degenerate dimensions."""