Implement custom energy-dependent cuts for IRFs and DL3 creations

[gabemery]

These changes introduce the option to provide energy-dependent cuts on gammaness or theta during the IRFs creation. And provides associated metadata in the IRFs and DL3 files.

Why is it needed : Allows to provide energy dependent cuts based on criteria different from gamma efficiency. Such as best sensitivity.

Usage : In the config file

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.51%. Comparing base (5e4f46b) to head (fa3c092).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
+ Coverage   77.19%   77.51%   +0.32%     
==========================================
  Files          22       22              
  Lines        2622     2660      +38     
==========================================
+ Hits         2024     2062      +38     
  Misses        598      598              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Elisa-Visentin]

Thank you @gabemery. Very useful. I was just checking this quickly: there are if clauses inside an elif? (Maybe we can reformulate a bit this and through elifs). As for the new io function, I would suggest passing only the dictionary and string, instead of the whole config, so that we can try to keep backward compatibility (i.e., if the new 'entry' is not in the config, the script can exit w/o erros in the module), but this depends on if we want to create a new major version or not

[gabemery]

Hi @Elisa-Visentin , Thank you for the review. I was also a bit bothered by the elif : if, but I didn't have a clear idea of a better solution.

Currently the structure is:

if global
    apply cut, do metadata
elif energy dependent
    if dynamic
        get dynamic cut and setup metadata
    if custom
        get custom cut and setup meta
    apply energy dependent cut + other common code (35 lines)
else
    handle wrong keyword

To avoid duplicating 35 lines of code, for the energy depedent case, it is needed to have the cuts recovery handled for both before the application.
To handle the wrong key word, everything needs to be handled in the same if, elif, else.

I can think of alternatives, but I am not sure which would be good. To give a few I can invent now :

1. Handle the error first, do only if

if not global or dynamic or custom
    error
if global
    apply cut, do metadata
if dynamic
     get dynamic cut and setup metadata
if custom
    get custom cut and setup meta
if energy dependent (dynamic or custom)
    apply energy dependent cut + other common code

2. Duplicate the code or make a function handling it. Keep current if elif else

if global
    apply cut, do metadata
elif dynamic
    get dynamic cut and setup metadata
    apply energy dependent cut + other common code (35 lines)
elif custom
    get custom cut and setup meta
    apply energy dependent cut + other common code (35 lines)
else
    handle wrong keyword

[gabemery]

As for the new io function, I would suggest passing only the dictionary and string, instead of the whole config,

This is possible without issue, it is only moving config["interpolate_kind"], and config["custom_cuts"] as input to the function instead of config.

so that we can try to keep backward compatibility (i.e., if the new 'entry' is not in the config, the script can exit w/o erros in the module), but this depends on if we want to create a new major version or not

For backward compatibility I think it is already ok, since the new entries are only read in the new io function, and in log.info messages in the if blocks for "custom" cuts.
Also, if the method selected is custom, and the associated entries are missing, we should have an error. And looking into the config in the io function or before would lead to the same failure.

So I think there is nothing to change there.

[Elisa-Visentin]

Hi @Elisa-Visentin , Thank you for the review. I was also a bit bothered by the elif : if, but I didn't have a clear idea of a better solution.

Currently the structure is:

if global
    apply cut, do metadata
elif energy dependent
    if dynamic
        get dynamic cut and setup metadata
    if custom
        get custom cut and setup meta
    apply energy dependent cut + other common code (35 lines)
else
    handle wrong keyword

To avoid duplicating 35 lines of code, for the energy depedent case, it is needed to have the cuts recovery handled for both before the application. To handle the wrong key word, everything needs to be handled in the same if, elif, else.

I can think of alternatives, but I am not sure which would be good. To give a few I can invent now :

1. Handle the error first, do only if

if not global or dynamic or custom
    error
if global
    apply cut, do metadata
if dynamic
     get dynamic cut and setup metadata
if custom
    get custom cut and setup meta
if energy dependent (dynamic or custom)
    apply energy dependent cut + other common code

2. Duplicate the code or make a function handling it. Keep current if elif else

if global
    apply cut, do metadata
elif dynamic
    get dynamic cut and setup metadata
    apply energy dependent cut + other common code (35 lines)
elif custom
    get custom cut and setup meta
    apply energy dependent cut + other common code (35 lines)
else
    handle wrong keyword

Hi, probably the most 'beautiful' solution is the last one, where a new function (which could also be defined inside the main function), so that we have only one if-elif-else statement, but I would suggest to wait for Alessio and Julian before changing this (maybe they have a better solution)

[Elisa-Visentin]

As for the new io function, I would suggest passing only the dictionary and string, instead of the whole config,

This is possible without issue, it is only moving config["interpolate_kind"], and config["custom_cuts"] as input to the function instead of config.

so that we can try to keep backward compatibility (i.e., if the new 'entry' is not in the config, the script can exit w/o erros in the module), but this depends on if we want to create a new major version or not

For backward compatibility I think it is already ok, since the new entries are only read in the new io function, and in log.info messages in the if blocks for "custom" cuts. Also, if the method selected is custom, and the associated entries are missing, we should have an error. And looking into the config in the io function or before would lead to the same failure.

So I think there is nothing to change there.

I have to check this better.... (sorry, I'm quite busy and I'm working on too many things at the same time). Probably we should first fix the nested ifs, to be sure about where we call the involved function: if it is only in the 'elif custom', it should work fine

[jsitarek]

I checked the code and I do not see anything critical there. I left some minor comments and in particular a proposal how to simplify the nested ifs pointed out by @Elisa-Visentin
I would recommend to modify it that way, but it can also be merged as it is without much problem

[jsitarek]

Hi @gabemery
It would be nice to finalize this one and merge it. Please let me know if you will have time to have a look in the comments.

[gabemery]

Hi @jsitarek ,
Yes, sorry for the delay. I am implementing your comments today.

[jsitarek]

thanks a lot @gabemery
I am merging now