fix sorting issue for concatinating backends

Author: peter-urban

meson.build

@@ -8,7 +8,7 @@ project(
   'themachinethatgoesping_pingprocessing',
   'cpp',
   license: 'MPL-2.0',
-  version: '0.14.0',
+  version: '0.14.1',
   default_options: ['warning_level=0', 'buildtype=release', 'cpp_std=c++23'],
   meson_version: '>=1.8.1' #there is a problem with meson 1.8.0 so just use a higher version
 )

python/tests/watercolumn/echograms/test_concat_combine_params.py

@@ -152,3 +152,75 @@ def test_depth_only_builder_has_res_ranges_none_not_attribute_error():
     # attribute (None) rather than raising AttributeError.
     b = EchogramBuilder.from_backend(_make_backend([100.0, 101.0, 102.0]))
     assert b.coord_system.res_ranges is None
+
+
+# ---------------------------------------------------------------------------
+# concat(sort_by_time=True) must produce a strictly increasing timeline even
+# when backends overlap in time. Sorting inputs by start time alone leaves the
+# concatenated ping times non-monotonic, which the coordinate system's time
+# feature (a strictly-increasing interpolator) rejects. These reproduce that
+# crash and verify the per-ping time ordering keeps data access consistent.
+# ---------------------------------------------------------------------------
+
+
+def _make_valued_backend(times, row_values, n_samples=8):
+    """Backend whose every sample in ping i equals row_values[i] (so a column
+    can be traced back to the ping it came from)."""
+    times = np.asarray(times, dtype=np.float64)
+    rows = np.asarray(row_values, dtype=np.float32).reshape(len(times), 1)
+    image = np.repeat(rows, n_samples, axis=1)
+    return ImageBackend.from_image(
+        image, times, y_min=0.0, y_max=20.0, y_axis="depth"
+    )
+
+
+def test_concat_sort_by_time_interleaves_overlapping_backends():
+    # Even vs odd timestamps -> the two backends must interleave ping-by-ping.
+    a = _make_valued_backend([0.0, 2.0, 4.0], [0.0, 2.0, 4.0])
+    b = _make_valued_backend([1.0, 3.0, 5.0], [1.0, 3.0, 5.0])
+
+    combined = EchogramBuilder.concat([a, b], sort_by_time=True)
+    backend = combined.backend
+
+    times = np.asarray(backend.ping_times, dtype=np.float64)
+    assert np.all(np.diff(times) > 0)  # strictly increasing
+    np.testing.assert_array_equal(times, [0.0, 1.0, 2.0, 3.0, 4.0, 5.0])
+    # Each public column must carry the value of the ping at that time.
+    for g in range(6):
+        assert backend.get_column(g)[0] == times[g]
+
+
+def test_concat_sort_by_time_handles_fully_duplicate_backends():
+    # Real-world trigger: the same recording exported under two paths, so both
+    # backends carry identical timestamps. Previously raised
+    # "X list is not sorted in ascending order!" from set_ping_times.
+    times = [100.0, 101.0, 102.0]
+    a = _make_valued_backend(times, [10.0, 11.0, 12.0])
+    b = _make_valued_backend(times, [10.0, 11.0, 12.0])
+
+    combined = EchogramBuilder.concat([a, b], sort_by_time=True)
+    pt = np.asarray(combined.backend.ping_times, dtype=np.float64)
+
+    assert len(pt) == 6  # no pings dropped
+    assert np.all(np.diff(pt) > 0)  # duplicates nudged to strictly increasing
+
+    # The full display path (time feature + image) must not raise.
+    combined.set_x_axis_date_time(max_steps=64)
+    combined.set_y_axis_depth(max_steps=64)
+    image, _ = combined.build_image()
+    assert np.isfinite(image).any()
+
+
+def test_concat_sort_by_time_leaves_sorted_timeline_untouched():
+    # Non-overlapping, already-ordered inputs must not be permuted or nudged.
+    a = _make_valued_backend([100.0, 101.0, 102.0], [1.0, 2.0, 3.0])
+    b = _make_valued_backend([200.0, 201.0, 202.0], [4.0, 5.0, 6.0])
+
+    combined = EchogramBuilder.concat([a, b], sort_by_time=True)
+    backend = combined.backend
+
+    assert backend._order is None  # fast path, no permutation
+    np.testing.assert_array_equal(
+        np.asarray(backend.ping_times, dtype=np.float64),
+        [100.0, 101.0, 102.0, 200.0, 201.0, 202.0],
+    )

python/themachinethatgoesping/pingprocessing/watercolumn/echograms/backends/concat_backend.py

@@ -14,6 +14,27 @@
 from ..indexers import EchogramImageRequest
 
 
+def _make_strictly_increasing(times: np.ndarray, eps: float = 1e-4) -> np.ndarray:
+    """Return a copy of ``times`` nudged to be strictly increasing.
+
+    Assumes ``times`` is already non-decreasing. Equal / zero-gap timestamps
+    (common when overlapping echograms are concatenated) are pushed forward by
+    ``eps`` seconds so the downstream strictly-increasing time interpolator
+    accepts them.
+    """
+    t = np.asarray(times, dtype=np.float64).copy()
+    if t.size < 2:
+        return t
+    bad = np.where(np.diff(t) <= 0)[0]
+    # Each pass fixes the first collision in every run of equal values; a run of
+    # N equal timestamps needs at most N-1 passes, which is tiny for real ping
+    # timelines (typically just duplicated pairs).
+    while bad.size > 0:
+        t[bad + 1] = t[bad] + eps
+        bad = np.where(np.diff(t) <= 0)[0]
+    return t
+
+
 class ConcatBackend(EchogramDataBackend):
     """Virtual backend that concatenates multiple backends along X (ping) axis.
     
@@ -36,6 +57,7 @@ def __init__(
         self,
         backends: List[EchogramDataBackend],
         gap_handling: str = "preserve",
+        sort_by_time: bool = False,
     ):
         """Initialize ConcatBackend.
         
@@ -45,6 +67,10 @@ def __init__(
             gap_handling: How to handle gaps between backends:
                 - "preserve": Keep real time gaps (x-axis shows true times)
                 - "continuous": Virtual continuous (ignore gaps between files)
+            sort_by_time: If True, reorder all pings into a single strictly
+                increasing timeline. Needed when backends overlap in time
+                (sorting inputs by start time alone leaves the concatenated
+                times non-monotonic, which the coordinate system rejects).
         
         Raises:
             ValueError: If backends list is empty or has incompatible metadata.
@@ -57,7 +83,15 @@ def __init__(
 
         self._backends = backends
         self._gap_handling = gap_handling
-        
+        self._sort_by_time = sort_by_time
+
+        # Per-ping time ordering. ``_order`` maps a public (sorted) ping index
+        # to its physical position in the backend concatenation; ``_inv_order``
+        # is the inverse. ``None`` means the public order already equals the
+        # physical concatenation order (fast path).
+        self._order = None
+        self._inv_order = None
+
         # Validate and collect metadata
         self._validate_backends()
 
@@ -67,6 +101,10 @@ def __init__(
         # Combine metadata from all backends
         self._compute_combined_metadata()
 
+        # Reorder pings into a strictly increasing timeline when requested.
+        if self._sort_by_time:
+            self._apply_time_ordering()
+
     def _validate_backends(self):
         """Validate that backends are compatible for concatenation."""
         first = self._backends[0]
@@ -193,23 +231,85 @@ def _concat_ping_params(self) -> Dict[str, Tuple[str, Tuple[np.ndarray, np.ndarr
 
         return result
 
+    def _apply_time_ordering(self):
+        """Reorder pings so the combined timeline is strictly increasing.
+
+        Sorting inputs by their start time (as done by ``concat``) is not
+        enough when backends overlap in time (e.g. the same recording exported
+        under two paths): the concatenated ping times then step backwards at a
+        backend boundary, which the coordinate system's time feature (a
+        strictly-increasing interpolator) rejects.
+
+        This performs a stable global sort of all pings by time and applies the
+        resulting permutation to every per-ping array. Exact-duplicate / zero
+        gap timestamps are nudged apart so the timeline is strictly increasing.
+        The permutation is routed through ``_global_to_local`` so data access
+        stays correct; it is left as ``None`` (fast path) when the concatenated
+        timeline is already sorted.
+        """
+        times = np.asarray(self._ping_times, dtype=np.float64)
+        if times.size < 2:
+            return
+
+        if np.any(np.diff(times) < 0):
+            order = np.argsort(times, kind="stable")
+            times = times[order]
+        else:
+            order = None
+
+        # The time feature interpolator forbids equal (zero-gap) x values.
+        if np.any(np.diff(times) <= 0):
+            times = _make_strictly_increasing(times)
+
+        if order is None:
+            # Physical order already time-sorted; publish the (possibly nudged)
+            # times and keep the identity mapping.
+            self._ping_times = times
+            return
+
+        # Apply the permutation to every per-ping array so ping_times, extents
+        # and data access all refer to the same ping.
+        self._ping_times = times
+        self._max_sample_counts = self._max_sample_counts[order]
+        self._sample_nr_min = self._sample_nr_min[order]
+        self._sample_nr_max = self._sample_nr_max[order]
+        if self._range_min is not None:
+            self._range_min = self._range_min[order]
+            self._range_max = self._range_max[order]
+        if self._depth_min is not None:
+            self._depth_min = self._depth_min[order]
+            self._depth_max = self._depth_max[order]
+        if self._latitudes is not None:
+            self._latitudes = self._latitudes[order]
+            self._longitudes = self._longitudes[order]
+
+        self._order = order
+        self._inv_order = np.empty(order.size, dtype=np.int64)
+        self._inv_order[order] = np.arange(order.size, dtype=np.int64)
+
     def _global_to_local(self, global_ping: int) -> Tuple[int, int]:
         """Convert global ping index to (backend_index, local_ping_index).
         
         Uses binary search for O(log n) lookup.
         """
         if global_ping < 0 or global_ping >= self._n_pings:
             raise IndexError(f"Ping index {global_ping} out of range [0, {self._n_pings})")
-        
+
+        # Map the public (sorted) index back to its physical concat position.
+        physical = global_ping if self._order is None else int(self._order[global_ping])
+
         # Binary search: find which backend contains this ping
-        backend_idx = bisect_right(self._cumulative_pings, global_ping) - 1
-        local_ping = global_ping - self._cumulative_pings[backend_idx]
+        backend_idx = bisect_right(self._cumulative_pings, physical) - 1
+        local_ping = physical - self._cumulative_pings[backend_idx]
 
         return backend_idx, local_ping
 
     def _local_to_global(self, backend_idx: int, local_ping: int) -> int:
-        """Convert (backend_index, local_ping_index) to global ping index."""
-        return self._cumulative_pings[backend_idx] + local_ping
+        """Convert (backend_index, local_ping_index) to global (public) ping index."""
+        physical = self._cumulative_pings[backend_idx] + local_ping
+        if self._inv_order is None:
+            return physical
+        return int(self._inv_order[physical])
 
     # =========================================================================
     # Metadata properties
@@ -333,6 +433,12 @@ def get_raw_column(self, ping_index: int) -> np.ndarray:
 
     def get_chunk(self, start_ping: int, end_ping: int) -> np.ndarray:
         """Get a chunk of WCI data spanning potentially multiple backends."""
+        # When pings were reordered by time, a contiguous public range no longer
+        # maps to contiguous per-backend ranges; fall back to the per-ping
+        # gather (which honours the permutation via get_column).
+        if self._order is not None:
+            return super().get_chunk(start_ping, end_ping)
+
         # Find which backends are involved
         start_backend, start_local = self._global_to_local(start_ping)
         end_backend, end_local = self._global_to_local(end_ping - 1)

python/themachinethatgoesping/pingprocessing/watercolumn/echograms/echogrambuilder.py

@@ -465,7 +465,7 @@ def _start_time(backend: "EchogramDataBackend"):
 
             backends.sort(key=_start_time)
 
-        concat_backend = ConcatBackend(backends, gap_handling=gap_handling)
+        concat_backend = ConcatBackend(backends, gap_handling=gap_handling, sort_by_time=sort_by_time)
         result = cls(concat_backend)
         # Coordinate-system params added after construction (e.g. detect_bottom
         # results referenced by 'Ping index') are not part of the backend and