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

Add MC DL2 to point-like IRF notebook #1223

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

morcuended
Copy link
Member

@morcuended morcuended commented Feb 9, 2024

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.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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
@@            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.
📢 Have feedback on the report? Share it here.

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.
@morcuended morcuended added ready for review Pull request is ready for review and removed ready for review Pull request is ready for review labels Feb 12, 2024
@morcuended morcuended marked this pull request as draft February 12, 2024 11:58
@morcuended
Copy link
Member Author

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 morcuended marked this pull request as ready for review February 21, 2024 15:37
@morcuended morcuended added the ready for review Pull request is ready for review label Feb 21, 2024
@morcuended
Copy link
Member Author

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

Copy link
Contributor

@chaimain chaimain left a 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)

@morcuended
Copy link
Member Author

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
Copy link
Contributor

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.

@morcuended morcuended marked this pull request as draft March 18, 2024 09:56
@morcuended morcuended removed the ready for review Pull request is ready for review label Mar 18, 2024
@morcuended morcuended self-assigned this Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants