search

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

`OBS_ID` within IRF headers #132

Open TarekHC opened 5 years ago

TarekHC commented 5 years ago

Hi all,

Question: Why is the OBS_ID allowed in the effective areas, but not in the rest of the IRFs?

The reasoning behind adding OBS_ID to the effective area is "If the effective area corresponds to a given observation". That same reasoning may apply to other IRFs, no?

Should we add a similar text to other IRF components and allow its use?

cdeil commented 5 years ago

Question: Why is the OBS_ID allowed in the effective areas, but not in the rest of the IRFs?

No good reason. Looks like it was added in https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/commit/abeee47250fad24fd65e1c9e54919e876f8a2ffb#diff-f560ad1cc04c0ea518004c6df78b7549R50 without discussion, and then in #128 the OBS_ID header key in the IRFs was changed from required to optional.

The reasoning behind adding OBS_ID to the effective area is "If the effective area corresponds to a given observation". That same reasoning may apply to other IRFs, no?

Yes

Should we add a similar text to other IRF components and allow its use?

The use of optional header keys is always and thus already allowed by the spec.

E.g. in HESS we do add OBS_ID and other information to the other IRFs as well and they are compliant with the current spec.

It's optional to accommodate the use case of using one IRF for multiple observations, like FACT does in the joint Crab data.

If you want, feel free to send a PR clarifying the situation (and if you think it's fine as-is, please close the issue). The trade-off is of course between wanting to be clear in the spec, but also having as little text overall to maintain, so it's just a matter of preference whether to repeat this for all IRF pages, or whether to keep as is or clarify on the AEFF page.

TarekHC commented 5 years ago

The use of optional header keys is always and thus already allowed by the spec.

Right now, the only optional header keyword list I can find (although it's been a while since I played around with the repo, so I'm not 100% sure) is the one within the event list (+ some listed briefly in the effective area text). I think we should not assume that those keywords are also applicable to the IRFs.

Would it make sense to add a list of required/optional header keywords to the (general) IRFs section?

E.g. in HESS we do add OBS_ID and other information to the other IRFs as well and they are compliant with the current spec.

At least in the files within the joint-crab repo, I believe only the effective area contains the OBS_ID keyword (while EDISP nor PSF do). That is fine, I guess, if they are re-used. But the text should be consistent I guess.

cdeil commented 5 years ago

I think the spec is complete concerning what is required. Is that correct or is something required missing?

The description of optional things is certainly minimal and could be improved. It's up to you if you want to spend time / send PRs to improve that aspect of the spec, it's certainly welcome. Just make sure real spec changes are separate in small PRs from (presumably many) better descriptions and notes of optional things.