Skip to content

Commit

Permalink
Fix StereoTrigger non-deterministically discarding LST-1 in prod6 files
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
maxnoe committed May 13, 2024
1 parent 63e915d commit 358955c
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 10 deletions.
6 changes: 6 additions & 0 deletions docs/changes/2552.bugfix.rst
Original file line number Diff line number Diff line change
@@ -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``.
22 changes: 13 additions & 9 deletions src/ctapipe/instrument/trigger.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
"""
Expand All @@ -79,24 +85,22 @@ def __call__(self, event: ArrayEventContainer) -> bool:
"""

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:
for tel_id in tels_in_event:
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
Expand Down
1 change: 0 additions & 1 deletion src/ctapipe/tools/process.py
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,6 @@ def start(self):
disable=not self.progress_bar,
):
self.log.debug("Processing event_id=%s", event.index.event_id)

if not self.event_type_filter(event):
continue

Expand Down

0 comments on commit 358955c

Please sign in to comment.