From 804996cbd44b0bee2d2be6b7ef8ae65dea832493 Mon Sep 17 00:00:00 2001 From: Markus Rosenstihl Date: Sun, 18 Oct 2026 15:12:57 +0200 Subject: [PATCH] fix time_period latent crashes: treat missing time information as None time_period is list[datetime] | None at runtime (None from old-format HDF5 files, [] from the constructor), but several sites subscripted it unguarded and crashed with TypeError/IndexError. - annotate time_period as list[datetime.datetime] | None - copy paths (get_accu_by_index, __iadd__ self-empty) use the guarded pattern already present in _copy_provenance - merges in __add__/__iadd__ only combine min/max when both sides carry time information; otherwise the result is None (provenance unknown) - write_to_tecmag raises ValueError when no time information is available instead of crashing while writing the header Clears all remaining time_period pyrefly errors (72 -> 63). Unit tests reproduce the crashes and pass with the fix. Generated by Mistral Vibe. Co-Authored-By: Mistral Vibe --- src/damaris/data/Accumulation.py | 40 +++++++++----- tests/test_Accumulation.py | 93 ++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 13 deletions(-) diff --git a/src/damaris/data/Accumulation.py b/src/damaris/data/Accumulation.py index d6fd85f..9e7e943 100644 --- a/src/damaris/data/Accumulation.py +++ b/src/damaris/data/Accumulation.py @@ -70,7 +70,7 @@ class Accumulation(Errorable, Drawable, DamarisFFT, Signalpath): self.is_clipped = False self.common_descriptions=None - self.time_period=[] + self.time_period: list[datetime.datetime] | None = [] self.job_id = None # this does not make sense for an object accumulated from multiple job_ids self.job_ids = {} @@ -139,7 +139,7 @@ class Accumulation(Errorable, Drawable, DamarisFFT, Signalpath): r.y_square.append(self.y_square[i][start:end+1].copy()) if self.common_descriptions is not None: r.common_descriptions = self.common_descriptions.copy() - r.time_period = self.time_period[:] + r.time_period = self.time_period[:] if self.time_period is not None else None r.job_ids = self.job_ids.copy() r.is_clipped = self.is_clipped self.lock.release() @@ -369,6 +369,8 @@ class Accumulation(Errorable, Drawable, DamarisFFT, Signalpath): base_freq = [frequency, frequency, 0., 0.] offset_freq = [0., 0., 0., 0.] ob_freq = [sum(x) for x in zip(base_freq, offset_freq)] + if self.time_period is None or len(self.time_period)==0: + raise ValueError("write_to_tecmag: no time information available") date = self.time_period[0].strftime("%Y/%m/%d %H:%M:%S") # data handling: @@ -679,7 +681,7 @@ class Accumulation(Errorable, Drawable, DamarisFFT, Signalpath): r = Accumulation(x = numpy.array(other.x, dtype="float32"), y = tmp_y, index = other.index, sampl_freq = other.sampling_rate, n = 1, error = False) if hasattr(other, "is_clipped"): r.is_clipped = other.is_clipped - r.time_period=[other.job_date,other.job_date] + r.time_period=[other.job_date,other.job_date] if other.job_date is not None else None r.job_id = other.job_id r.common_descriptions=other.description.copy() r.job_ids[other.job_id] = other.get_job_date() @@ -713,8 +715,11 @@ class Accumulation(Errorable, Drawable, DamarisFFT, Signalpath): else: r = Accumulation(x = numpy.array(self.x, dtype="float32"), y = tmp_y, n = self.n + 1, index = self.index, sampl_freq = self.sampling_rate, error = False) r.is_clipped = self.is_clipped or (hasattr(other, "is_clipped") and other.is_clipped) - r.time_period=[min(self.time_period[0],other.job_date), - max(self.time_period[1],other.job_date)] + if self.time_period and other.job_date is not None: + r.time_period=[min(self.time_period[0],other.job_date), + max(self.time_period[1],other.job_date)] + else: + r.time_period=None r.job_id = other.job_id r.job_ids = self.job_ids.copy() r.job_ids[other.job_id] = other.get_job_date() @@ -785,8 +790,11 @@ class Accumulation(Errorable, Drawable, DamarisFFT, Signalpath): else: r = Accumulation(x = numpy.array(self.x, dtype="float32"), y = tmp_y, n = other.n + self.n, index = self.index, sampl_freq = self.sampling_rate, error = False) r.is_clipped = self.is_clipped or (hasattr(other, "is_clipped") and other.is_clipped) - r.time_period=[min(self.time_period[0],other.time_period[0]), - max(self.time_period[1],other.time_period[1])] + if self.time_period and other.time_period: + r.time_period=[min(self.time_period[0],other.time_period[0]), + max(self.time_period[1],other.time_period[1])] + else: + r.time_period=None r.job_id = other.job_id r.job_ids = self.job_ids.copy() for i in other.job_ids: @@ -997,7 +1005,7 @@ class Accumulation(Errorable, Drawable, DamarisFFT, Signalpath): if hasattr(other, "is_clipped"): self.is_clipped = other.is_clipped - self.time_period=[other.job_date,other.job_date] + self.time_period=[other.job_date,other.job_date] if other.job_date is not None else None self.job_id = other.job_id # added by Oleg Petrov self.job_ids[other.job_id] = other.get_job_date() self.common_descriptions=other.description.copy() @@ -1027,8 +1035,11 @@ class Accumulation(Errorable, Drawable, DamarisFFT, Signalpath): self.n += 1 if hasattr(other, "is_clipped"): self.is_clipped = self.is_clipped or other.is_clipped - self.time_period=[min(self.time_period[0],other.job_date), - max(self.time_period[1],other.job_date)] + if self.time_period and other.job_date is not None: + self.time_period=[min(self.time_period[0],other.job_date), + max(self.time_period[1],other.job_date)] + else: + self.time_period=None self.job_id = other.job_id @@ -1068,7 +1079,7 @@ class Accumulation(Errorable, Drawable, DamarisFFT, Signalpath): if hasattr(other, "is_clipped"): self.is_clipped = other.is_clipped self.common_descriptions=other.common_descriptions.copy() - self.time_period=other.time_period[:] + self.time_period=other.time_period[:] if other.time_period is not None else None self.job_id = other.job_id # added by Oleg Petrov self.job_ids.update(other.job_ids) self.lock.release() @@ -1099,8 +1110,11 @@ class Accumulation(Errorable, Drawable, DamarisFFT, Signalpath): self.n += other.n if hasattr(other, "is_clipped"): self.is_clipped = self.is_clipped or other.is_clipped - self.time_period=[min(self.time_period[0],other.time_period[0]), - max(self.time_period[1],other.time_period[1])] + if self.time_period and other.time_period: + self.time_period=[min(self.time_period[0],other.time_period[0]), + max(self.time_period[1],other.time_period[1])] + else: + self.time_period=None self.job_id = other.job_id self.job_ids.update(other.job_ids) # Removes mismatched common description keys diff --git a/tests/test_Accumulation.py b/tests/test_Accumulation.py index 760aed7..0fd0a75 100644 --- a/tests/test_Accumulation.py +++ b/tests/test_Accumulation.py @@ -430,5 +430,98 @@ class TestAccumulation(unittest.TestCase): self.assertIsInstance(accu.y_square, list) np.testing.assert_array_equal(accu.get_ysquare(0), y_square_2d[0]) + def test_add_accumulations_without_time_period(self): + """accu + accu without time information must not crash.""" + accu1 = Accumulation() + accu1 += self.create_adc_result() + accu2 = Accumulation() + accu2 += self.create_adc_result() + accu1.time_period = None + accu2.time_period = [] + + r = accu1 + accu2 + + self.assertIsNone(r.time_period) + + def test_iadd_accumulation_without_time_period(self): + """accu += accu without time information must not crash (was IndexError).""" + accu1 = Accumulation() + accu1 += self.create_adc_result() + accu2 = Accumulation() + accu2 += self.create_adc_result() + accu1.time_period = [] + accu2.time_period = [] + + accu1 += accu2 + + self.assertIsNone(accu1.time_period) + + def test_add_adc_without_time_period(self): + """accu + adc without time information must not crash.""" + accu = Accumulation() + accu += self.create_adc_result() + accu.time_period = [] + + r = accu + self.create_adc_result() + + self.assertIsNone(r.time_period) + + def test_time_period_merged_on_add(self): + """accu + accu with both time periods set merges to min/max.""" + accu1 = Accumulation() + accu1 += self.create_adc_result() + accu2 = Accumulation() + accu2 += self.create_adc_result() + accu1.time_period = [datetime(2023, 1, 3), datetime(2023, 1, 5)] + accu2.time_period = [datetime(2023, 1, 2), datetime(2023, 1, 7)] + + r = accu1 + accu2 + + self.assertEqual(r.time_period, [datetime(2023, 1, 2), datetime(2023, 1, 7)]) + + def test_time_period_merged_with_adc_job_date(self): + """accu + adc merges self.time_period with other.job_date.""" + accu = Accumulation() + accu += self.create_adc_result() + accu.time_period = [datetime(2023, 1, 3), datetime(2023, 1, 5)] + adc = self.create_adc_result() + adc.job_date = datetime(2023, 1, 1) + + r = accu + adc + + self.assertEqual(r.time_period, [datetime(2023, 1, 1), datetime(2023, 1, 5)]) + + def test_iadd_adc_without_job_date_sets_time_period_none(self): + """empty accu += adc without job_date gets time_period None.""" + accu = Accumulation() + adc = self.create_adc_result() + adc.job_date = None + + accu += adc + + self.assertIsNone(accu.time_period) + + def test_get_accu_by_index_without_time_period(self): + """get_accu_by_index without time information must not crash.""" + accu = Accumulation() + adc = self.create_adc_result() + adc.index = [(0, 1), (1, 2)] + accu += adc + accu.time_period = None + + r = accu.get_accu_by_index(0) + + self.assertIsNone(r.time_period) + + def test_write_to_tecmag_without_time_period_raises(self): + """write_to_tecmag without time information must raise ValueError.""" + accu = Accumulation() + accu += self.create_adc_result() + accu.time_period = None + + self.assertRaises(ValueError, accu.write_to_tecmag, "/tmp/damaris_tecmag_no_time.tnt") + self.assertTrue(accu.lock.acquire(timeout=1), "lock leaked after ValueError in write_to_tecmag") + accu.lock.release() + if __name__ == "__main__": unittest.main()