Update trigger rate derivation use reduced event data list #1685
Conversation
GernotMaier
added
the
no-changelog-needed
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1685 +/- ##
=======================================
Coverage 98.16% 98.16%
=======================================
Files 90 90
Lines 11593 11620 +27
=======================================
+ Hits 11380 11407 +27
Misses 213 213 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
…und-indexes.rst Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
@GernotMaier thanks for the very nice updates. Have just added few minor comments, but nothing blocking to merge.
| Command line arguments | ||
| ---------------------- | ||
| """ | ||
| Define histograms including event data, binning, naming, and labels. | ||
| All histograms are defined for simulated and triggered event (note |
| -1.0 * self.core_distance_bins.max(), | ||
| self.core_distance_bins.max(), | ||
| len(self.core_distance_bins), | ||
| hists["energy"] = self.get_histogram_definition( |
There was a problem hiding this comment.
this looks a bit repetetive, can it be simplified with a dictionary containting the parameters and a loop?
There was a problem hiding this comment.
I thought of that as well while reviewing, but realised that we replace one repetitive code with another one defining the dictionary, so decided to drop it.
| reduced_info["scatter_area"] = self.scatter_area( | ||
| core_scatter_min=reduced_info.get("core_scatter_min", 0.0 * u.m), | ||
| core_scatter_max=reduced_info.get("core_scatter_max", 0.0 * u.m), | ||
| ) |
There was a problem hiding this comment.
Should the default be zero? Can it be derived from the layout instead?
There was a problem hiding this comment.
I've chosen 0 to make sure that the results do not make sense.
Not sure how they can be read from the layout, isn't this a CORSIKA configuration parameter?
There was a problem hiding this comment.
Yes, I guess in this class you can't know the layout. I lost track a bit of which file I am in, I saw above that a layout is used, but it's not transferred to this class. If the idea is to make sure the results don't make sense, then this code does the job.
|
Thanks @tobiaskleiner and @orelgueta for the review. I've updated the code accordingly. Let me know if something is missing. |
|
Add an application to derive single telescope and array trigger rates from reduced event data lists.
This replaces the implementation of
simtools-calculate-trigger-rate, which relies on sim_telarray histograms.Minimal implementation to replace functionality of
simtools-calculate-trigger-rate- at this point we can generate a lot of plots and derive the proton-induced trigger rates (with the additional functionality compared tosimtools-calculate-trigger-ratethat we can do this also for arrays). There are a couple of items missing, which will be added in upcoming pull requests (e.g., derive proton+helium trigger rates; understand output file format; derive errors on trigger rates, etc).Most work in this PR is in filling histograms and plotting them correctly.
Tested this for all telescope types and get:
Konrad's numbers are from https://www.mpi-hd.mpg.de/hfm/CTA/MC/Prod6/Config/Rates/prates.lis
For testing, there are a couple of single-telescope sims available at /lustre/fs22/group/cta/users/maierg/single-telescope-tests/merged_hdf5_files These are quickly made sims and not all parameters might be optimal.
To test, run e.g.
Closes #1001 (this tool now can calculate trigger rates for any array configuration)
Closes #1023 (issue partly outdated, partly covered with this PR)