cta-observatory / cta-lstchain

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

How to deal with the correction of EDISP calculation #1160

Closed morcuended closed 11 months ago

morcuended commented 1 year ago

We have to think about how to deal with the correction of EDISP calculation for those analyses using v0.9 (and even v0.10) that are in an advanced state. Fix was introduced in pyirf v0.10.0 (https://github.com/cta-observatory/pyirf/pull/250).

lstchain v0.9 depends on pyirf v0.6 lstchain v0.10 depends on pyirf v0.8

One possibility is to include the correction directly in lstchain and make bugfix releases for lstchain v0.9 y v0.10. For lstchain v0.11, directly use pyirf >=v0.10 with the fix included, and drop the hack included in lstchain.

In this way we will have properly tagged lstchain versions with the EDISP calculation corrected.

Action items:

moralejo commented 1 year ago

Other opinions?

rlopezcoto commented 1 year ago

The solution of analyzing everything with lstchain >= v0.10.5 will be clearner, otherwise I am predicting a mess for which only very expert analyzers will know which version to use

morcuended commented 1 year ago

The solution of analyzing everything with lstchain >= v0.10.5 will be clearner, otherwise I am predicting a mess for which only very expert analyzers will know which version to use

If we can require pyirf v0.10 in lstchain >= v0.10.5 I think this is the best option. Then advocate using lstchain >= v0.10.5 for the ongoing projects.

maxnoe commented 1 year ago

If we can require pyirf v0.10 in lstchain >= v0.10.5 I think this is the best option. Then advocate using lstchain >= v0.10.5 for the ongoing projects.

This would apply updates to the IRF interpolation code, which would be very good to have, but the pyirf API has changed here with the introduction of the more advanced interpolation algorithms.

@RuneDominik can comment what might be needed. I think we can make this API compatible at the lstchain level, so it would be possible.

For lstchain 0.9, I'd indeed propose to post-process the pyirf computed EDISP in lstchain to fix them, I'd be very glad If I don't have to do a whole bug release for an old version of pyirf if the fix here is two lines of code.

RuneDominik commented 1 year ago

Right now, lstchain uses the old interpolation functions e.g. interpolate_effective_area_per_energy_and_fov. Those are no longer existent, they where replaced by estimator objects like the EffectiveAreaEstimator. The needed changes would thus be to add the creation of the respective estimator object and then replace the old function calls with a call to each estimators __call__ interface. This would open the possibility to add some kind of configuration system passing the desired inter- and extrapolator classes to the estimator class. For a quick fix it should suffice to just keep the defaults and thus let extrapolator_cls, interpolator_kwargs and extrapolator_kwargs all be None and use the default interpolator_cls (which should irc correspond to the currently used algorithms).

For additional information on the API consider to build the docs in pyirf #255, I've added some information on the API's usage there, including examples.

rlopezcoto commented 1 year ago

Hi, are there any updates on this issue? we need to make lstchain compatible with pyirf >=v0.10 not to produce buggy IRFs

morcuended commented 11 months ago

Done in #1164 for v0.9.x and #1184 for lstchain v0.10.x