open-gamma-ray-astro / gamma-astro-data-formats

Data formats for gamma-ray astronomy
https://gamma-astro-data-formats.readthedocs.io
Creative Commons Attribution 4.0 International
29 stars 27 forks source link

Missing effective area suggested units #101

Closed TarekHC closed 6 years ago

TarekHC commented 6 years ago

Hi all,

I just realized in the specs we don't suggest any unit for the effective area (unlike all the rest IRFs).

Shall we suggest to use cm^2? I can do the change in a second if people agree.

cdeil commented 6 years ago

@TarekHC - Yes, we should mention the unit.

I checked one AEFF file from HESS, from MAGIC and from CTA, and they all had "m^2" as unit. (and not "cm^2" as you suggest)

In Gammapy we take the unit from the FITS header, i.e. don't rely an using "m^2" or "cm^2": https://github.com/gammapy/gammapy/blob/cebedcf1f630e2ae84f4f294157951dc759dcefc/gammapy/irf/effective_area.py#L399

@jknodlseder - Can you confirm that Gammalib / ctools expects "m^2" for AEFF?


Also, the example IRF files we have in the spec should be updated to have a unit: https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/blob/master/source/irfs/full_enclosure/aeff/create_example_file.py @TarekHC - I think you wrote that? Note that for Table you can assign quantities as columns and then the unit will be written to the FITS header. Example: https://github.com/gammapy/gammapy/blob/master/examples/example_write_irf.py Would you be willing to update the scripts / example files for IRFs as well? If no, I could put that on my TODO list.

TarekHC commented 6 years ago

I checked one AEFF file from HESS, from MAGIC and from CTA, and they all had "m^2" as unit. (and not "cm^2" as you suggest)

Indeed. I checked later and I saw the m^2. I was lazy to update the issue... :)

Would you be willing to update the scripts / example files for IRFs as well? If no, I could put that on my TODO list.

Sure, no problem. I can do that.

cdeil commented 6 years ago

Just in case the discussion on area units is still open: I vote for "nano-acres"! (Wikipedia)

TarekHC commented 6 years ago

Just in case the discussion on area units is still open: I vote for "nano-acres"! (Wikipedia)

:D I really loved the Power as "Pirate Ninja" unit...

cdeil commented 6 years ago

We're finalising the HESS data release files (for real this time) and I'd like to tag and release a stable version of the data formats we have here.

@TarekHC - Do you have time to add the missing units to the spec and example files for AEFF as discussed above soon, or should I do it?

TarekHC commented 6 years ago

Hi @cdeil,

True, I never got to do it, sorry about that. I am currently on shift, so I doubt I will have the time to focus on it in a 2 weeks time scale.

cdeil commented 6 years ago

I'll do it today. Clear skies!

cdeil commented 6 years ago

I have added the missing unit in the AEFF spec in 4696d82 in master. Everyone uses the same, so this should be uncontroversial, it was just an oversight to not add it when typing the spec.

This is what the current example file generated by source/irfs/full_enclosure/aeff/create_example_file.py contains: https://gist.github.com/cdeil/9b83747502a85c6339c062304aeab00a

There's no unit at all, and I think also the data array isn't stored correctly. Currently there is

TDIM5   = '(5,20)  '

but I think it should be

TDIM5   = '(20,5)  '

Also there's no CREF given, see #102 and similar issues are in the other example scripts that make reference IRF files.

I'll try to find time to write new scripts to generate example IRFs soon, and test that they work with Gammapy & Gammalib and go through a pull request to have them reviewed. But that's separate from this issue which was about AEFF unit.