coordinate classes misused in many places

[kosack]

I noticed that a lot of places where ctapipe.coordinate classes are used, they are not used correctly as described in the astropy docs. In the misused cases, the coordinate Frame is used to constuct the coordinates, not the generic SkyCoord class (which is what all coordinates should be).

All coordinates should be astropy.coordinates.SkyCoord instances, and it is not correct to construct the coordinates using the frame directly. The correct way to construct a coordinate (or array of them) is:

az, alt  = 180 * u.deg, 80 * u.deg

pointdir = SkyCoord(az=az, alt=alt, frame='altaz')
tilted = TiltedGroundFrame(pointing_direction=pointdir)

# build a list of coordinates in the GroundFrame:
tel_coords = SkyCoord(x=[-1,0,1], y=[0,1,-1], z=[0,0,0], frame=GroundFrame)

# transform them to our new frame
tel_coords.transform_to(tilted)

# transform to a modified frame:
tilted.pointing_direction = SkyCoord(alt=10*u.deg, az=90*u.deg, frame='altaz')
tel_coords.transform_to(tilted)

We should update all places where coordinates are used to correctly construct them as SkyCoords and keep frames separate, as this may break code in future updates of astropy.

[ParsonsRD]

Hi @kosack, OK apologies thats my fault for not paying attention to the documentation. However, I've got a bit sick of this astropy coordinates code, it's far too slow and it will never be fast as it is rebuilding the frame graph for navigating between systems each time a coordinate object is instantiated. For ctapipe we do not need this functionality and can instead hard code this map simply as a list of systems and their appropriate transformations. This will be much faster than the astropy implementation without much of the other checks and cruft that are slowing it down.

I've been working on a prototype implementation of this for the last couple of days and will issue a pull request for comment soon.

[maxnoe]

Premature optimization is the root of all evil – Don Knuth

Do you really think rewriting the astropy coordinate module for ctapipe is necessary at this point in time?

[ParsonsRD]

No I don't and this is not what I suggest, in fact it's quite the opposite. The vast majority of the functionality and the checks performed by the astropy coordinates are unnecessary, that's why it's so slow. I instead propose to write a bare bones implementation of the coordinate transformations which does what we want and nothing else, is easier to use (for example each coordinate frame should already contain enough information to convert of any other compatible system) and isn't dominated in execution time by either the instantiation of the coordinate frame or checking whether the units we use are compatible.

I agree that premature optimisation is not a great idea, but this code has sat in the repository for > 1 year and isn't used in many sections of the code because it's either too hard to use or far too slow!

[maxnoe]

that's why it's so slow.

That's not correct. The main part why astropy transformations are slow is the ludicrous precision with which the conversions are performed and a missing optimization of the calculation of time dependent parameters.

There is active work done in providing a way to optimize for speed rather precision on micro arc second level, which is discussed here:

astropy/astropy#6068

[kosack]

Good to know they are working on it! I think some of the slowness is also just the overhead of constructing a SkyCoord and related Frames - transforming a single coordinate is quite slow, but transforming a few thousand at once in an array is vastly faster, so there is clearly more to do, but I think we can optimize that later since it's not (yet) the bottleneck. In the end, if it's really a major problem, once we have everything working, we can of course just implement the few transforms we need using cython or numba or something.

[maxnoe]

At the LST Bootcamp, I just realised, that conversions to and from NominalFrame are currently implemented with the "wrong" semantics imho.

As I understood it, there is only one NominalFrame only dependen on array_pointing, and one needs indiviual telescope pointing directions to convert to nominal frame.

If this is correct, NominalFrame should not have a telescope_pointing member.

Also, the transformation from CameraFrame to TelescopeFrame currently is not keeping the pointint_direction, this needs to be fixed.

I will look into this.

[maxnoe]

Should be fixed by merging #896