Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement stereo trigger for subarray triggers #2136

Merged
merged 1 commit into from
Nov 26, 2022
Merged

Conversation

maxnoe
Copy link
Member

@maxnoe maxnoe commented Nov 25, 2022

No description provided.

ctapipe/instrument/trigger.py Outdated Show resolved Hide resolved
"""
Remove telescope events that have not the required number of telescopes of
a given type from the subarray event and decide if the event would
have triggered the stereo trigger.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stereo software? Otherwise this seems very LST-specific.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence refers to the general stereo trigger of the array, which is not LST specific.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I was biased by my other comment. But this leads to another question: when I read "stereo" (instead of "mono") I think of at least two (>=2), this is not needed here, right? By default a single telescope can trigger this SoftwareTrigger?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think that wording is fine: there are 2 triggeres: the subarray trigger (SWAT) and the LST-Hardware Stereo Trigger. Both are examples of stereo triggers, just different implementations.

ctapipe/instrument/trigger.py Outdated Show resolved Hide resolved
- Ignore events with only 1 telescope after this has been applied
"""

min_telescopes = Integer(
Copy link
Contributor

@nbiederbeck nbiederbeck Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not happy with the naming here, as only the help of the other parameter shows that after applying other checks this one is used. I was very confused when I read the tests (which I did before reading this file), that min_telescopes is (intentionally) lower than sum(min_telescopes_of_type)

nbiederbeck
nbiederbeck previously approved these changes Nov 25, 2022
Comment on lines +87 to +89
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice use of sets

This trigger allows it to remove lone LST telescope events from
array events (as required by the hardware stereo trigger in real life),
and to reject events with only one telescope after a subarray has been
selected.
@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Base: 92.75% // Head: 92.76% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (8a49255) compared to base (491d298).
Patch coverage: 95.83% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2136   +/-   ##
=======================================
  Coverage   92.75%   92.76%           
=======================================
  Files         214      216    +2     
  Lines       17947    18045   +98     
=======================================
+ Hits        16647    16739   +92     
- Misses       1300     1306    +6     
Impacted Files Coverage Δ
ctapipe/tools/process.py 86.66% <77.77%> (-1.22%) ⬇️
ctapipe/instrument/trigger.py 96.96% <96.96%> (ø)
ctapipe/instrument/__init__.py 100.00% <100.00%> (ø)
ctapipe/instrument/tests/test_trigger.py 100.00% <100.00%> (ø)
ctapipe/tools/apply_models.py 93.10% <0.00%> (-3.33%) ⬇️
ctapipe/tools/tests/test_apply_models.py 100.00% <0.00%> (ø)
ctapipe/io/tests/test_table_loader.py 99.47% <0.00%> (+0.02%) ⬆️
ctapipe/io/tableloader.py 97.58% <0.00%> (+0.15%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@maxnoe maxnoe merged commit f789773 into master Nov 26, 2022
@maxnoe maxnoe deleted the software_trigger branch November 26, 2022 08:31
@maxnoe maxnoe added this to the v0.18.0 milestone Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants