From 73058d63d6ca716e2d19d7231b5e53fec2d18f22 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 17:04:35 +0200 Subject: [PATCH 1/4] Keep the Febus G1 sample window, drop its redundant attrs The G1 HDF5 attr map copied the root start_time/end_time header attrs into patch attrs, where they restated the time datasets as epoch-second floats; both are bit-identical to the first start_times and last end_times entries. The A1 text path already excludes them. formatVersion went the same way: it sanitized to a reserved index column, so every index build warned and the attr was unqueryable, and the same value is already reported as source_version. The readers also ignored end_times entirely, presenting each sample as instantaneous when it is really an average over an acquisition window. Patches now carry a sample_span coord holding that length. The span is differenced off the raw arrays because starts and ends are each near-regular and snap to slightly different steps, which would turn the per-sample jitter into a linear drift. It is not called time_span: a name starting with a dimension shadows the {dim}_{suffix} envelope convention and breaks update_coords with an unpack error. Mismatched start_times/end_times lengths now raise instead of broadcasting into garbage spans, and start_time/end_time leave the VENDOR_ATTRS allowlist so no reader can reintroduce them unnoticed. --- dascore/io/febus/core.py | 15 ++++- dascore/io/febus/g1utils.py | 26 ++++++-- tests/test_io/test_common_io.py | 2 - tests/test_io/test_febus/test_febusbsl.py | 74 ++++++++++++++++++++++- tests/test_io/test_febus/test_febusg1.py | 8 ++- 5 files changed, 115 insertions(+), 10 deletions(-) diff --git a/dascore/io/febus/core.py b/dascore/io/febus/core.py index 590dde056..bb9071c99 100644 --- a/dascore/io/febus/core.py +++ b/dascore/io/febus/core.py @@ -188,7 +188,12 @@ def read(self, resource: TextReader, **kwargs) -> dc.BaseSpool: class FebusMTXH5V1(FiberIO): - """HDF5 format used by Febus for storing Brillouin spectra.""" + """ + HDF5 format used by Febus for storing Brillouin spectra. + + As with the BSL files, ``time`` holds the start of each acquisition + window and the non-dimensional ``sample_span`` coord holds its length. + """ name = "febus_mtx_h5" preferred_extensions = ("h5", "hdf5") @@ -246,7 +251,13 @@ def read( class FebusBSLH5V1(FiberIO): - """HDF5 format used by Febus G1 for storing BSL strain files.""" + """ + HDF5 format used by Febus G1 for storing BSL strain files. + + Samples are not instantaneous; each one covers an acquisition window. + The ``time`` coord holds the start of that window and the non-dimensional + ``sample_span`` coord, mapped to ``time``, holds how long it ran. + """ name = "febus_bsl_h5" preferred_extensions = ("h5", "hdf5") diff --git a/dascore/io/febus/g1utils.py b/dascore/io/febus/g1utils.py index b1d89be5a..58be301bf 100644 --- a/dascore/io/febus/g1utils.py +++ b/dascore/io/febus/g1utils.py @@ -22,6 +22,10 @@ _MTX_DIMS = ("time", "distance", "frequency") _BSL_DIMS = ("time", "distance") _BSL_H5_MODE_STRAIN = 1 +# The root "start_time"/"end_time" attrs are bit-identical to the first +# start_times and last end_times entries, so they only restate the time +# datasets as untyped epoch floats. "formatVersion" is already reported as +# source_version. All three are deliberately absent here. _G1_H5_ATTR_MAP = { "acq_res": "acq_res", "ampliPower": "ampli_power", @@ -31,7 +35,6 @@ "fiberFrom": "fiber_from", "fiberLength": "fiber_length", "fiberTo": "fiber_to", - "formatVersion": "format_version", "freq_fiber": "freq_fiber", "freq_offset": "freq_offset", "freq_offset_abs": "freq_offset_abs", @@ -41,8 +44,6 @@ "sampling_resolution": "sampling_resolution", "signal_size": "signal_size", "spatial_resolution": "spatial_resolution", - "start_time": "start_time", - "end_time": "end_time", "zoneCount": "zone_count", "zones": "zones", "febusDataKind": "febus_data_kind", @@ -190,13 +191,30 @@ def _coord(values, units=None): return get_exact_coord(values, units=units) extra_coords = {} if extra_coords is None else extra_coords - time = _coord(dc.to_datetime64(resource["start_times"][...])) + starts = resource["start_times"][...] + ends = resource["end_times"][...] + if ends.shape != starts.shape: + msg = ( + f"start_times has shape {starts.shape} but end_times has " + f"{ends.shape}; the file is truncated or still being written." + ) + raise ValueError(msg) + time = _coord(dc.to_datetime64(starts)) + # Each sample covers a window rather than being instantaneous, so keep how + # long it ran. The span is differenced off the raw arrays rather than + # 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. + sample_span = get_exact_coord(dc.to_timedelta64(ends - starts)) distance = _coord(resource["distances"][...], units="m") temperature = _coord(resource["temperatures"][...], units="°C") coords = { "time": time, "distance": distance, "temperature": ("time", temperature), + "sample_span": ("time", sample_span), **extra_coords, } return dc.get_coord_manager(coords, dims=dims) diff --git a/tests/test_io/test_common_io.py b/tests/test_io/test_common_io.py index abdbd2571..47626c4fe 100644 --- a/tests/test_io/test_common_io.py +++ b/tests/test_io/test_common_io.py @@ -152,7 +152,6 @@ "demod_data_type", "dtype", "end_frequency", - "end_time", "epsg_code", "event_depth_km", "event_id", @@ -216,7 +215,6 @@ "source", "spatial_resolution", "start_frequency", - "start_time", "temperature", "time_decimation_filter", "trace_count", diff --git a/tests/test_io/test_febus/test_febusbsl.py b/tests/test_io/test_febus/test_febusbsl.py index 00931eb7d..ceb7da1bd 100644 --- a/tests/test_io/test_febus/test_febusbsl.py +++ b/tests/test_io/test_febus/test_febusbsl.py @@ -12,7 +12,7 @@ import dascore as dc from dascore.constants import STORAGE_PROVENANCE_ATTRS from dascore.io.febus import FebusBSLH5V1 -from dascore.io.febus.g1utils import _get_bsl_attrs +from dascore.io.febus.g1utils import _get_bsl_attrs, _get_g1_h5_base_coords from dascore.utils.downloader import fetch BSL_NAME = "febusg1_C2_2026-06-03T17.18.13+0200.bsl.h5" @@ -100,6 +100,78 @@ def test_time_coord(self, bsl_patch): assert time.step is None assert bsl_patch.summary.get_coord_summary("time").step is None + def test_sample_span_coord(self, bsl_path, bsl_patch): + """Each sample covers a window; its exact length should be kept.""" + with h5py.File(bsl_path) as fi: + expected = fi["end_times"][...] - fi["start_times"][...] + span = bsl_patch.get_coord("sample_span") + assert bsl_patch.coords.dim_map["sample_span"] == ("time",) + assert np.array_equal(span.values, dc.to_timedelta64(expected)) + + def test_sample_span_differenced_before_snapping(self): + """Spans come off the raw arrays, not from subtracting built coords. + + Starts and ends are each near-regular and snap to slightly different + steps, so differencing the two coords would show a linear drift where + the file has jitter. + """ + starts = np.array([0.0, 1.0, 2.0, 3.0]) + ends = starts + np.array([0.9, 1.2, 0.8, 1.1]) + 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)) + # the snapped time coord would have given a constant span + assert len(np.unique(span.values)) > 1 + + 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 + shutil.copy2(bsl_path, new_path) + with h5py.File(new_path, "a") as fi: + ends = fi["end_times"][...] + del fi["end_times"] + fi.create_dataset("end_times", data=ends[:1]) + with pytest.raises(ValueError, match="truncated or still being written"): + self.parser.read(new_path) + + def test_redundant_time_attrs_absent(self, bsl_patch): + """Neither the epoch-float time attrs nor format_version reach attrs.""" + names = set(dict(bsl_patch.attrs)) + assert not names & {"start_time", "end_time", "format_version"} + + def test_dropped_time_attrs_lose_nothing(self, bsl_path, bsl_patch): + """What the dropped attrs said is still recoverable from the coords. + + This is what justifies removing them rather than retyping them. + """ + with h5py.File(bsl_path) as fi: + start_attr = float(fi.attrs["start_time"][0]) + end_attr = float(fi.attrs["end_time"][0]) + time = bsl_patch.get_coord("time") + span = bsl_patch.get_coord("sample_span") + assert time.min() == dc.to_datetime64(start_attr) + # The end rebuilds through two independent float-seconds-to-ns + # roundings, so it lands within a sample of the stored attr rather + # than on it; the file's own epoch floats are only good to ~238 ns + # here anyway. + rebuilt = time.max() + span.values[-1] + assert abs(rebuilt - dc.to_datetime64(end_attr)) < np.timedelta64(1, "us") + + def test_select_slices_sample_span(self, bsl_path, bsl_patch): + """Selecting on time should carry the associated span along.""" + time = bsl_patch.get_coord("time") + out = self.parser.read(bsl_path, time=(time.values[3], time.values[8]))[0] + expected = bsl_patch.get_coord("sample_span").values[3:9] + assert np.array_equal(out.get_coord("sample_span").values, expected) + def test_select(self, bsl_path, bsl_patch): """Partial reads should reduce coords and data consistently.""" time = bsl_patch.get_coord("time") diff --git a/tests/test_io/test_febus/test_febusg1.py b/tests/test_io/test_febus/test_febusg1.py index 8adbf970d..afd41773e 100644 --- a/tests/test_io/test_febus/test_febusg1.py +++ b/tests/test_io/test_febus/test_febusg1.py @@ -157,10 +157,16 @@ def test_read(self, mtx_h5_path): assert patch.shape == (68, 100, 128) assert patch.attrs.data_category == "DSS" assert patch.attrs.data_type == "brillouin_spectrum" - assert patch.attrs.format_version == 1 assert patch.attrs.fiber_from == 50 assert "temperature" in patch.coords.coord_map assert patch.coords.dim_map["temperature"] == ("time",) + assert patch.coords.dim_map["sample_span"] == ("time",) + + def test_format_version_not_duplicated_in_attrs(self, mtx_h5_path): + """The file version is reported as the format version, not as an attr.""" + patch = dc.read(mtx_h5_path)[0] + assert "format_version" not in dict(patch.attrs) + assert dc.scan(mtx_h5_path)[0].source_version == FebusMTXH5V1.version def test_read_preserves_mtx_array_order(self, tmp_path): """The patch data should match the MTX array stored in the file.""" From 69c512ab37f1605d3ce4114065487f0a449ed3e2 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 17:04:42 +0200 Subject: [PATCH 2/4] Keep a stepless numeric coord's step null in the index The envelope columns were assigned as bare object arrays, letting pandas re-infer a dtype. When no numeric coord in a result carries a step the array holds only Timedeltas and None, so it inferred timedelta64 and turned those numeric nulls into NaT; the later pd.to_numeric mapped them to int64 min. Assigning an explicit object-dtype Series keeps them NaN. _env_min/_env_max shared the hazard. --- dascore/io/index/backend.py | 6 +- .../test_index/test_index_edge_cases.py | 56 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/dascore/io/index/backend.py b/dascore/io/index/backend.py index b8103614b..7bc5eaccb 100644 --- a/dascore/io/index/backend.py +++ b/dascore/io/index/backend.py @@ -994,7 +994,11 @@ def _time_objects(ns_series: pd.Series, flavor: str) -> np.ndarray: for mask, flavor in time_flavors: if mask.any(): values[mask] = _time_objects(coords[ns_col][mask], flavor) - coords[out_col] = values + # Assigning the bare array lets pandas re-infer a dtype; a result + # whose numeric coords all have null envelopes holds only + # Timestamp/Timedelta and None, which would infer a temporal dtype + # and silently turn those numeric nulls into NaT. + coords[out_col] = pd.Series(values, index=coords.index, dtype=object) # Summary-only definitions are useful for indexing/dedup but cannot # prove coordinate value identity for merge grouping. coords["_key"] = coords["def_key"].where(coords["fingerprint"].notna(), None) diff --git a/tests/test_io/test_index/test_index_edge_cases.py b/tests/test_io/test_index/test_index_edge_cases.py index 572562172..0134ebd91 100644 --- a/tests/test_io/test_index/test_index_edge_cases.py +++ b/tests/test_io/test_index/test_index_edge_cases.py @@ -141,6 +141,62 @@ def _summary(gain, path): finally: back.close() + def test_stepless_numeric_coord_keeps_null_step(self, tmp_path): + """A numeric coord with no step stays NaN rather than an int sentinel. + + When no numeric coord in the result carries a step, the envelope + column holds only timedeltas and nulls; inferring a dtype there + turned those nulls into NaT, and then into int64 min. + """ + # the time coord's step is what puts a Timedelta in the shared envelope + # column; without it there is nothing for pandas to infer a temporal + # dtype from, and the bug cannot occur. Stated here rather than + # inherited from a helper default so the trigger cannot drift away. + time_coord = _time_coord("2024-01-01T00:00:00", 60, step_s=0.004) + assert time_coord["step"] is not None + summary = PatchSummary( + attrs={"tag": "t"}, + coords={ + "time": time_coord, + # both numeric coords are stepless, so nothing anchors the + # envelope column to a numeric dtype + "distance": { + "dtype": "float64", + "min": 0.0, + "max": 10.0, + "step": None, + "units": "m", + "dims": ("distance",), + "len": 11, + }, + "temperature": { + "dtype": "float64", + "min": 20.0, + "max": 25.0, + "step": None, + "units": "degC", + "dims": ("time",), + "len": 15000, + }, + }, + dims=("time", "distance"), + shape=(15000, 11), + dtype="float32", + source_path="a.h5", + source_format="X", + source_version="1", + ) + back = get_backend(tmp_path / "i.sqlite3") + try: + back.write_sources(s2r([summary])) + df = back.query() + step = df["temperature_step"] + # int64 min is what a NaT in this column decays to + assert step.dtype == np.dtype("float64") + assert step.isna().all() + finally: + back.close() + def test_reopen_missing_meta_row(self, tmp_path): """An index whose meta row was lost is rejected on reopen.""" path = tmp_path / "i.sqlite3" From 2b424185c4e8a48bd654dcf5923ac3b20dc7d640 Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 17:04:42 +0200 Subject: [PATCH 3/4] Merge a coordinate with its own values, not its dimension's _get_merged_coords used dim where it meant coord_name, so every coordinate mapped to the merge dimension had the dimension coordinate's values concatenated in place of its own. On any multi-file Febus G1 spool this made temperature come back as datetime64 copies of time after a chunk, and it would have done the same to sample_span. Snapping now applies only to the dimension coordinate, since only it defines contiguity. --- dascore/utils/coordmanager.py | 11 +++++++---- tests/test_utils/test_coordmanager_utils.py | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/dascore/utils/coordmanager.py b/dascore/utils/coordmanager.py index e176aebe3..4d3222de1 100644 --- a/dascore/utils/coordmanager.py +++ b/dascore/utils/coordmanager.py @@ -117,7 +117,7 @@ def _get_merged_coords(managers, coords_to_merge): if dim_coord is not None and coord_name == dim: out[coord_name] = (managers[0].dim_map[dim], dim_coord) continue - merge_coords = [x.coord_map[dim] for x in managers] + merge_coords = [x.coord_map[coord_name] for x in managers] axis = managers[0].dim_map[coord_name].index(dim) if len(units := {x.units for x in merge_coords}) != 1: # TODO: we might try to convert all the units to a common @@ -127,9 +127,12 @@ def _get_merged_coords(managers, coords_to_merge): f"share the same units. Units found are: {set(units)}" ) raise CoordMergeError(msg) - snap_coords = _snap_coords(merge_coords) - data = [x.data for x in snap_coords] - dims = managers[0].dim_map[dim] + # Only the dimension coordinate defines contiguity, so only it is + # snapped; coords merely associated with dim just follow along. + if coord_name == dim: + merge_coords = _snap_coords(merge_coords) + data = [x.data for x in merge_coords] + dims = managers[0].dim_map[coord_name] new_data = np.concatenate(data, axis=axis) # raw value concatenation loses the coord's units; reattach # the (verified common) units so the merge stays unit-true diff --git a/tests/test_utils/test_coordmanager_utils.py b/tests/test_utils/test_coordmanager_utils.py index 7c511a280..12ede0fc8 100644 --- a/tests/test_utils/test_coordmanager_utils.py +++ b/tests/test_utils/test_coordmanager_utils.py @@ -51,6 +51,23 @@ def test_merge_simple(self, cm_basic): assert new_time.min() == time.min() assert new_time.max() == cm2.coord_map["time"].max() + def test_merge_keeps_associated_coord_values(self, cm_basic): + """A coord along the merge dim keeps its own values, not the dim's.""" + cm1 = cm_basic.update_coords( + quality=("time", np.arange(cm_basic.shape[cm_basic.get_axis("time")])) + ) + time = cm1.coord_map["time"] + cm2 = self._get_offset_coord_manager(cm1, time=time.step) + out = merge_coord_managers([cm1, cm2], dim="time") + quality = out.coord_map["quality"] + expected = np.concatenate( + [cm1.coord_map["quality"].values, cm2.coord_map["quality"].values] + ) + # the dim coord's values used to be substituted here, which for a + # datetime time dim also silently changed the coord's dtype + assert quality.dtype == cm1.coord_map["quality"].dtype + assert np.array_equal(quality.values, expected) + def test_merge_offset_close_no_snap(self, cm_basic): """When the coordinate don't line up, it should produce monotonic Coord.""" cm1 = cm_basic From e957f1d791a51ded231989bbe86a59e52cbac9af Mon Sep 17 00:00:00 2001 From: Derrick Chambers Date: Thu, 13 Aug 2026 17:45:49 +0200 Subject: [PATCH 4/4] Give the merge test values only a real merge can produce Both managers carried identical values for the associated coord, so concatenating the first one's values twice would have passed. They now differ. The merge also runs with a snap tolerance and a gap that lands inside it, which is what exercises snapping being restricted to the dimension coordinate; snapping a stepless coord raises. --- tests/test_utils/test_coordmanager_utils.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/tests/test_utils/test_coordmanager_utils.py b/tests/test_utils/test_coordmanager_utils.py index 12ede0fc8..98c59f466 100644 --- a/tests/test_utils/test_coordmanager_utils.py +++ b/tests/test_utils/test_coordmanager_utils.py @@ -53,20 +53,24 @@ def test_merge_simple(self, cm_basic): def test_merge_keeps_associated_coord_values(self, cm_basic): """A coord along the merge dim keeps its own values, not the dim's.""" - cm1 = cm_basic.update_coords( - quality=("time", np.arange(cm_basic.shape[cm_basic.get_axis("time")])) - ) + size = cm_basic.shape[cm_basic.get_axis("time")] + cm1 = cm_basic.update_coords(quality=("time", np.arange(size))) time = cm1.coord_map["time"] - cm2 = self._get_offset_coord_manager(cm1, time=time.step) - out = merge_coord_managers([cm1, cm2], dim="time") + # each manager gets its own quality values; identical ones could not + # tell a real merge from the first manager's values used twice + cm2 = self._get_offset_coord_manager(cm1, time=time.step * 1.1) + cm2 = cm2.update_coords(quality=("time", np.arange(size) + 100)) + # a tolerance the offset above lands inside, so snapping runs: the + # dim coord is snapped and quality, having no step, must be left alone + out = merge_coord_managers([cm1, cm2], dim="time", snap_tolerance=1.3) quality = out.coord_map["quality"] - expected = np.concatenate( - [cm1.coord_map["quality"].values, cm2.coord_map["quality"].values] - ) + expected = np.concatenate([np.arange(size), np.arange(size) + 100]) # the dim coord's values used to be substituted here, which for a # datetime time dim also silently changed the coord's dtype assert quality.dtype == cm1.coord_map["quality"].dtype assert np.array_equal(quality.values, expected) + assert out.dim_map["quality"] == ("time",) + assert out.coord_map["time"].shape == quality.shape def test_merge_offset_close_no_snap(self, cm_basic): """When the coordinate don't line up, it should produce monotonic Coord."""