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

How to deal with the correction of EDISP calculation #1160

Closed
2 tasks done
morcuended opened this issue Sep 14, 2023 · 7 comments
Closed
2 tasks done

How to deal with the correction of EDISP calculation #1160

morcuended opened this issue Sep 14, 2023 · 7 comments
Assignees

Comments

@morcuended
Copy link
Member

morcuended commented Sep 14, 2023

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 (cta-observatory/pyirf#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:

  • Add directly the correction of EDISP normalization in lstchain v0.9 and make a new lstchain minor release v0.9.14 (Properly normalize edisp in lstchain v0.9 #1164)
  • Update lstchain v0.10 to use pyirf >= 0.10.0 (already including the proper EDISP normalization definition) and make a new minor lstchain release v0.10.5
@moralejo
Copy link
Collaborator

Other opinions?

@rlopezcoto
Copy link
Contributor

rlopezcoto commented Sep 14, 2023

  • for the second case, I would make lstchain compatible with pyirf v0.10 and make a bugfix release (lstchain v0.10.5)
  • for DL1 files produced with lstchain v0.9, we need to decide if it is worth it to introduce that bugfix or wait for the reprocessing with lstchain v0.10 and analyze them with the future lstchain v0.10.5.

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
Copy link
Member Author

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

maxnoe commented Sep 14, 2023

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

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

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

@morcuended
Copy link
Member Author

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

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

No branches or pull requests

5 participants