Impact intersection

[kbruegge]

While discussing with @TarekHC, @moralejo and @kpfrang I also came up with a fix for the impact parameter. A quick comparison between #777 and this PR on a small sample (1000 events) can be seen below.

I will try and get more statistics next week.

[kbruegge]

I added the same method to estimate max height. Seems to be better than the current method. At least on a small sample (1000 events). Also it is much faster since its only matrix operations.

[kbruegge]

Alright I added some changes to make this work properly. Note that I fixed the coordinate conversion by changing the sign of the azimuth direction when conversion to cartesian. Since the astropy convention is spinning in the other direction.

See the attached plots. Where I compare old reconstrunction (orange )to this fix (yellow).

[TarekHC]

Hi @mackaiver

With old reconstrunction method you mean before the fix? It probably makes more sense to compare the method you implemented with the one @kpfrang fixed in #777 and #785. The changes are so minimal, that it would probably be easy to add them and compare the methods again.

[kbruegge]

@TarekHC @kpfrang did the plots you asked for. For now just ~3000 events each.

Runtime on my machine is improving by about 50% for the matrix method.

[kbruegge]

I guess the optimal solution would be to put the conversion between clockwise and counter clockwise somehere here

ctapipe/ctapipe/coordinates/angular_frames.py

Line 109 in 72d9b90

class HorizonFrame(BaseCoordinateFrame):

[kosack]

So what is the final decision about these? Will somebody make the change in the coordinate code, or should we merge one of these PRs? If not, we should close these ones. I guess now that the coordinates are done in a more central way, it might be better to do it there rather than to hack minus signs in several places, but it's up to you which you think is cleaner.

[kbruegge]

I think it should be fixed in the HorizonFrame class. This might take some time however to test it. Maybe we can also discuss next week in person.

[codecov]

Codecov Report

Merging #778 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #778      +/-   ##
==========================================
+ Coverage   71.78%   71.78%   +<.01%     
==========================================
  Files         202      202              
  Lines       10968    10967       -1     
==========================================
  Hits         7873     7873              
+ Misses       3095     3094       -1

Impacted Files	Coverage Δ
ctapipe/reco/HillasReconstructor.py	97% <100%> (+0.5%)	⬆️
ctapipe/reco/tests/test_HillasReconstructor.py	94.59% <100%> (+1.15%)	⬆️

---

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 b40b1de...adb79b5. Read the comment docs.

[kosack]

I'd like to at least merge this one (which seems more general than #785, even if not as general as a fix in the coordinate transform), as a stop-gap so that we can make some end-to-end tests with correct h_max very soon. We can then open another issue to do this fix in a better way.

Do you have any problem with that?

[kbruegge]

Fine for me. Not sure why the Travis tests failed though.

[jjlk]

Hi @mackaiver ,
It might be better to return the integrality of the impact parameter and not only the first two components in estimate_core_position. It's needed if someone is interested to compute the impact distance in the tilted frame via coordinate transformations implemented in ctapipe (e.g., for the energy estimation), right?

Could you add it? Thanks!

[kosack]

Ok the travis tests work now - can you just add the 3D vector return as @jjlk mentioned, and I will merge it and we can do some tests on the grid.