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

Move EVENTS header keys from required to optional #116

Closed cdeil closed 5 years ago

cdeil commented 5 years ago

The current EVENTS spec has a ton of required header keys: https://gamma-astro-data-formats.readthedocs.io/en/latest/events/events.html#mandatory-header-keywords

This PR moves the following keys from required to optional:

TELAPSE is superfluous, it's defined as TSTOP - START which are required. So IMO should be optional.

All of the others are "provenance" information, not used by the science tools.

As far as I can see, all of them are problematic and not well defined.

There is no agreement on observing mode values to use, or how to put event classes or what OBJECT to put for survey operations, or what OBSERVER to put when simulating CTA data now. So making those keys required when it's not clear what to put is IMO counter-productive. We can add them back to required when there is a provenance / metadata model for CTA in the future.

Note that very soon all IACTs will release DL3 data and we want to be fully spec compliant, and this question of whether / what to put for these header keys came up.

@cboisson @jknodlseder @kosack @lmohrmann @tony32lin @mackaiver - Please comment.

tony32lin commented 5 years ago

Sounds good to me. Just one small comment, the TELESCOP keyword seems easy to fill. And I feel that having some standardized keyword for getting information about which observatory the file is coming from is a good idea. Maybe there's other concern, but I think leaving it in as mandatory is fine.

cdeil commented 5 years ago

@tony32lin - Agree, I'll put back TELESCOP as required.

@cboisson - Please comment on which you think should be required and optional here, so that we can finalise the data releases.

cdeil commented 5 years ago

Just to be clear: I do think that something like OBS_MODE could be part of the DL3 spec in the future. If CTA has well-defined observing modes and the science tools need to do something to analysis the data, that key has to be there. But maybe the key CTA wants will have a different name, or will not be located in the EVENTS header. So removing it now and having something not as huge with so many required and preliminary keys will be actually help a bit to define something clean and proper in CTA in the coming months / years.

Otherwise it looks like that is already done in this spec, where really it isn't.

cdeil commented 5 years ago

Comments by @cboisson via email:


Also they are informative, and when working on multiwavelength (so with many diferent instruments) it is useful.

cdeil commented 5 years ago

@maxnoe also made it clear in #119 and #120 that `EV_CLAS and EVENT_TYPE need to be explained.

To try to address the comments above by Tony and Tarek, and also the ones by Kai and Catherine, I have made these changes: eef4cc5

I moved the ORIGIN, CREATOR, TELESCOP and INSTRUME back to required, following the comment by Catherine that they are very important provenance infos that should usually be filled. I've clarified why both ORIGIN and TELESCOP are needed, i.e. they can be different.

For EV_CLASS and EVENT_TYPE I have added a very long notes section at the bottom of the page explaining what they are. I strongly feel that EV_CLASS should be optional. I do not think EV_CLASS header key in EVENTS can be the proper solution to the event class / configuration issue at the IACT DL3 level, and making it required now gives that impression. I also don't think EVENT_TYPE with a 32 bit bitfield is a good solution, we might want an INT for this or something else. That was already optional, so left as-is. We could consider removing EV_CLASS and EVENT_TYPE completely from the spec, only leaving the comment that this is future work. But given that we had this event spec for the past years, I don't think it's appropriate to make last-minute changes without a long discussion period again, apart from correcting big mistakes, which IMO making EV_CLASS required without a proper description or agreement what it is was.

For OBS_MODE, I have also left it as optional. the problem is the same as for event class. If you put a required key for somthing important, but then have no definition what values are allowed, or how this info is supposed to be used by science tools, you're just creating a mess. So I've left OBS_MODE as optional, and added an explanation at the bottom of the page that for now it's just provenance data, not used by the science tools, but that very likely in a future version of the spec this would become something required, e.g. if we really support different observation modes, such as e.g. from slewing data from IACTs or also HAWC / Fermi-LAT.

I'm merging this now, so that all of you can have a final look at the events spec at https://gamma-astro-data-formats.readthedocs.io/en/latest/events/events.html before I make the stable version v0.2 Monday evening.

If you have any more suggestions to improve something, or to change something, please open a new issue on Monday latest.