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

Field of view coordinates and IRF axis names #13

Merged
merged 1 commit into from
Jun 27, 2018

Conversation

cdeil
Copy link
Member

@cdeil cdeil commented Jun 22, 2018

The spec for field of view coordinates still needs to be written:
http://gamma-astro-data-formats.readthedocs.org/en/latest/info/coordinates.html#field-of-view

Also, I've started to write up recommended names for IRF coordinate axes:
http://gamma-astro-data-formats.readthedocs.org/en/latest/irfs/index.html#irf-axes
This info should probably be moved onto the coordinates page.

The hardest part is to describe / fix the FOV coordinates and names.
I'll make a proposal in the next few days and then we can discuss ...

@cdeil cdeil self-assigned this Dec 9, 2015
@cdeil cdeil added this to the 0.1 milestone Jan 13, 2016
@cdeil cdeil modified the milestones: 0.1, 0.2 Apr 5, 2016
@cdeil cdeil removed the enhancement label Apr 17, 2016
@cdeil
Copy link
Member Author

cdeil commented Apr 29, 2016

Today's APOD is gamma-rays in a "relative RA/DEC" system.
http://apod.nasa.gov/apod/ap160429.html

image

I wonder how exactly they define it. (I didn't look, no time today.)

Made me 😄 and I thought I'd mention it here.
cc @jknodlseder @alziegler

@cdeil
Copy link
Member Author

cdeil commented May 22, 2016

Our proposal (from extensive discussions in HESS) for field of view coordinates would be this:

  • Spherical coordinates, no projection / WCS.
  • One version for RADEC aligned and a separate one for ALTAZ aligned.
  • So FOV_RADEC is just a rotated version of RADEC and FOV_ALTAZ is just a rotated version of ALTAZ
  • One version LON / LAT where the pointing position is on the equator and one THETA / PHI where it is at the pole.

This means there are 8 FOV coordinate columns defined:

FOV_ALTAZ_LON, FOV_ALTAZ_LAT
FOV_ALTAZ_THETA, FOV_ALTAZ_PHI
FOV_RADEC_LON, FOV_RADEC_LAT
FOV_RADEC_THETA, FOV_RADEC_PHI

We drop the current DETX, DETY because it was never well-defined / was used inconsistently in exporters and science tools. (Is this OK or do you want to do something here to avoid breakage?)

We have started to implement this in the HESS exporters and to test this with Astropy / Gammapy.
See example FITS file and script test_astrometric.py here:
https://github.com/cdeil/astrometric_checks

screen shot 2016-05-23 at 00 03 31

@jknodlseder @TarekHC and anyone else that might be interested in FOV coordinates -- thoughts?

Would it be OK if I put this into the spec for now? (Because we need something that's well-defined for HESS now and something is better than nothing) There could then still be other proposals in the coming months and extensive discussions on pros / cons of different definitions for FOV coordinates and how to serialise them for FITS events and IRFs.

@TarekHC
Copy link
Member

TarekHC commented May 24, 2016

Fine by me. I think it is ok if you want to add it to current specs. Other experiments are far behind regarding these issues, so if you find it optimal for HESS, then it will very likely also be optimal for IACTs/CTA.

Although just to make sure. Would dropping DETX/DETY deprecate current science tools? Are they currently using those columns?

@cdeil
Copy link
Member Author

cdeil commented May 24, 2016

Would dropping DETX/DETY deprecate current science tools? Are they currently using those columns?

Yes, Gammapy and ctools use DETX / DETY at the moment for background modeling. But they are using it inconsistently, so it might be a good thing that we introduce completely new, well-defined fields.

I see two concrete options:

A. Don't have DETX / DETY in the spec. Have some short-term pain, science tools need to update.
B. Recommend that data providers fill DETX=FOV_ALTAZ_LON and DETY=FOV_ALTAZ_LAT for a while, explaining that this was the old name and some people filled them differently in the past.

I'm in favour of A, but open to do B or something else if @jknodlseder or others want.

@TarekHC
Copy link
Member

TarekHC commented May 24, 2016

You two are the ones that will very likely need to implement those changes... so I think it is reasonable that you two take that decision.

I would suggest B just because it would be a bit more realistic, given that they are currently in use. Although it is true that A is more formal... And would help to get rid of bad practises.

@lmohrmann
Copy link
Collaborator

@cdeil @jknodlseder

I would like to re-initiate this discussion. The current context is that we're trying to build a background model for HESS and need to decide in which coordinates to store it. I'd like to learn what the science tools expect and see whether we can agree on a common format.

I think we have agreed that the model should be provided in a FoV coordinate system that is simply rotated with respect to some astronomical system. The two options are to align the FoV-system coordinates with the Alt/Az-system coordinates or the Ra/Dec-system coordinates.

My first question to you is: in which of these two versions do ctools and Gammapy expect the background model to be provided? The docs aren't very clear about this in both cases, my suspicion is that Gammapy expects Alt/Az-aligned coordinates and ctools Ra/Dec-aligned coordinates, but I'm not sure.

In case my suspicion is true and the tools expect the model to be provided in different formats, my second question would be whether we can agree on one of the options?

We've discussed this topic in our group here in Erlangen and feel that it would be most convenient to provide the background for each run in Ra/Dec-aligned coordinates, since the data will likely be analyzed in this system as well.

(Note that we construct the background model in an Alt/Az-aligned coordinate system in any case, since systematic effects related to the experiment will be captured best in this way. The question we need to discuss is the coordinate system in which to store the model.)

I'm looking forward to hear your thoughts about this!

@cdeil
Copy link
Member Author

cdeil commented Feb 4, 2018

In Gammapy we currently only support rotationally symmetric background, see here. We had working code that projected Alt/Az aligned background models into sky images, I'm not sure if that was in Gammapy at some point or one of the earlier HESS-internal FITS analysis repos.

My suggestion for the event lists (which is half of the discussion above) is to not store any of those coordinates, it's just a waste of space. Instead implement the FOV coordinates from the event RA/DEC. I think that's still missing in both tools, but in the spec, everything is already well-defined in a good way (e.g. FOV coords are shows as optional here). It would save 20% space for all DL3 FITS data (assuming EVENTS dominate), which something.

For the background models, it's a much more difficult decision.

#73 contains some relevant points - non-radial symmetries in AEFF or BKG or other IRFs are pretty much the same - we don't really know much how they look like or how to handle them in a reasonably efficient way. In #73 different options are discussed, and in addition if you have axes attached to a sky frame, you could also consider just using one of the existing maps formats.

Note that we construct the background model in an Alt/Az-aligned coordinate system in any case, since systematic effects related to the experiment will be captured best in this way. The question we need to discuss is the coordinate system in which to store the model.

If you construct it in ALT/AZ, then it's also immensely helpful to have a standard format to store it in ALT/AZ in a well-defined format. We will need that in CTA anyways (cc @kosack). Otherwise you get a mess where in H.E.S.S. you build and store and check models one way, and then in CTA they do something different and we have to adapt in HESS later. IMO we should try to avoid that and collaborate and find something good now. We can discuss this a bit tomorrow, see Karl's presentation), and see if we can't define some formats short-term that support a few options and the most likely work that is ongoing on non-radial IRF dependencies in HESS & CTA now. Concretely for the two options discussed here that just differ by a rotation, a frame = {'radec, 'altaz'} header key would suffice to support both ways in the format, and it wouldn't be terribly hard (but yes, some work) to implement support for them in the two science tool prototypes, no?

@cdeil cdeil requested review from jknodlseder and lmohrmann June 22, 2018 07:23
@cdeil
Copy link
Member Author

cdeil commented Jun 22, 2018

@lmohrmann - This issue is the last substantial topic to finalise the v0.2 of the spec, that we need for the HESS release. I'd like to propose again to drop DETX and DETY from the HESS events table. IMO it's just a waste of space, and given that we don't have agreement on it's definition (we put ALTAZ aligned, ctools assumes RADEC aligned) it just creates confusion / problems. Both Gammapy and ctools work perfectly fine with EVENTS that don't have a DETX / DETY column.

@TarekHC - Please check what you fill in DETX / DETY for MAGIC and also consider removing it, or document the orientation clearly.

@mackaiver - I see that for FACT you're already not adding these two columns to your DL3 EVENTS files. Well done!

Please review the attached commit, and directly edit via a follow-up commit if something is incorrect or unclear.

For now, I just tried to clearly describe the different FOV coordinates in use, but leave the decision what to use open, stating that this will be made by CTA hopefullly soon (cc @kosack from CTA / ctapipe).

@jknodlseder - Maybe you want to comment on this one as well?

Copy link
Collaborator

@lmohrmann lmohrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with dropping DETX and DETY from the event column in general. Are you 100% sure it's not being used by ctools at the moment though?

putting FOV coordinates in EVENTS, or if they are added, to clearly state how
they are defined. This still leaves the problem with the background models,
in case they are non-radially symmetric. We expect CTA to make a decision and to define
FOV coordinate systems soon, to resolve this issue.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't recommend to not put FOV coordinates in EVENTS. Maybe better to encourage to use unambiguous names such as e.g. FOV_ALTAZ_LON, FOV_ALTAZ_LAT.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -1 on encouraging the use of new column names such as FOV_ALTAZ_LON, FOV_ALTAZ_LAT and starting to add support for those in exporters and Gammapy / ctools.
We weren't able to agree with DETX / DETY even while no-one was using it, so it's highly unlikely that we're going to agree on new column names.
I really suggest to leave this decision to the future and to CTA, we can't resolve this here while the relevant people aren't participating in the discussion and no-one has decision making power.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fine with me!

* Reconstructed event Y-coordinate in detector system
(nominal system, see :ref:`coords-fov`).
See also the `OGIP event list`_ standard.
* Reconstructed field of view Y (see :ref:`coords-fov`).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't DETX and DETY be replaced by the more clearly named FOV_ALTAZ_LON, etc...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above.

@@ -58,7 +58,7 @@ Required columns:
* ``ENERG_LO``, ``ENERG_HI`` -- ndim: 1, unit: TeV
* Reconstructed energy axis
* ``DETX_LO``, ``DETX_HI``, ``DETY_LO``, ``DETY_HI`` -- ndim: 1, unit: deg
* Field of view coordinates binning, see :ref:`coords-fov`
* Field of view coordinates binning (see :ref:`coords-fov`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are of course also somewhat ambiguous, it's not clear along which system these are aligned. Add a note that this should be specified clearly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have written long comments / notes in the general FOV section. Does it really help to partly re-hash those comments here?
If you think so, would you be willing to add a commit to this branch with any text changes you like?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this is probably not necessary. Ignore my comment.

@lmohrmann
Copy link
Collaborator

@cdeil Once we have a decision here, I can remove DETX and DETY from the event columns in the HAP exporter.

@cdeil
Copy link
Member Author

cdeil commented Jun 25, 2018

I'm OK with dropping DETX and DETY from the event column in general. Are you 100% sure it's not being used by ctools at the moment though?

They might be accessed and used when using a bkg3d model. I don't know. Can you or someone in Erlangen try? (I don't have ctools installed and don't want to spend time on it).

@lmohrmann
Copy link
Collaborator

They might be accessed and used when using a bkg3d model. I don't know. Can you or someone in Erlangen try?

We'll have a look

@cdeil
Copy link
Member Author

cdeil commented Jun 25, 2018

Just to comment: on the Gammapy side, the plan is to only access RA / DEC / TIME and to compute FOV coordinates from that, and then to interpret background model DETX / DETY with ALTAZ alignment, i.e. how we fill it at the moment in HESS.
And then once CTA makes a decision (e.g. in 2019), change the supported format and coordinate definitions to that.

@lmohrmann
Copy link
Collaborator

@cdeil It appears ctools does use DETX and DETY, albeit only to compute offset angles that are used to query the IRFs event-wise. Maybe someone involved in ctools development could verify this? As I said, I'd be OK with removing these columns if none of the tools uses them. I'd still vote for including both the Ra/Dec and Alt/Az aligned FOV coordinates in the HESS test release, but that's not at issue here, right?

@cdeil
Copy link
Member Author

cdeil commented Jun 27, 2018

@dtiziani confirmed that ctools runs fine without DETX / DETY present, so I'll remove it from the HESS release files EVENTS tables.

Merging this PR.

@cdeil cdeil merged commit 316f03b into open-gamma-ray-astro:master Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants