Add optional LST stereo trigger requirement #131

HealthyPear · 2021-05-04T11:03:33Z

Added an LST_stereo option to the analysis configuration file (True, if not specified)
modified protopipe.pipeline.event_preparer so,
- all images from all triggered telescopes from all events are still recorded up to DL1b included (this is needed by the intermediate benchmarking)
- events with telescope trigger pattern #LST_images < min_ntel_LST and <2 other telescope_types are not processed further (min_ntel_LST is set to 2, but is configurable)
- events with telescope trigger pattern #LST_images < min_ntel_LST and >=2 other telescope_types are processed further by removing the single-LST image and provided that the remaining images satisfy the quality cuts for the shower geometry reconstructor

HealthyPear · 2021-05-04T11:05:48Z

@moralejo and @GernotMaier could you please check one last time if the PR description reflects what is the correct final result of this modification?

codecov · 2021-05-04T11:22:12Z

Codecov Report

Merging #131 (4d4a6b5) into master (10a75a2) will increase coverage by 0.46%.
The diff coverage is 86.48%.

@@            Coverage Diff             @@
##           master     #131      +/-   ##
==========================================
+ Coverage   59.88%   60.35%   +0.46%     
==========================================
  Files          23       23              
  Lines        2064     2101      +37     
==========================================
+ Hits         1236     1268      +32     
- Misses        828      833       +5

Impacted Files	Coverage Δ
protopipe/pipeline/event_preparer.py	`70.15% <86.48%> (+2.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10a75a2...4d4a6b5. Read the comment docs.

moralejo · 2021-05-04T11:23:44Z

protopipe/pipeline/event_preparer.py

@@ -790,11 +799,7 @@ def prepare_event(self, source, return_stub=True, save_images=False, debug=False
            if self.LST_stereo and (n_triggered_LSTs < self.min_ntel_LST) and (n_triggered_LSTs != 0) and (not_LST_triggered_telescopes >= 2):


The condition can be simpler I think. You have to set good_for_reco to 0 for all LSTs whenever
self.LST_stereo and (n_triggered_LSTs < self.min_ntel_LST)

I'd say the other two conditions can be removed. Besides, the last one is misleading (do you mean "non-LST"?) Anyway, if there is only one telescope left, there won't be stereo reco, why do you need to check for it here?

yes I mean non-LST

the >0 or !=0 is needed I'd say otherwise there is no point in looking for LSTs tel_ids no?

this check will never catch cases in which there is only 1 tel_id left (before taking into account the quality of each image which happens just after) because of the last condition

the check wich catches 1 telescope left (in general) is the next one

self.event_cutflow.cut("min2Tels reco", n_tels["GOOD images"])

It is already taken into account in the repository issues

kosack

This is far too complex for a simple cut on the stereo condition. Please implement it as a single function that returns true/false if the LST stereo trigger condition is met, and use sets inside rather than lots of loops. Then you can just cut on the results of that function.

kosack · 2021-05-11T11:16:00Z

protopipe/pipeline/event_preparer.py

+            # - >=2 any other telescope type,
+            # we remove the single-LST image and continue reconstruction with
+            # the images from the other telescope types
+            if self.LST_stereo and (n_triggered_LSTs < self.min_ntel_LST) and (n_triggered_LSTs != 0) and (non_LST_triggered_telescopes >= 2):


Perhaps rename non_LST_triggered_telescopes to n_triggered_non_LST - it confused me when reading it originally, since I read it as n_LST_triggered_telescopes.

kosack · 2021-05-11T11:36:59Z

protopipe/pipeline/event_preparer.py

+            subarray = source.subarray
+            for i, tel_type in enumerate(subarray.telescope_types):
+                tel_type_name = f"{subarray.telescope_types[i].type}_{subarray.telescope_types[i].optics.name}_{subarray.telescope_types[i].camera.camera_name}"
+                tels_with_trigger = event.trigger.tels_with_trigger


Just a comment: event.trigger_tels_with_trigger is the list of telscopes that triggered, not the list of the list of telescopes that read out, which may not be the same due to deadtime. So you should use event.dl1.tel.keys() (or DL0) instead

At that point fo the code DL1 should not be filled yet - perhaps event.r0.tel.keys() would be safer?

@moralejo I apologize if it sounds obvious....

In your (and I guess also Gernot's) analysis do you consider for this cut the number of telescopes with R0 data (in your equivalent data format of course) or the number of originally triggered telescopes?

I guess telescopes with R0 data

kosack · 2021-05-11T11:43:05Z

protopipe/pipeline/event_preparer.py

+            for i, tel_type in enumerate(subarray.telescope_types):
+                tel_type_name = f"{subarray.telescope_types[i].type}_{subarray.telescope_types[i].optics.name}_{subarray.telescope_types[i].camera.camera_name}"
+                tels_with_trigger = event.trigger.tels_with_trigger
+                tels_ids = source.subarray.get_tel_ids_for_type(tel_type_name)


This is a huge amount of code to implement a very simple cut, so I would suggest using sets to reduce nearly all of this code to a few lines, and ideally put those lines of code in a function. For example (this is not a working example, just an idea of how you would start):

lst_tel_ids = set(source.subarray.get_tel_ids_for_type("LST_LSTCam")) triggered_lsts = set(event.trigger.tels_with_trigger).intersection(lst_tel_ids) n_non_lsts = len(event.trigger.tels_with_trigger) - len(triggered_lsts) ... if not meets_lst_stereo_trigger_condition(triggered_lsts, n_non_lsts): # remove the LSTs good_for_reco = good_for_reco - triggered_lsts # using sets you can just subtract the telescopes ...

using sets is indeed cleaner...

though I don't think the function is really needed since the call to cutflow will take care of both the check and the book-keeping

also I do not want to remove keys from good_for_reco, I set those values to 0 since I do not use them in shower geometry

I still have to record them to DL1

kosack · 2021-05-11T15:51:19Z

protopipe/pipeline/event_preparer.py

+                        + bcolors.ENDC
+                    )
+                for tel_id in triggered_LSTs:  # in case we test for min_ntel_LST>2
+                    if good_for_reco[tel_id]:


I think this is not needed: just set every telescope in triggers_LSTs to 0 - it doesn't matter if it was true before.

What should good_for_reco be in the end? a variable-length list of tel_ids? or a boolean array of tel indices? Right now it is a dict, but it seems you encode both the tels with data, and which ones pass the cuts in it at the same time, which is why it is confusing. I'd suggest just storing those separately if you need both. In ctapipe, we automatically convert telescope lists into boolean arrays so they can be stored in the hdf5 output without using var-length arrays (SubarrayDescription provides a method for that:)

In [3]: source.subarray.tel_ids_to_mask([1,3,4]) Out[3]: array([ True, False, True, True, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False, False]) )

If it's a boolean mask like that, you can get rid of this explicit loop and just use array indexing or set subtraction:

good_for_reco[subarray.tel_ids_to_indices(list(triggered_LSTs))] = False

This way you remove this explicit loop entirely

good_for_reco is not a boolean mask, but a dictionary where the keys are tel_ids (only the ones related to that particular event) and the values are 1 if the cleaned+parametrized image passed all quality cuts, 0 otherwise.
It is then written to file

honestly, I would avoid trying to make this more fancier now considering that soon it will all disappear, unless it it clear that there is a bug due to how I wrote it

so basically what is happening here is that I set the value to 0 only if the image passed the quality cuts (otherwise it is already been set to 0) thus I "remove" the single-LST image by not using it later for shower geometry reconstruction, but since I need to process it up to DL1b for the benchmarking I still need to know (via my dictionary of labels "good_for_reco") if it would have been used in other circumstances

ok, if this is not meant to be long-term code, fine. This was just so that you could write out the used telescope pattern as we do in ctapipe, but maybe that is not needed since we will soon move to ctapipe's version.

I was more thinking about doing it right here, so we can just move the code to ctapipe later.

HealthyPear added 2 commits May 4, 2021 12:55

add LST_stereo mode to the analysis configuration file

6ecb0b6

update event_preparer accordingly

0fb975a

HealthyPear added the enhancement New feature or request label May 4, 2021

HealthyPear added this to the v0.5.0 milestone May 4, 2021

HealthyPear requested a review from kosack May 4, 2021 11:06

Set LST_stereo to True if not specified in the configuration file

edd137b

moralejo reviewed May 4, 2021

View reviewed changes

HealthyPear added 3 commits May 4, 2021 13:37

not_LST_triggered_telescopes to non_LST_triggered_telecopes

22304dc

Update documentation

e76abf2

Remove section proposal for improvement

bda9593

It is already taken into account in the repository issues

HealthyPear linked an issue May 10, 2021 that may be closed by this pull request

Add option to cut mono-tel_type events #130

Closed

HealthyPear added 3 commits May 11, 2021 09:36

Merge branch 'master' into feature_add-LST-stereo-trigger-option

da24201

Add concentration dict to stub event in case of bad_LST_stereo

8bbda29

Update from master and solve merge conflicts

b9a19b6

kosack requested changes May 11, 2021

View reviewed changes

HealthyPear added 2 commits May 11, 2021 15:28

Use sets for triggered telescopes with data

390d744

Fix bug and improve debug printed information

be46eb9

HealthyPear requested review from moralejo and kosack May 11, 2021 15:04

kosack requested changes May 11, 2021

View reviewed changes

HealthyPear requested a review from kosack May 11, 2021 16:49

Merge branch 'master' into feature_add-LST-stereo-trigger-option

4d4a6b5

kosack approved these changes May 12, 2021

View reviewed changes

HealthyPear merged commit f3ccc76 into cta-observatory:master May 12, 2021

HealthyPear deleted the feature_add-LST-stereo-trigger-option branch May 12, 2021 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional LST stereo trigger requirement #131

Add optional LST stereo trigger requirement #131

HealthyPear commented May 4, 2021

HealthyPear commented May 4, 2021

codecov bot commented May 4, 2021 •

edited

Loading

moralejo May 4, 2021

HealthyPear May 4, 2021

HealthyPear May 4, 2021 •

edited

Loading

kosack left a comment

kosack May 11, 2021

kosack May 11, 2021

HealthyPear May 11, 2021

HealthyPear May 11, 2021 •

edited

Loading

kosack May 11, 2021

HealthyPear May 11, 2021

HealthyPear May 11, 2021

kosack May 11, 2021

kosack May 11, 2021

HealthyPear May 11, 2021 •

edited

Loading

HealthyPear May 11, 2021

kosack May 12, 2021 •

edited

Loading

		@@ -790,11 +799,7 @@ def prepare_event(self, source, return_stub=True, save_images=False, debug=False
		if self.LST_stereo and (n_triggered_LSTs < self.min_ntel_LST) and (n_triggered_LSTs != 0) and (not_LST_triggered_telescopes >= 2):

Add optional LST stereo trigger requirement #131

Add optional LST stereo trigger requirement #131

Conversation

HealthyPear commented May 4, 2021

HealthyPear commented May 4, 2021

codecov bot commented May 4, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HealthyPear May 4, 2021 • edited Loading

Choose a reason for hiding this comment

kosack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HealthyPear May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HealthyPear May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kosack May 12, 2021 • edited Loading

Choose a reason for hiding this comment

codecov bot commented May 4, 2021 •

edited

Loading

HealthyPear May 4, 2021 •

edited

Loading

HealthyPear May 11, 2021 •

edited

Loading

HealthyPear May 11, 2021 •

edited

Loading

kosack May 12, 2021 •

edited

Loading