search

cta-observatory / pyirf

Python IRF builder
https://pyirf.readthedocs.io/en/stable/
MIT License
15 stars 25 forks source link

Add cref #275

Closed jsitarek closed 7 months ago

jsitarek commented 8 months ago

I found in GDAF specification: https://gamma-astro-data-formats.readthedocs.io/en/latest/irfs/irf_axes/index.html (see also the example in https://gamma-astro-data-formats.readthedocs.io/en/latest/general/fits-arrays.html#id1) that the axis order of IRFs is not fully fixed, but only recommended, and that the tools using the IRFS should read it from CREF header key, which so far was not filled in GDAF I/O classes of pyirf. I added it for all the IRFs in gadf.py file, and added a test function to validate that all the shapes are consistent

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (e591335) 95.36% compared to head (31a668b) 95.40%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #275 +/- ## ========================================== + Coverage 95.36% 95.40% +0.03% ========================================== Files 60 60 Lines 3109 3135 +26 ========================================== + Hits 2965 2991 +26 Misses 144 144 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

maxnoe commented 8 months ago

Hi @jsitarek,

Thanks, this in general looks fine, but given the discussion in https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/issues/102 and the fact that Gammapy only supports the recommended axis order and the general let's say "under specification" of this in the GADF docs, I am not sure we gain something from it...

jsitarek commented 8 months ago

Thank you @maxnoe for a quick review. The issue that you mentioned is still open since 5 years and in the meantime some of the instruments included this CREF in their output IRF files, so even if gammapy is not using it (yet?) I think it makes sense to have it in the file. This also gives information to people checking the irf file what axis order was really used, and allows consistency checks.

maxnoe commented 8 months ago

@jsitarek Ok. Let's include the change for automatically determining the column index and then include it.

I'll also open an issue with Gammapy.

maxnoe commented 8 months ago

There seems to be an issue with the docs builds unrelated to your changes, I'll investigate and come back yo you

jsitarek commented 7 months ago

There seems to be an issue with the docs builds unrelated to your changes, I'll investigate and come back yo you

Hi @maxnoe, any update on the docs issue? If it is unrelated to these changes, maybe this PR can be merged already?

maxnoe commented 7 months ago

Sorry, looking into it, could reproduce locally. The issue is the downloading using a raw cell.