Repository: cta-observatory/ctapipe
Pull request #2552 (2024-04-29 16:35:29)

Fix StereoTrigger non-deterministically discarding LST-1 in prod6 files
This was one of the weirder bugs I have ever tracked down. The symptom was that in consecutive runs of `ctapipe-process` on the same input file, sometimes LST-1 was missing from the output completely.

Tracking it down to the SoftwareTrigger class, which used a mixture of using ``TelescopeDescription`` and its string representation for storing and looking up things.

The dict `_ids_by_type` was the culprit, its content depended on the hash of str(TelescopeDescription), which was random, since hash of strings are random for security reasons in python.

ctao-dpps-sonarqube[bot]: ![Failed](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/QualityGateBadge/failed.svg?sanitize=true)

- B Maintainability Rating on New Code (is worse than A)

# Analysis Details
## 1 Issue
- ![Bug](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/bug.svg?sanitize=true) 0 Bugs
- ![Vulnerability](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/vulnerability.svg?sanitize=true) 0 Vulnerabilities
- ![Code Smell](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/code_smell.svg?sanitize=true) 1 Code Smell

## Coverage and Duplications
- ![Coverage](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/CoverageChart/100.svg?sanitize=true) 100.00% Coverage (94.10% Estimated after merge)
- ![Duplications](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/Duplications/3.svg?sanitize=true) 0.00% Duplicated Code (0.60% Estimated after merge)

**Project ID:** cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

[View in SonarQube](https://sonar-cta-dpps.zeuthen.desy.de/dashboard?id=cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs&pullRequest=2552)


Tobychev: Can you think of a test that could catch this problem?

Telescopes being silently dropped from the trigger is quite serious, and worrying it was not caught for two years.
maxnoe: This condition only happens with the new configuration of Prod 6, before all 4 LSTs were identical.

We haven't really worked on anything Prod 6 yet, it's not even finished simulating.
maxnoe: I'd prefer to fixing this the way I do it here as the other change would require us to do a 0.22 and adapt configuration files.

With the fix at it is now, we can just publish an 0.21.1 without adapting configuration files.

I will add a test that tests for the bug.
ctao-dpps-sonarqube[bot]: ![Passed](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/QualityGateBadge/passed.svg?sanitize=true)

# Analysis Details
## 1 Issue
- ![Bug](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/bug.svg?sanitize=true) 0 Bugs
- ![Vulnerability](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/vulnerability.svg?sanitize=true) 0 Vulnerabilities
- ![Code Smell](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/code_smell.svg?sanitize=true) 1 Code Smell

## Coverage and Duplications
- ![Coverage](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/CoverageChart/100.svg?sanitize=true) 100.00% Coverage (94.20% Estimated after merge)
- ![Duplications](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/Duplications/3.svg?sanitize=true) 0.00% Duplicated Code (0.60% Estimated after merge)

**Project ID:** cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

[View in SonarQube](https://sonar-cta-dpps.zeuthen.desy.de/dashboard?id=cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs&pullRequest=2552)



Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 7654d368..c3ab1609 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -179,3 +179,18 @@ jobs:
 
       - name: Build docs
         run: make doc
+
+      - name: Deploy to github pages
+        # only run on push to master
+        if: ${{ github.event_name == 'push' && github.ref == 'refs/heads/master' }}
+        uses: JamesIves/github-pages-deploy-action@v4.6.0
+        with:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+          BRANCH: gh-pages
+          FOLDER: docs/_build/html
+          CLEAN: true
+          SINGLE_COMMIT: true
+      - uses: actions/upload-artifact@v4
+        with:
+          name: docs
+          path: docs/_build/html
diff --git a/docs/changes/2552.bugfix.rst b/docs/changes/2552.bugfix.rst
new file mode 100644
index 00000000..b56894f9
--- /dev/null
+++ b/docs/changes/2552.bugfix.rst
@@ -0,0 +1,6 @@
+Fix ``SoftwareTrigger`` not correctly handling different telescope
+types that have the same string representation, e.g. the four LSTs
+in prod6 files.
+
+Telescopes that have the same string representation now always are treated
+as one group in ``SoftwareTrigger``.
diff --git a/src/ctapipe/instrument/trigger.py b/src/ctapipe/instrument/trigger.py
index 33c3b3d1..f5c519ee 100644
--- a/src/ctapipe/instrument/trigger.py
+++ b/src/ctapipe/instrument/trigger.py
@@ -59,10 +59,16 @@ class SoftwareTrigger(TelescopeComponent):
     def __init__(self, subarray, *args, **kwargs):
         super().__init__(subarray, *args, **kwargs)
 
-        self._ids_by_type = {
-            str(type): set(self.subarray.get_tel_ids_for_type(type))
-            for type in self.subarray.telescope_types
-        }
+        # we are grouping telescopes by the str repr of the type
+        # this is needed since e.g. in prod6, LST-1 is slightly different
+        # from LST-2 to LST-4, but we still want the trigger to work with all
+        # LSTs
+        self._ids_by_type = {}
+        for tel in self.subarray.telescope_types:
+            tel_str = str(tel)
+            if tel_str not in self._ids_by_type:
+                self._ids_by_type[tel_str] = set()
+            self._ids_by_type[tel_str].update(self.subarray.get_tel_ids_for_type(tel))
 
     def __call__(self, event: ArrayEventContainer) -> bool:
         """
@@ -79,16 +85,14 @@ class SoftwareTrigger(TelescopeComponent):
         """
 
         tels_removed = set()
-        for tel_type in self.subarray.telescope_types:
-            tel_type_str = str(tel_type)
-            min_tels = self.min_telescopes_of_type.tel[tel_type_str]
+        for tel_type, tel_ids in self._ids_by_type.items():
+            min_tels = self.min_telescopes_of_type.tel[tel_type]
 
             # no need to check telescopes for which we have no min requirement
             if min_tels == 0:
                 continue
 
             tels_with_trigger = set(event.trigger.tels_with_trigger)
-            tel_ids = self._ids_by_type[tel_type_str]
             tels_in_event = tels_with_trigger.intersection(tel_ids)
 
             if len(tels_in_event) < min_tels:
@@ -96,7 +100,7 @@ class SoftwareTrigger(TelescopeComponent):
                     self.log.debug(
                         "Removing tel_id %d of type %s from event due to type requirement",
                         tel_id,
-                        tel_type_str,
+                        tel_type,
                     )
 
                     # remove from tels_with_trigger
diff --git a/src/ctapipe/tools/process.py b/src/ctapipe/tools/process.py
index 484a42f0..ef272346 100644
--- a/src/ctapipe/tools/process.py
+++ b/src/ctapipe/tools/process.py
@@ -301,7 +301,6 @@ class ProcessorTool(Tool):
             disable=not self.progress_bar,
         ):
             self.log.debug("Processing event_id=%s", event.index.event_id)
-
             if not self.event_type_filter(event):
                 continue
 


Test patch:
diff --git a/src/ctapipe/instrument/tests/test_trigger.py b/src/ctapipe/instrument/tests/test_trigger.py
index 865f4c68..f188b32a 100644
--- a/src/ctapipe/instrument/tests/test_trigger.py
+++ b/src/ctapipe/instrument/tests/test_trigger.py
@@ -1,10 +1,12 @@
 import json
+from copy import deepcopy
 
 import numpy as np
 import pytest
 from numpy.testing import assert_equal
 
 from ctapipe.containers import ArrayEventContainer
+from ctapipe.instrument import SubarrayDescription
 from ctapipe.io import EventSource
 
 
@@ -235,3 +237,68 @@ def test_software_trigger_simtel_process(tmp_path):
         )
 
     assert len(events_no_trigger) > len(events_trigger)
+
+
+def test_different_telescope_type_same_string(subarray_prod5_paranal):
+    """
+    Test that for a subarray that contains two different telescope types
+    with the same string representation, the subarray trigger is working
+    as expected, treating these telescopes as a single group.
+    """
+    from ctapipe.instrument.trigger import SoftwareTrigger
+
+    lst234 = deepcopy(subarray_prod5_paranal.tel[2])
+    # make LST-1 slightly different from LSTs 2-4
+    lst1 = deepcopy(subarray_prod5_paranal.tel[1])
+    lst1.optics.mirror_area *= 0.9
+
+    assert lst1 != lst234
+
+    subarray = SubarrayDescription(
+        name="test",
+        tel_positions={
+            tel_id: subarray_prod5_paranal.positions[tel_id] for tel_id in (1, 2, 3, 4)
+        },
+        tel_descriptions={
+            1: lst1,
+            2: lst234,
+            3: lst234,
+            4: lst234,
+        },
+        reference_location=subarray_prod5_paranal.reference_location,
+    )
+
+    trigger = SoftwareTrigger(
+        subarray=subarray,
+        min_telescopes=2,
+        min_telescopes_of_type=[
+            ("type", "*", 0),
+            ("type", "LST*", 2),
+        ],
+    )
+
+    # all four LSTs, nothing to change
+    event = ArrayEventContainer()
+    event.trigger.tels_with_trigger = [1, 2, 3, 4]
+    assert trigger(event)
+    assert_equal(event.trigger.tels_with_trigger, [1, 2, 3, 4])
+
+    # two LSTs, nothing to change
+    event = ArrayEventContainer()
+    event.trigger.tels_with_trigger = [1, 2]
+    assert trigger(event)
+    assert_equal(event.trigger.tels_with_trigger, [1, 2])
+
+    # two LSTs, nothing to change
+    event = ArrayEventContainer()
+    event.trigger.tels_with_trigger = [1, 3]
+    assert trigger(event)
+    assert_equal(event.trigger.tels_with_trigger, [1, 3])
+
+    # one LST, remove
+    event = ArrayEventContainer()
+    event.trigger.tels_with_trigger = [
+        1,
+    ]
+    assert not trigger(event)
+    assert_equal(event.trigger.tels_with_trigger, [])