From ec6dbe205f441b61002c9db173a8fa1965483320 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 18:29:21 +0200 Subject: [PATCH 1/2] Canonicalize a coordinate only when it keeps its values get_coord_manager re-inferred every non-CoordRange coord through get_coord to canonicalize it, but that evenness test is tolerant (rtol 1e-3), so a coord whose spacing varied by less than a tenth of a percent was replaced by an idealized ramp. The values were rewritten silently and the result claimed to be evenly sampled, so nothing downstream could tell the spacing had been invented. The collapse is still worth doing when it is a pure change of representation, so it now happens only when the resulting range reproduces the original values exactly. A CoordPartial is excluded: its values are NaN placeholders, so canonicalizing it is the whole point. This is what kept the Febus acquisition-window coord from being exact even when built with get_exact_coord, so that caveat comes off the comment and a test pins the drifting-span case end to end. --- dascore/core/coordmanager.py | 27 +++++++++++++++++ dascore/io/febus/g1utils.py | 3 +- tests/test_core/test_coordmanager.py | 36 ++++++++++++++++++++++- tests/test_io/test_febus/test_febusbsl.py | 20 +++++++++++++ 4 files changed, 83 insertions(+), 3 deletions(-) diff --git a/dascore/core/coordmanager.py b/dascore/core/coordmanager.py index c61ccea9f..f1ffe85b6 100644 --- a/dascore/core/coordmanager.py +++ b/dascore/core/coordmanager.py @@ -55,6 +55,7 @@ from dascore.constants import dascore_styles, select_values_description from dascore.core.coords import ( BaseCoord, + CoordPartial, CoordRange, CoordSummary, get_coord, @@ -1219,6 +1220,29 @@ def get_coord_manager( return out +def _canonicalization_moved_values(original, out) -> bool: + """ + Return True if canonicalizing `original` to `out` changed any value. + + Collapsing an array coord to a CoordRange is meant to be a change of + representation, but the evenness test behind it is tolerant, so a + coordinate carrying small real irregularity (measured positions, timing + jitter) would be replaced by an idealized ramp. Only the exact case is a + canonicalization; the rest is data loss. + """ + # Only collapsing to a range can move values, and only a coord we were + # handed can be kept, so everything else skips the comparison. + if original is None or not isinstance(out, CoordRange): + return False + # A CoordPartial is a placeholder whose values are all NaN, so it has + # nothing to lose; canonicalizing it is the whole point. + if isinstance(original, CoordRange | CoordPartial): + return False + if original.shape != out.shape: + return False + return not np.array_equal(original.values, out.values) + + def _get_coord_dim_map(coords, dims): """Get coord_map, dim_map, and new dims from coord input.""" @@ -1233,12 +1257,15 @@ def _get_coord(coord): # CoordRange -- get_coord performs that inference. if isinstance(coord, CoordRange): return coord + original = coord if isinstance(coord, BaseCoord) else None if hasattr(coord, "model_dump"): coord = coord.model_dump(exclude_defaults=True) if isinstance(coord, Mapping): # input is a dict out = get_coord(**coord) else: out = get_coord(data=coord) + if _canonicalization_moved_values(original, out): + return original return out def _coord_from_simple(name, coord): diff --git a/dascore/io/febus/g1utils.py b/dascore/io/febus/g1utils.py index 58be301bf..78065b502 100644 --- a/dascore/io/febus/g1utils.py +++ b/dascore/io/febus/g1utils.py @@ -205,8 +205,7 @@ def _coord(values, units=None): # stored as end_times: starts and ends are each near-regular and snap to # slightly different steps, so subtracting the two built coords would turn # the jitter into a linear drift. Built exactly, and ignoring `snap`, - # because that jitter is the signal -- though a span array regular enough - # to look like a range is still normalized to one by the coord manager. + # because that jitter is the signal. sample_span = get_exact_coord(dc.to_timedelta64(ends - starts)) distance = _coord(resource["distances"][...], units="m") temperature = _coord(resource["temperatures"][...], units="°C") diff --git a/tests/test_core/test_coordmanager.py b/tests/test_core/test_coordmanager.py index afe88248f..98790ab7d 100644 --- a/tests/test_core/test_coordmanager.py +++ b/tests/test_core/test_coordmanager.py @@ -17,6 +17,7 @@ ) from dascore.core.coords import ( BaseCoord, + CoordMonotonicArray, CoordPartial, CoordRange, get_coord, @@ -1068,7 +1069,9 @@ class TestPreserveBaseCoord: 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. + even/empty subset, is canonicalized. That re-inference keeps the coord it was + given whenever collapsing it to a range would move any value, since only the + exact case is a change of representation rather than of data. """ def test_update_preserves_range_coord_identity(self, cm_basic): @@ -1135,6 +1138,37 @@ def test_even_subset_of_irregular_coord_canonicalizes(self): assert isinstance(new_coord, CoordRange) assert new_coord.evenly_sampled + def test_near_even_coord_keeps_its_values(self): + """Small real irregularity must survive; see #896. + + The evenness test is tolerant (rtol 1e-3), so spacing that varies by + less than that used to be replaced by an idealized ramp -- silently + inventing sample positions. + """ + values = np.array([0.0, 1.0, 2.0005, 3.0015, 4.002]) + coord = CoordMonotonicArray(values=values) + cm = get_coord_manager({"distance": coord}, dims=("distance",)) + out = cm.coord_map["distance"] + assert np.array_equal(out.values, values) + # a range here would mean the spacing was made up + assert not isinstance(out, CoordRange) + + def test_near_even_coord_survives_patch(self): + """The public Patch path must not move the values either.""" + values = np.array([0.0, 1.0, 2.0005, 3.0015, 4.002]) + coord = CoordMonotonicArray(values=values) + patch = dc.Patch(data=np.zeros(5), coords={"x": coord}, dims=("x",)) + assert np.array_equal(patch.get_coord("x").values, values) + + def test_exactly_even_array_still_canonicalizes(self): + """Preserving values must not disable the lossless collapse.""" + values = np.arange(5, dtype=float) + coord = CoordMonotonicArray(values=values) + cm = get_coord_manager({"distance": coord}, dims=("distance",)) + out = cm.coord_map["distance"] + assert isinstance(out, CoordRange) + assert np.array_equal(out.values, values) + class TestSqueeze: """Tests for squeezing degenerate dimensions.""" diff --git a/tests/test_io/test_febus/test_febusbsl.py b/tests/test_io/test_febus/test_febusbsl.py index ceb7da1bd..5a424aa78 100644 --- a/tests/test_io/test_febus/test_febusbsl.py +++ b/tests/test_io/test_febus/test_febusbsl.py @@ -131,6 +131,26 @@ def test_sample_span_differenced_before_snapping(self): # the snapped time coord would have given a constant span assert len(np.unique(span.values)) > 1 + def test_sample_span_keeps_a_monotonic_drift(self): + """Spans that drift steadily are regular enough to be snapped away. + + They survive because the coord manager no longer collapses a coord + when doing so would move its values (#896). + """ + starts = np.arange(4, dtype=np.float64) + ends = starts + np.array([1.0, 1.001, 1.0020005, 1.0030015]) + coords = _get_g1_h5_base_coords( + { + "start_times": starts, + "end_times": ends, + "distances": np.arange(2, dtype=np.float64), + "temperatures": np.zeros(4, dtype=np.float64), + }, + dims=("time", "distance"), + ) + span = coords.coord_map["sample_span"] + assert np.array_equal(span.values, dc.to_timedelta64(ends - starts)) + def test_mismatched_time_dataset_lengths_raise(self, bsl_path, tmp_path): """A half-written file should fail loudly, not broadcast to garbage.""" new_path = tmp_path / bsl_path.name From 454016d14153aee65a748196f47277d46d42ea20 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 18:52:49 +0200 Subject: [PATCH 2/2] Drop the unreachable guards from the value check A CoordRange never reaches the helper: the caller returns it before consulting it. The shape comparison could not fail either, since canonicalization re-labels a coordinate rather than resampling it, so it becomes an assert of that invariant instead of a dead branch. --- dascore/core/coordmanager.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/dascore/core/coordmanager.py b/dascore/core/coordmanager.py index f1ffe85b6..4e6332eb8 100644 --- a/dascore/core/coordmanager.py +++ b/dascore/core/coordmanager.py @@ -1231,15 +1231,16 @@ def _canonicalization_moved_values(original, out) -> bool: canonicalization; the rest is data loss. """ # Only collapsing to a range can move values, and only a coord we were - # handed can be kept, so everything else skips the comparison. + # handed can be kept, so everything else skips the comparison. A CoordRange + # cannot reach here; the caller returns it before this is consulted. if original is None or not isinstance(out, CoordRange): return False # A CoordPartial is a placeholder whose values are all NaN, so it has # nothing to lose; canonicalizing it is the whole point. - if isinstance(original, CoordRange | CoordPartial): - return False - if original.shape != out.shape: + if isinstance(original, CoordPartial): return False + # Canonicalization re-labels a coordinate, it never resamples one. + assert original.shape == out.shape return not np.array_equal(original.values, out.values)