-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add MC DL2 to point-like IRF notebook #1223
base: main
Are you sure you want to change the base?
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1223 +/- ##
==========================================
+ Coverage 72.66% 72.67% +0.01%
==========================================
Files 132 132
Lines 13648 13655 +7
==========================================
+ Hits 9917 9924 +7
Misses 3731 3731 ☔ View full report in Codecov by Sentry. |
This reverts commit da4cb5e. By mistake I committed the new notebook with the same name as the original notebook MC_DL2_to_IRF.ipynb whereas I intended to upload a new file including pointlike in the filename.
Updated the notebook example on pointlike IRF creation, clearing the outputs. I also leave the old notebook on IRF creation (also describing full-enclosure IRFs) untouched. Ready for review. |
just a reminder that this is ready for review/merging. It is an updated version of the notebook used in the analysis school. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If we would like to keep the older notebooks (just for the extra Background IRF plots...?), it would be helpful to mention explicitly that these are produced with older MC production and lstchain/pyirf versions. It would also help (a general/fresh user) to understand or hint at, why we do not have some of the simulations.
- Can you also mention explicitly that these IRFs are for source-independent analysis? We may link to an updated notebook discussing the source-dependent analysis files later.
- Can you also explain why we need to produce IRFs for all nodes of a declination line? The answer lies in providing the full scope for IRF interpolation. But maybe a smarter analyzer can avoid some nodes when using them for analyses of specific sources.
- Maybe it is trivial, but it might be useful to explain empty IRF bins (in energy axes) for some IRFs (zenith-dependence and usage of over-encompassing range in the common config)
Many thanks for the comments @chaimain, will work on the changes.
Indeed, I can indicate that they were produced with older software versions and MC productions.
Sure
Yes, I can mention this. The idea of having the IRF produced for all nodes was for simplicity and to have IRFs ready for "any source". Indeed you will not use all nodes for a specific source, but looking for the closest MC nodes, to interpolate later requires an extra step that it's not trivial in my opinion. Side comment: I've noticed sometimes that for some data, no simplex in which IRFs are interpolated can be found and the nearest node is used. I can also mention that this can happen.
Ok, is there something that can be done in these cases? Like redefining the energy binning in the irf config? Later in Gammapy, you redefine the energy axes anyway. |
Can you clarify again why we need to see the older (obsolete) Background IRF plots? The Full-Enclosure contains PSF, and in that notebook, I had not produced or shown the plots. Maybe we can just link to the Gammapy docs (For example CTA with Gammapy or the v1.1 version of it) for the users to see what FE IRFs for CTA would look like.
I was thinking more in the case, where an analyzer selects a particular declination line of testing MC, for analyses of some source/s, where usually we have some zenith (or maybe azimuth as well) limits. For such case-by-case analysis, the analyzer need not concern with interpolated IRFs "far beyond" the expected/selected range.
In principle, yes the analyzer may change the irf_dl3_tool_config file to be as required. However, there is no problem with any (or most) high-level analysis with Gammapy when such empty bins exist. So, for any general analyses, there should be no problem with working with the default setting. See #1159 for possible confusion an analyzer might face when seeing the empty bins. A concern may be on the interpolations within a grid of IRFs with different energy ranges (of the MC events), around discrete changes (for example at 52.5 deg zenith). This is not a significant problem as the interpolation code considers the empty IRF bins to have value as 0, making it mathematically possible to continue with the interpolation process. |
With this PR I intend to upload a new notebook (
MC_DL2_to_pointlike_IRF.ipynb
) focusing on the point-like IRF production as opposed to the existing notebook that describes the full-enclosure IRFs (including DL2 diffuse gammas, protons and electrons)MC_DL2_to_IRF.ipynb
. I accidentally uploaded the new notebook with the same name as the old one, which was not my intention. Here I also revert that commit that overwrote the old notebook.The old notebook is no longer working with the current lstchain version anyway. Still, I did not want to modify it because its content on full-enclosure IRFs, which I find interesting to keep, although it should be updated somewhere else.