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
30 stars 27 forks source link

Change PSF table axis order #56

Open jknodlseder opened 8 years ago

jknodlseder commented 8 years ago

For all other IRFs we have ENERG, THETA first for being consistent it should be ENERG, THETA and RAD.

cdeil commented 8 years ago

I think we discussed this before, but now I can't find the issue. Maybe it was via email? Or in HESS?

Currently the psf_table format spec has:

Recommended axis order: RAD, THETA, ENERGY.

This is the same as the axis order chosen in THE OGIP FORMAT FOR RADIAL POINT SPREAD FUNCTION DATASETS which is referenced by the psf_table spec:

The order of RPSF (Rad,θXMA,ϕXMA, E) whereby the radial parameters change fastest, and the energy parameters slowest was chosen to facilitate access for the most common applications: interpolation in θXMA,ϕXMA-space of RPSF vs Rlow,Rhigh arrays. This ordering is further confirmed by the value of the mandatory TDIMnnn keyword for this array (where nnn = 7 in the above example).

It's not clear to me if being consistent with OGIP or other formats in our spec is more important (or if the axis order matters much at all).

@jknodlseder - Do you still think the axis order should be changed here? @lmohrmann @joleroi, all - Thoughts?

jknodlseder commented 8 years ago

Yes, we use the ENERG, THETA, OTHER order for all other IRFs, Fermi-LAT uses the same order, hence I would also follow it through consistently for DL3.

Le 9 juin 2016 à 10:53, Christoph Deil notifications@github.com a écrit :

I think we discussed this before, but now I can't find the issue. Maybe it was via email? Or in HESS?

Currently the psf_table http://gamma-astro-data-formats.readthedocs.io/en/latest/irfs/psf/psf_table/index.html format spec has:

Recommended axis order: RAD, THETA, ENERGY.

This is the same as the axis order chosen in THE OGIP FORMAT FOR RADIAL POINT SPREAD FUNCTION DATASETS http://heasarc.gsfc.nasa.gov/docs/heasarc/caldb/docs/memos/cal_gen_92_020/cal_gen_92_020.html which is referenced by the psf_table spec:

The order of RPSF (Rad,θXMA,ϕXMA, E) whereby the radial parameters change fastest, and the energy parameters slowest was chosen to facilitate access for the most common applications: interpolation in θXMA,ϕXMA-space of RPSF vs Rlow,Rhigh arrays. This ordering is further confirmed by the value of the mandatory TDIMnnn keyword for this array (where nnn = 7 in the above example).

It's not clear to me if being consistent with OGIP or other formats in our spec is more important (or if the axis order matters much at all).

@jknodlseder https://github.com/jknodlseder - Do you still think the axis order should be changed here? @lmohrmann https://github.com/lmohrmann @joleroi https://github.com/joleroi, all - Thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/issues/56#issuecomment-224837038, or mute the thread https://github.com/notifications/unsubscribe/AC2oV9egSqPFvVLltgs5Sx6Qi8BIP2JAks5qJ9SUgaJpZM4Iw6zr.

cdeil commented 8 years ago

@jknodlseder - I found the issue where we discussed this ... in February you commented that following OGIP axis order for RPSF is OK: https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/pull/28#issuecomment-188149267

I don't care either way.

@lmohrmann, @joleroi Do you have an opinion for axis order of RPSF?

lmohrmann commented 8 years ago

No.

cdeil commented 8 years ago

I've made the change in #58. Closing this issue now.

If someone sees this later and disagrees, feel free to comment here or make a new issue proposing another change.

TarekHC commented 5 years ago

Dear @cdeil and @jknodlseder

Sorry to re-open an old issue, but today I bumped with it again, and I must say I don't see the current axis order making much sense.

The way I see it is the following:

Given that usually these IRFs are calculated for a given camera offset, it makes more sense not breaking that order, maintaining the 2 dimensions of the direction dispersion for a given offset together.

What do you think? I don't mind updating the specs, but I want to make sure you both agree, or at least explain why direction and energy dispersions should have a different order?

Thanks!

cdeil commented 5 years ago

@TarekHC - I don't remember if there were small pros / cons for certain axis orders or if what we have now is just what people had implemented and thus we documented it that way in the spec. There seems to be quite a bit of discussion above and in linked issues.

I think basically the order doesn't matter, and we should just stick with what we have now, and then in 2019 design new and better DL3 (there is good reason to think that this will happen in CTA, this was discussed at the CTA SUSS workshop this week), not just in this tiny incremental way of changing an axis order, but allowing for new DL3 based on all the lessons learnt in the past years.

If you want to pursue this change now - could you please make an example file with the axis order you like, and try to read and evaluate it with Gammapy and Gammalib (should be just 2 lines of Python in each case) and see if that works or not. My guess is that for Gammapy this doesn't work yet, because we're not parsing the CREF header key yet that declares the axis order, but I'm not sure.

TarekHC commented 5 years ago

I think basically the order doesn't matter, and we should just stick with what we have now

But below you say that different order will not work for gammapy... So in the end (unfortunately) it does matter.

and then in 2019 design new and better DL3 (there is good reason to think that this will happen in CTA, this was discussed at the CTA SUSS workshop this week), not just in this tiny incremental way of changing an axis order, but allowing for new DL3 based on all the lessons learnt in the past years.

Who will do this? I would say it will be us... Identifying the thinks to improve in the current format (adding more IRFs to improve interpolation, 3D+ effective areas, etc..) but also building over what we have now. And now, we have a format with an axis order that is inconsistent between energy and direction dispersion.

We might want to leave it as it is (I'm ok with that), but we have the risk of not fixing at all these issues (which are in any case small, as long as the science tools read the axis order properly).

If you want to pursue this change now - could you please make an example file with the axis order you like, and try to read and evaluate it with Gammapy and Gammalib (should be just 2 lines of Python in each case) and see if that works or not. My guess is that for Gammapy this doesn't work yet, because we're not parsing the CREF header key yet that declares the axis order, but I'm not sure.

I will soon be able to play around and test this with actual data, so I might take some time, but soon test it both for ctools and gammapy. I will leave the issue open for now.

cdeil commented 5 years ago

But below you say that different order will not work for gammapy... So in the end (unfortunately) it does matter.

Just because the Gammapy IRF classes need a bit of work. This is planned, it will happen in early 2019, but if you want to use VERITAS PSF or whatever you're woking on with Gammapy now, the easiest is to just export to this format and axis order that we have used for the past years.

Who will do this? I would say it will be us...

I was thinking you do the work, and me and a few others complain from the sidelines. OK?

I will leave the issue open for now.

OK. We already have #102 open, but having this issue open as a reminder in addition could be useful.

TarekHC commented 5 years ago

Just because the Gammapy IRF classes need a bit of work. This is planned, it will happen in early 2019, but if you want to use VERITAS PSF or whatever you're woking on with Gammapy now, the easiest is to just export to this format and axis order that we have used for the past years.

Yes, I agree. I will test both formats once I have a decent VERITAS run exported with full-enclosure IRFs.

I was thinking you do the work, and me and a few others complain from the sidelines. OK?

xD Is that what was decided in the CTA SUSS workshop?

OK. We already have #102 open, but having this issue open as a reminder in addition could be useful.

Ok. I leave things as they are then, and bother you again if I feel it should be changed.

cdeil commented 5 years ago

Is that what was decided in the CTA SUSS workshop?

Yes, we all agreed, it's now an official requirement:

CTA-SUSS-42 - Tarek must produce perfect DL3+ data model and formats for CTA by March 2019.