Use Astropy Coordinate Transofmations For Reconstruction

[kbruegge]

As the title says I refactored the HillasReconstructor to use the astropy coordinate transformations specified in ctapipe.coordinates. The algorithm did not change.

This changes the interface of the predict method slightly. You now need to pass the alt, az of the telescope pointing. Before it was alt and (90 - alt), az I think.

I also deleted some methods that were unused in HillasReconstructor.

[kosack]

Just to be clear before reviewing it all - does this still do the reconstruction as 3D vectors, and not project all cameras onto a nominal plane? I would like to keep it in 3D, since the 2d-style reconstruction is far more complex and error prone and needs special attention for divergent pointing.

If so, thanks for cleaning this up! It's good to start to merge toward consistent coordinate systems.

[kosack]

Also, does it affect the speed much? I know using astropy coords is not fast, which is one reason I had considered dropping them altogether and just writing our own transforms, and only using them to go from alt/az to ra/dec at the end .

[kbruegge]

does this still do the reconstruction as 3D vectors, and not project all cameras onto a nominal plane

Nothing has changed in the algorithms themselves. The transformation from camera coordinate to a eucledian (3D) direction vector was done by the pixel_position_to_direction method before.

[kbruegge]

Also, does it affect the speed much?

It might be slower. I don't know. I'm sure we can optimize that.
In any case right now the h_max prediction takes the longest due to the minimizer being used there.
I have a solution which uses linear least square to do the same calculation. I can add it in a follow-up PR if you like. But this requires some more discussions since we need to come up with a new way to compute h_max. The method here seems not to be working very well. (be it linear least squares or numerical minimization)

[kbruegge]

I know using astropy coords is not fast, which is one reason I had considered dropping them altogether and just writing our own transforms, and only using them to go from alt/az to ra/dec at the end .

I agree astropy coords an units can add quite some overhead. But maybe we should profile that once we get the reconstruction working for all cases (divergent, point-like, diffuse, north, south) as well?

[maxnoe]

Yes, please let's first focus on getting it correct. Astropy units help a lot with that.

Once we verified it's working for all usecases, and only then, we should start optimizing this, potentially removing astropy coordinates and units completely.

But: having it working and tested with astropy units / coordinates means that we can have reliable unit tests before we start removing those safety nets.

build not passing

[kosack]

Looks good and cleans up the code a lot! Please also update the examples and notebooks that use it - that will get the build working: It seems at least these two need small updates:

• examples/plot_theta_square.py
• examples/notebooks/Theta square plot.ipynb

[kosack]

Should we wait for #753 before this is ready to merge? or are they relatively independent?

[maxnoe]

Should be independent.

[kbruegge]

I adapted the script and the notebook. I did not find any other usages. Here's the output of the theta_square script for O(100) events.

[codecov]

Codecov Report

Merging #758 into master will increase coverage by 0.33%.
The diff coverage is 87.96%.

@@            Coverage Diff             @@
##           master     #758      +/-   ##
==========================================
+ Coverage   69.49%   69.82%   +0.33%     
==========================================
  Files         198      197       -1     
  Lines       10708    10648      -60     
==========================================
- Hits         7441     7435       -6     
+ Misses       3267     3213      -54

Impacted Files	Coverage Δ
ctapipe/coordinates/__init__.py	100% <ø> (ø)	⬆️
ctapipe/coordinates/angular_frames.py	79.72% <0%> (+37.77%)	⬆️
ctapipe/reco/tests/test_HillasReconstructor.py	93.44% <100%> (-1.74%)	⬇️
ctapipe/utils/linalg.py	61.11% <33.33%> (-26.77%)	⬇️
ctapipe/reco/HillasReconstructor.py	96.49% <95.65%> (+0.72%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c399a8...cb86311. Read the comment docs.

[kosack]

Ok this looks fine, other than a few unused imports, so I'll merge it shortly