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

Order of axes in Edisp IRF #178

Open
GernotMaier opened this issue Jun 18, 2021 · 20 comments
Open

Order of axes in Edisp IRF #178

GernotMaier opened this issue Jun 18, 2021 · 20 comments

Comments

@GernotMaier
Copy link

GernotMaier commented Jun 18, 2021

I am trying to understand the axes order in the Edisp IRF compared to all other IRFs with similar axes

For Edisp, we have:

  • ENERG_LO, ENERG_HI
  • MIGRA_LO, MIGRA_HI
  • THETA_LO, THETA_HI
  • ...

while for other IRFs, it is (e.g. for PSF_TABLE):

  • ENERG_LO, ENERG_HI
  • THETA_LO, THETA_HI
  • RAD_LO, RAD_HI
  • ...

I would suggest to have it always in the order energy, offset axis. Any reasons not the change the order of the axis in Edisp?

@maxnoe
Copy link
Member

maxnoe commented Jun 18, 2021

Related: also #102 for defining a solution to define the axis order properly in the header.

But +1 for having the recommended axis order consistent

@TarekHC
Copy link
Member

TarekHC commented Jun 18, 2021

Agree. I would propose changing quickly the current order of EDISP_2D, and solve #102 in the future.

@adonath
Copy link
Collaborator

adonath commented Jun 18, 2021

Just wanted to support the proposal here, it's probably not controversial at all. From the side of the analysis tools having to support the format, it's of course convenient to introduce both changes at the same time, so that there is not need to support an intermediate format, where just the order of the energy dispersion differs. The long-term solution is to solve #102 anyway.

@jknodlseder
Copy link
Collaborator

jknodlseder commented Jun 18, 2021 via email

@adonath
Copy link
Collaborator

adonath commented Jun 18, 2021

@jknodlseder I think its just about the consistency between the IRFs. There is nothing wrong about the actual "global" order of the axes...

@adonath
Copy link
Collaborator

adonath commented Jun 18, 2021

I guess one could even argue, that the current order of the energy dispersion is easier extensible for a future "phi" (angle in the field of view) dependency and maybe the other IRFs should be adapted instead. So the "global order" can be possibly optimised as well...

@maxnoe
Copy link
Member

maxnoe commented Jun 18, 2021

@adonath I disagree, I think the actual IRF axes should always be the last.

so [general binning dims, irf dims].

That allows to be consistent over all the irfs.

@maxnoe
Copy link
Member

maxnoe commented Jun 18, 2021

Separating ENERG from MIGRA would not allow displaying the energy migration matrix using fv for example.

I don't understand this statement. What does the order of the axes have to do with visualization? And what is fv here?

@adonath
Copy link
Collaborator

adonath commented Jun 18, 2021

@maxnoe I actually agree. For me there was just the question whether there is a "natural" global order to the common axes as well, like: [spatial dims, energy, irf dims]

@maxnoe
Copy link
Member

maxnoe commented Jun 18, 2021

@adonath Yes, I think having the energy closest to the irf dims makes sense for many use cases, maybe this is also what @jknodlseder was referring to?

@jknodlseder
Copy link
Collaborator

jknodlseder commented Jun 18, 2021 via email

@maxnoe
Copy link
Member

maxnoe commented Jun 18, 2021

And it doesn't allow choosing which Axes of a row in a binary table should be displayed?

If yes, do we really need to support this use case, if we have lots of other possibilities to inspect and visualize the IRFs?

I think we should value consistency and practicability of the data format over some very specific tool being able to plot the data.

@jknodlseder
Copy link
Collaborator

jknodlseder commented Jun 18, 2021 via email

@maxnoe
Copy link
Member

maxnoe commented Jun 18, 2021

But it has no support for the GADF data formats other than that we use FITS files, and I guess the EDISP matrix is the only IRF that displays nicely. (and that quite by accident as it sounds).

I really don't think that should have an influence on our decision making here.

@maxnoe
Copy link
Member

maxnoe commented Jun 18, 2021

So I thought I was missing out maybe and installed fv. I used the DL3 files from the HESS Data Release 1 used for the open crab paper and pressed the movie button for the EDISP, and a really nice animation appeared going through the single field of view slices showing the dispersion. Great stuff!

Then I tried to do the same with the PSF, and fv segfaulted.

@lmohrmann
Copy link
Collaborator

Then I tried to do the same with the PSF, and fv segfaulted.

I take full responsibility - I did not test these files with fv 😅

@adonath
Copy link
Collaborator

adonath commented Jun 18, 2021

@jknodlseder But seriously there must be the possibility to choose the axis order for the visualisation, or is there any official FITS convention that fixes the visualisation order for ND table data?

@jknodlseder
Copy link
Collaborator

jknodlseder commented Jun 18, 2021 via email

@TarekHC
Copy link
Member

TarekHC commented Jun 21, 2021

I think that this is even a consistent way to store the data: put the response matrix first, then put parameters as outer dimension.

This can only be consistent with the energy dispersion. In turn, this would be inconsistent among the different IRFs.

Even if I also use fv to visualize FITS files many times, I am fully against building a data format thinking about a specific data visualization tool. A different story would be if fv reflects usual standards in astronomy, and therefore it might be a good idea to use a column order similar to what is used over other fields in astronomy. I personally believe the image/video visualization tool was thought as a quick-look tool for sky/spectra images, and was never thought to be used for IRFs.

I just checked LAT IRFs to make sure I was not making a mistake, and every single LAT IRF I found has the following order: ENERGY, THETA, ... (including the energy dispersion). Which is the same standard we are following right now, except for EDISP (as Gernot pointer out in the issue).

So I would personally strongly support the idea of using a consistent column order among IRFs, and as LAT is the closest to us, I would use their same order unless there is a strong justification.

Last comment: If there is consensus ENERGY, THETA, ... is not the right order for EDISP, then we should change other IRFs. But not being consistent among IRFs makes no sense. As the different arguments in favor of leaving EDISP column order as it is now, go against leaving the PSF column order as it is... (e.g. because we cannot plot the PSF with fv).

@maxnoe
Copy link
Member

maxnoe commented Sep 17, 2021

This, by the way, goes also back to #102, as in we shouldn't really prescribe an axis order (or maybe we should) but require using a consistent method the description of the axis order.

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

No branches or pull requests

6 participants