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

OBS_ID within IRF headers #132

Open
TarekHC opened this issue Dec 4, 2018 · 3 comments
Open

OBS_ID within IRF headers #132

TarekHC opened this issue Dec 4, 2018 · 3 comments
Assignees
Labels
Milestone

Comments

@TarekHC
Copy link
Member

TarekHC commented Dec 4, 2018

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 cdeil added the question label Dec 4, 2018
@cdeil cdeil added this to the 0.3 milestone Dec 4, 2018
@cdeil
Copy link
Member

cdeil commented Dec 4, 2018

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 abeee47#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
Copy link
Member Author

TarekHC commented Dec 5, 2018

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

cdeil commented Dec 5, 2018

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.

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

No branches or pull requests

2 participants