Skip to content

Section validator and suggest_cable_shift mishandle their physical conventions #238

@bdestombe

Description

@bdestombe

Two independent silent failures around physical-position handling. Both let invalid configurations slip into calibration with no diagnostic.

1. validate_no_overlapping_sections allows touching slices that double-count the boundary pixel

Location: src/dtscalibration/calibration/section_utils.py:43-45 (root cause); symptom propagates through dts_accessor_utils.py:1286, 1305 to calibrate_utils.py:240-241.

all_start_stop_startsort_flat = sum(all_start_stop_startsort, [])
assert all_start_stop_startsort_flat == sorted(all_start_stop_startsort_flat), \
    "Sections contains overlapping stretches"

The flat == sorted(flat) check enforces stop_i ≤ start_{i+1} — a right-exclusive non-overlap convention. Downstream selection uses ds.x.sel(x=slice(start, stop)), which is right-inclusive (pandas slice_indexer).

For sections = {"cold": [slice(0., 17.)], "warm": [slice(17., 34.)]} (currently accepted), if 17.0 is one of the ds.x coordinates, that pixel ends up in ix_sec twice — once with T_cold, once with T_warm. The WLS observation matrix gets two rows for that pixel with conflicting RHS values, biasing the calibration silently.

Fix: require strict order at the boundary:

assert all(a < b for a, b in zip(flat[:-1], flat[1:])), \
    "Sections contain overlapping or touching stretches"

2. validate_sections does not check slice.start < slice.stop or in-range bounds

Location: src/dtscalibration/calibration/section_utils.py:81-124

  • slice(20, 10) (reversed): triggers the overlap assertion above with the misleading message "Sections contains overlapping stretches". Should report a malformed-slice error.
  • slice(20, 20) (degenerate): passes — ds.x.sel(x=slice(20, 20)).size == 1 thanks to xarray's inclusive bound. Silently included as a one-pixel section.
  • slice(45, 55) when ds.x.max() == 50: passes — xarray clips to [45, 50]. User gets a section silently shorter than they specified.

Fix: explicit per-slice checks before reaching the overlap test:

for k, v in sections.items():
    for vi in v:
        if vi.start is None or vi.stop is None:
            raise ValueError(f"Section {k!r} slice {vi}: both endpoints required")
        if not (vi.start < vi.stop):
            raise ValueError(f"Section {k!r} slice {vi}: start must be < stop")
        if vi.start < float(ds.x.min()) or vi.stop > float(ds.x.max()):
            raise ValueError(f"Section {k!r} slice {vi} outside x-range "
                             f"[{float(ds.x.min())}, {float(ds.x.max())}]")

3. suggest_cable_shift_double_ended hardcoded 1.0 < x < 150.0 mask

Location: src/dtscalibration/dts_accessor_utils.py:1052, 1057

err1_mask = np.logical_and(att_x_dif1 > 1.0, att_x_dif1 < 150.0)
err1.append(np.nansum(np.abs(att_dif1[err1_mask])))

For cables entirely outside (1 m, 150 m) (long field cables, short bench setups), err1_mask is all-False, the nansum(|...|) is 0.0 for every shift in irange. np.argmin then breaks the all-zero tie by returning the first index in irange — a meaningless shift returned with no warning.

Fix: either (a) derive the mask from (ds["x"].min(), ds["x"].max()) with a small inset; (b) expose xmin, xmax keyword arguments; (c) raise ValueError when the mask selects zero pixels.

Suggested tests

  • Two touching sections at a coordinate that exists in ds.x → expect raise from validate_sections. Currently silently accepted.
  • slice(20, 10), slice(20, 20), slice(45, 55) (with x.max() == 50) → each expects a specific raise; currently misleading or silent.
  • Cable with x ∈ [200, 400]suggest_cable_shift_double_ended should warn or raise; currently returns irange[0].

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions