gamma-astro-data-formats icon indicating copy to clipboard operation
gamma-astro-data-formats copied to clipboard

Change PSF table axis order

Open jknodlseder opened this issue 8 years ago • 11 comments

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

jknodlseder avatar Jun 08 '16 12:06 jknodlseder

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?

cdeil avatar Jun 09 '16 08:06 cdeil

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 [email protected] 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.

jknodlseder avatar Jun 09 '16 09:06 jknodlseder

@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?

cdeil avatar Jun 09 '16 12:06 cdeil

No.

lmohrmann avatar Jun 09 '16 12:06 lmohrmann

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.

cdeil avatar Jun 10 '16 06:06 cdeil

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:

  • For 1D IRFs (e.g. effective area vs energy) indeed the preferred order will be ENERG, THETA.

  • For 2D IRFs (e.g. energy or direction dispersions) the preferred order is currently not consistent.

    • For the energy dispersion is ENERG, MIGRA (the 2 dimensions of the IRF for a given offset) and then THETA.
    • For the direction dispersion is ENERG, THETA, RAD.

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!

TarekHC avatar Dec 06 '18 14:12 TarekHC

@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.

cdeil avatar Dec 07 '18 11:12 cdeil

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.

TarekHC avatar Dec 07 '18 13:12 TarekHC

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.

cdeil avatar Dec 07 '18 13:12 cdeil

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.

TarekHC avatar Dec 07 '18 13:12 TarekHC

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.

cdeil avatar Dec 07 '18 13:12 cdeil