Merge pull request #2357 from cta-observatory/fix_software_trigger

Fix removal of telescope events in software trigger, fixes #2356

ctapipe/instrument/tests/test_trigger.py

@@ -1,3 +1,5 @@
+import json
+
 import numpy as np
 import pytest
 from numpy.testing import assert_equal
@@ -6,6 +8,20 @@
 from ctapipe.io import EventSource
 
 
+def assert_all_tel_keys(event, expected, ignore=None):
+    if ignore is None:
+        ignore = set()
+
+    expected = tuple(expected)
+    for name, container in event.items():
+        if hasattr(container, "tel"):
+            actual = tuple(container.tel.keys())
+            if name not in ignore and actual != expected:
+                raise AssertionError(
+                    f"Unexpected tel_ids in container {name}:" f"{actual} != {expected}"
+                )
+
+
 @pytest.mark.parametrize("data_type", (list, np.array))
 def test_software_trigger(subarray_prod5_paranal, data_type):
     from ctapipe.instrument.trigger import SoftwareTrigger
@@ -102,3 +118,118 @@ def test_software_trigger_simtel(allowed_tels):
         for e, expected_tels in zip(source, expected):
             trigger(e)
             assert_equal(e.trigger.tels_with_trigger, expected_tels)
+            assert_all_tel_keys(e, expected_tels, ignore={"dl0", "dl1", "dl2", "muon"})
+
+
+def test_software_trigger_simtel_single_lsts():
+    from ctapipe.instrument.trigger import SoftwareTrigger
+
+    path = "dataset://gamma_divergent_LaPalma_baseline_20Zd_180Az_prod3_test.simtel.gz"
+
+    # remove 3 LSTs, so that we trigger the 1-LST condition
+    allowed_tels = [1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19]
+    expected = [
+        [12, 16],
+        [],
+        [],
+        [],
+        [],
+        [],
+        [5, 6, 7, 12, 15, 16, 17, 18],
+        [13, 14],
+        [],
+        [7, 12],
+        [5, 17],
+        [13, 19],
+        [],
+        [],
+        [5, 11, 18],
+        [17, 18],
+        [7, 12],
+        [],
+    ]
+
+    with EventSource(
+        path, focal_length_choice="EQUIVALENT", allowed_tels=allowed_tels
+    ) as source:
+
+        trigger = SoftwareTrigger(
+            subarray=source.subarray,
+            min_telescopes=2,
+            min_telescopes_of_type=[
+                ("type", "*", 0),
+                ("type", "LST*", 2),
+            ],
+        )
+
+        for e, expected_tels in zip(source, expected):
+            print(e.trigger.tels_with_trigger)
+            trigger(e)
+            print(e.trigger.tels_with_trigger, expected_tels)
+            assert_equal(e.trigger.tels_with_trigger, expected_tels)
+            assert_all_tel_keys(e, expected_tels, ignore={"dl0", "dl1", "dl2", "muon"})
+
+
+def test_software_trigger_simtel_process(tmp_path):
+    from ctapipe.core import run_tool
+    from ctapipe.io import TableLoader
+    from ctapipe.tools.process import ProcessorTool
+
+    path = "dataset://gamma_divergent_LaPalma_baseline_20Zd_180Az_prod3_test.simtel.gz"
+    config = dict(
+        ProcessorTool=dict(
+            EventSource=dict(
+                focal_length_choice="EQUIVALENT",
+                # remove 3 LSTs, so that we trigger the 1-LST condition
+                allowed_tels=(1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19),
+            ),
+            SoftwareTrigger=dict(
+                min_telescopes=2,
+                min_telescopes_of_type=[
+                    ("type", "*", 0),
+                    ("type", "LST*", 2),
+                ],
+            ),
+        )
+    )
+
+    output_path = tmp_path / "software_trigger.dl1.h5"
+    config_path = tmp_path / "config.json"
+    config_path.write_text(json.dumps(config))
+
+    run_tool(
+        ProcessorTool(),
+        [f"--input={path}", f"--output={output_path}", f"--config={config_path}"],
+    )
+
+    del config["ProcessorTool"]["SoftwareTrigger"]
+    output_path_no_software_trigger = tmp_path / "no_software_trigger.dl1.h5"
+    config_path = tmp_path / "config_no_software_trigger.json"
+    config_path.write_text(json.dumps(config))
+
+    run_tool(
+        ProcessorTool(),
+        [
+            f"--input={path}",
+            f"--output={output_path_no_software_trigger}",
+            f"--config={config_path}",
+        ],
+    )
+
+    with TableLoader(
+        output_path,
+        load_simulated=True,
+        load_dl1_parameters=True,
+        focal_length_choice="EQUIVALENT",
+    ) as loader:
+        events_trigger = loader.read_telescope_events("LST_LST_LSTCam")
+
+    with TableLoader(
+        output_path_no_software_trigger,
+        load_simulated=True,
+        load_dl1_parameters=True,
+        focal_length_choice="EQUIVALENT",
+    ) as loader:
+        events_no_trigger = loader.read_telescope_events("LST_LST_LSTCam")
+
+    assert len(events_no_trigger) > len(events_trigger)

ctapipe/instrument/trigger.py

@@ -103,10 +103,11 @@ def __call__(self, event: ArrayEventContainer) -> bool:
                     tels_removed.add(tel_id)
 
                     # remove any related data
-                    for container in ("trigger", "r0", "r1", "dl0", "dl1", "dl2"):
-                        tel_map = getattr(event, container).tel
-                        if tel_id in tel_map:
-                            del tel_map[tel_id]
+                    for container in event.values():
+                        if hasattr(container, "tel"):
+                            tel_map = container.tel
+                            if tel_id in tel_map:
+                                del tel_map[tel_id]
 
         if len(tels_removed) > 0:
             # convert to array with correct dtype to have setdiff1d work correctly
@@ -117,8 +118,9 @@ def __call__(self, event: ArrayEventContainer) -> bool:
 
         if len(event.trigger.tels_with_trigger) < self.min_telescopes:
             event.trigger.tels_with_trigger = []
-            for container in ("trigger", "r0", "r1", "dl0", "dl1", "dl2"):
-                tel_map = getattr(event, container).tel
-                tel_map.clear()
+            # remove any related data
+            for container in event.values():
+                if hasattr(container, "tel"):
+                    container.tel.clear()
             return False
         return True

ctapipe/io/datawriter.py

@@ -647,6 +647,11 @@ def _write_dl1_telescope_events(
         """
 
         # write the telescope tables
+        # trigger info
+        for tel_id, trigger in event.trigger.tel.items():
+            writer.write(
+                "dl1/event/telescope/trigger", [_get_tel_index(event, tel_id), trigger]
+            )
 
         # pointing info
         for tel_id, pnt in event.pointing.tel.items():
@@ -659,12 +664,6 @@ def _write_dl1_telescope_events(
                 )
                 self._last_pointing_tel[tel_id] = current_pointing
 
-        # trigger info
-        for tel_id, trigger in event.trigger.tel.items():
-            writer.write(
-                "dl1/event/telescope/trigger", [_get_tel_index(event, tel_id), trigger]
-            )
-
         for tel_id, dl1_camera in event.dl1.tel.items():
             tel_index = _get_tel_index(event, tel_id)
 

docs/changes/2357.bugfix.rst

@@ -0,0 +1,2 @@
+Fix ``SoftwareTrigger`` not removing all parts of a removed telescope event
+from the array event leading to invalid files produced by ``DataWriter``.