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

Update DL3 Tool to to use pyirf IRF interpolation functions #711

Merged
merged 89 commits into from
May 4, 2023

Conversation

chaimain
Copy link
Contributor

@chaimain chaimain commented May 7, 2021

With the upcoming pyirf PR#150 and the merged PR#141, the DL3 Tool needs to be updated according to Max's suggestion in #696.

As soon as the IO functions in pyirf are merged, I will update the new function interpolate_irf in lstchain/irf/hdu_table.py and with the new MC DL2 productions of different zeniths (same RF model) are ready, the functions can be tested and then I can open the PR.

@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Patch coverage: 95.35% and project coverage change: +1.00 🎉

Comparison is base (972d908) 71.76% compared to head (ec65c04) 72.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
+ Coverage   71.76%   72.77%   +1.00%     
==========================================
  Files         120      122       +2     
  Lines       11348    11849     +501     
==========================================
+ Hits         8144     8623     +479     
- Misses       3204     3226      +22     
Impacted Files Coverage Δ
lstchain/tools/lstchain_create_irf_files.py 94.97% <86.36%> (-0.63%) ⬇️
lstchain/tools/lstchain_create_dl3_file.py 92.45% <88.70%> (-2.05%) ⬇️
lstchain/high_level/interpolate.py 93.42% <93.42%> (ø)
lstchain/io/io.py 79.19% <95.23%> (+0.49%) ⬆️
lstchain/high_level/__init__.py 100.00% <100.00%> (ø)
lstchain/high_level/hdu_table.py 95.04% <100.00%> (+0.07%) ⬆️
lstchain/high_level/tests/test_hdus.py 100.00% <100.00%> (ø)
lstchain/high_level/tests/test_interpolate.py 100.00% <100.00%> (ø)
lstchain/reco/tests/test_utils.py 100.00% <100.00%> (ø)
lstchain/reco/utils.py 75.52% <100.00%> (+0.52%) ⬆️
... and 2 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@chaimain chaimain marked this pull request as ready for review June 3, 2021 12:24
@chaimain
Copy link
Contributor Author

chaimain commented Jun 3, 2021

I have separated the interpolation functions in lstchain/irf/interpolate.py. These functions are:

  1. compare_irfs - Compare a given list of IRF files with their metadata, binning and cuts (fixed cuts checked only by checking the assigned metadata). I have to generalize this for energy-dependent cuts as well.
  2. load_irf_grid - Compiles a list of column data from the given list of IRFs which are to be used for interpolation
  3. interpolate_irf - Computes the interpolation from a given list of IRFs and a dict of target values for interpolation along a particular parameter (zenith, azimuth pointing, etc.)

Currently, only effective area and energy dispersion IRFs are interpolated and written into a final IRF.

The IRF Tool is slightly modified to include necessary metadata values for the interpolation.

The DL3 Tool has the following changes:

  1. Instead of providing an input IRF file path, the path where the IRFs are stored (input_irf_path) is to be provided, a glob search pattern (irf_file_pattern) to look up for either a single IRF or multiple for interpolation and the final filename of the interpolated IRF (final_irf_file). This puts the interpolation function as an optional feature.
  2. If the interpolation is to be done only on the zenith pointing, a flag interp_only_zd is to be added. By default, I have put zenith and azimuth as the interpolation parameters.
  3. There are a few checks on the number of IRFs provided. For interpolation, it has to be at least twice the number of interpolation parameters. The given list of IRFs are also compared first to check if the select metadata, event binning and selection are compatible to be used for interpolation.
  4. The target values for interpolation is provided from create_event_list function and then passed into the interpolate_irf function.

For testing these functions, I have temporarily created 2 different IRFs, one with different g/h cut and another with an extrapolated values to check interpolation function.

This PR still needs further cleaning and more improvements.

… outside a given grid, use the point projected to the closest facet on the grid to use the interpolated value of that point
@chaimain chaimain mentioned this pull request Dec 5, 2022
4 tasks
Copy link
Collaborator

@gabemery gabemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First quick pass over the code on my side. I will look more into the complex code later.

lstchain/high_level/__init__.py Outdated Show resolved Hide resolved
lstchain/high_level/__init__.py Outdated Show resolved Hide resolved
lstchain/high_level/__init__.py Show resolved Hide resolved
lstchain/high_level/hdu_table.py Show resolved Hide resolved
lstchain/high_level/hdu_table.py Outdated Show resolved Hide resolved
lstchain/high_level/interpolate.py Outdated Show resolved Hide resolved
lstchain/high_level/interpolate.py Show resolved Hide resolved
lstchain/tools/lstchain_create_dl3_file.py Outdated Show resolved Hide resolved
lstchain/tools/lstchain_create_dl3_file.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@gabemery gabemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed interpolate.py

lstchain/high_level/interpolate.py Show resolved Hide resolved
lstchain/high_level/interpolate.py Outdated Show resolved Hide resolved
lstchain/high_level/interpolate.py Outdated Show resolved Hide resolved
lstchain/high_level/interpolate.py Outdated Show resolved Hide resolved
lstchain/high_level/interpolate.py Outdated Show resolved Hide resolved
lstchain/high_level/interpolate.py Show resolved Hide resolved
lstchain/high_level/interpolate.py Show resolved Hide resolved
lstchain/high_level/interpolate.py Outdated Show resolved Hide resolved
lstchain/high_level/interpolate.py Outdated Show resolved Hide resolved
lstchain/high_level/interpolate.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@gabemery gabemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now went other the vast majority of the code (not always looking into the details).
Once current items are addressed I think the PR will be ready.

lstchain/high_level/tests/test_interpolate.py Outdated Show resolved Hide resolved
lstchain/high_level/tests/test_interpolate.py Show resolved Hide resolved
lstchain/high_level/tests/test_interpolate.py Outdated Show resolved Hide resolved
lstchain/tools/lstchain_create_dl3_file.py Outdated Show resolved Hide resolved
@chaimain chaimain requested a review from gabemery May 4, 2023 09:20
Copy link
Collaborator

@gabemery gabemery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this stage I think the PR is ready for merging.

@rlopezcoto rlopezcoto merged commit e41d81d into master May 4, 2023
@rlopezcoto rlopezcoto deleted the interp_irfs branch May 4, 2023 10:06
@chaimain chaimain restored the interp_irfs branch June 27, 2023 20:08
@chaimain chaimain deleted the interp_irfs branch June 27, 2023 20:19
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.

To compare Zen pointing of an event list and IRF before combining them to make a DL3 file
4 participants