cta-observatory / cta-lstchain

LST prototype testbench chain
https://cta-observatory.github.io/cta-lstchain/
BSD 3-Clause "New" or "Revised" License
22 stars 77 forks source link

Add MC DL2 to point-like IRF notebook #1223

Open morcuended opened 4 months ago

morcuended commented 4 months ago

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.

review-notebook-app[bot] commented 4 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (da4cb5e) 72.66% compared to head (2e4a8d9) 72.67%. Report is 27 commits behind head on main.

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

morcuended commented 4 months ago

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.

morcuended commented 4 months ago

just a reminder that this is ready for review/merging. It is an updated version of the notebook used in the analysis school.

morcuended commented 3 months ago

Many thanks for the comments @chaimain, will work on the changes.

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.

Indeed, I can indicate that they were produced with older software versions and MC productions.

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.

Sure

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.

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.

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)

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.

chaimain commented 3 months ago

Many thanks for the comments @chaimain, will work on the changes.

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.

Indeed, I can indicate that they were produced with older software versions and MC productions.

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.

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.

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.

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.

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)

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.

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.