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

Add cref #275

Merged
merged 4 commits into from
Jan 18, 2024
Merged

Add cref #275

merged 4 commits into from
Jan 18, 2024

Conversation

jsitarek
Copy link
Contributor

@jsitarek jsitarek commented Jan 3, 2024

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

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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              

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

@maxnoe
Copy link
Member

maxnoe commented Jan 3, 2024

Hi @jsitarek,

Thanks, this in general looks fine, but given the discussion in open-gamma-ray-astro/gamma-astro-data-formats#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...

pyirf/io/gadf.py Outdated Show resolved Hide resolved
@jsitarek
Copy link
Contributor Author

jsitarek commented Jan 4, 2024

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

maxnoe commented Jan 4, 2024

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

maxnoe commented Jan 4, 2024

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

@jsitarek
Copy link
Contributor Author

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

maxnoe commented Jan 18, 2024

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

@maxnoe maxnoe merged commit 273afd6 into main Jan 18, 2024
8 of 9 checks passed
@maxnoe maxnoe deleted the add_cref branch January 18, 2024 09:58
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.

2 participants