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

IRF axis order again #102

Open
cdeil opened this issue Jan 18, 2018 · 5 comments
Open

IRF axis order again #102

cdeil opened this issue Jan 18, 2018 · 5 comments

Comments

@cdeil
Copy link
Member

cdeil commented Jan 18, 2018

Friends, I fear we have to discuss IRF axis order again and work on it some more.

This was discussed in #28 and I thought we had fixed / concluded the discussion, basically agreeing to put a "recommended axis order" in the spec (see e.g. current EDISP spec), but also agreeing that we should use the CREF key from the OGIP multi-dimensional dataset FITS recommendation (see here and mention of that in our spec here). The advantage of using CREF is that it's more flexible, someone could change it e.g. for performance reasons, and when adding new IRFs with new axes there's a general scheme to declare in the file, not just in the spec, how to process the IRF.

Currently I see this status:

  • Fermi-LAT and older OGIP data have CREF header keys. @woodmd - do you rely on it in the Fermi ST, i.e. is it required in the Fermi-LAT IRFs?
  • The IRF example files in this spec repo don't. I'll make a pull request to add this shortly.
  • The CTA FITS IRFs (prod 3, 1DC) don't have CREF (cc @jknodlseder @GernotMaier)
  • The HESS FITS IRFs currently don't have CREF (cc @lmohrmann )
  • The MAGIC FITS IRFs currently don't have CREF (cc @TarekHC )
  • Gammapy doesn't support CREF (cc @thomasarmstrong)
  • For Gammalib I don't know. @jknodlseder - does it read and use CREF if present? I think I asked this before and you said "yes"?

What I did was to check AEFF files. They all have this to specify the IRF array shape:

TDIM5   = '(10, 5)'

but they are lacking this to specify the IRF axis order (except for Fermi-LAT and older OGIP IRFs):

 CREF5   = '(ENERG_LO:ENERG_HI,THETA_LO:THETA_HI)'

That example was what should be there for 10 energy bins and 5 theta bins, and following the recommended axis order of energy, offset for AEFF as given here.

Do you agree CREF is useful?
If yes, could you please update your IRF FITS writers and readers?

I'm not sure what to do about the spec. It depends on what people say here. My suggestion would be to add a note that CREF is required from now on for writers, but we keep the recommended axis order in the spec and a suggestion that readers look for CREF first, and if it's missing fall back to the hard-coded recommended order for the current IRFs we have. This means everything keeps working, but we get (in my opinion) better files from now on.

@cdeil
Copy link
Member Author

cdeil commented Jan 23, 2018

@lmohrmann is adding CREF to the HESS FITS exporters now.

Does anyone have thoughts on CREF for IRFs?
Maybe I should just go ahead and make a pull request for your review?

@tony32lin
Copy link

tony32lin commented Jul 23, 2018

@cdeil The CREF keywords is not currently listed as mandatory header keyword here for example (although it is briefly mentioned in the IRF axes section). Maybe it is better to mention these key word in the description for the IRF itself?

@maxnoe
Copy link
Member

maxnoe commented Dec 4, 2020

We need to document this and CREFN is a strange choice for BinTableHDUs, since all metadata concerning columns are stored in keywords called TXXXXN, (TTYPEN, TUNITN etc.)

CUNITN as mentioned in the docs currently is not needed, since this is already covered by TUNITN.

We need to update this for the next version to something we agree on so it can be implemented in the science tools.

So I would propose for bin tables to use TREFN instead of CREFN ( C is for image hdus).

@maxnoe
Copy link
Member

maxnoe commented Sep 17, 2021

Or more descriptive TAORD<N> for Axis order?

@jsitarek
Copy link

jsitarek commented Jan 4, 2024

Hi, I checked how it is nowadays in the MAGIC DL3 files and CREF is filled in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants