diff --git a/dascore/core/coordmanager.py b/dascore/core/coordmanager.py index c61ccea9f..4e6332eb8 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,30 @@ 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. 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, 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) + + def _get_coord_dim_map(coords, dims): """Get coord_map, dim_map, and new dims from coord input.""" @@ -1233,12 +1258,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