Reconstruction fix (also for Divergent Pointing) #946

thomasgas · 2019-02-01T18:11:07Z

I'm trying to analyze some MC in divergent pointing mode.
The reconstruction was not working out of the box due to few fixes needed to the HillasIntersection class after the coordinates refactoring in #896: seems to be working correctly now...I'm still working on this.
I've not checked the HillasReconstructor in divergent mode (see #873) yet. I just know that divergent pointing is not allowed now with HillasReconstructor.

codecov · 2019-02-01T18:22:18Z

Codecov Report

Merging #946 into master will increase coverage by 1.28%.
The diff coverage is 92.57%.

@@            Coverage Diff             @@
##           master     #946      +/-   ##
==========================================
+ Coverage   84.25%   85.53%   +1.28%     
==========================================
  Files         181      183       +2     
  Lines       10979    11235     +256     
==========================================
+ Hits         9250     9610     +360     
+ Misses       1729     1625     -104

Impacted Files	Coverage Δ
ctapipe/reco/reco_algorithms.py	`90% <100%> (+6.66%)`	⬆️
ctapipe/reco/HillasReconstructor.py	`98.38% <100%> (+0.92%)`	⬆️
ctapipe/reco/tests/test_ImPACT.py	`63.51% <100%> (+1.54%)`	⬆️
ctapipe/reco/tests/test_HillasReconstructor.py	`94.78% <100%> (+0.33%)`	⬆️
ctapipe/utils/unstructured_interpolator.py	`62.85% <25%> (-1.85%)`	⬇️
ctapipe/reco/ImPACT.py	`44.25% <50%> (+1.44%)`	⬆️
ctapipe/utils/template_network_interpolator.py	`42.85% <50%> (+0.54%)`	⬆️
ctapipe/reco/tests/test_reconstruction_methods.py	`90.9% <90.9%> (ø)`
ctapipe/reco/hillas_intersection.py	`96.53% <94.73%> (+70.21%)`	⬆️
ctapipe/reco/tests/test_hillas_intersection.py	`96.72% <95.83%> (-3.28%)`	⬇️
... and 11 more

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 c154718...f5c847f. Read the comment docs.

maxnoe · 2019-02-02T09:10:05Z

Can you create Test cases for the issues you fixed so this does not happen again?

thomasgas · 2019-02-03T11:15:51Z

Can you create Test cases for the issues you fixed so this does not happen again?

The HillasIntersector class is not very well tested, so I think I can add some tests.

kosack · 2019-02-04T09:48:47Z

divergent pointing is not allowed now with HillasReconstructor

What do you mean by not allowed? It used to work (in a very old version, before coordinates were implemented anyhow). In principle, the methodology in HillasReconstructor should be better for divergent pointing, since no Nominal system is needed. The HillasIntersection one was just a copy of the HESS implementation, I think, and is not used anywhere. We should probably decide on a single method.

maxnoe · 2019-02-04T09:53:48Z

divergent pointing is not allowed now with HillasReconstructor

Multiple code path in the HillasReconstructor as it is now assume a common pointing position.
I found this out when debugging the diffuse reconstruction issue and added an exception for divergent pointing.

It it is possible to support divergent pointing in principle, then these code paths must be changed.
For me, there was no obvious, simple fix, so I just added the exception to make it clear that it currently does not support divergent pointing.

ParsonsRD · 2019-02-04T09:58:05Z

Hi, I agree Karl the HillasReconstructor should work better here where the assumption of a flat nominal system starts to become a bad assumption. I created the HillasIntersection code as a baseline comparison as this is (as far as I know) the way all current instruments do their Hillas reconstruction (and also HillasReconstructor gave weird results at the time). I also agree that we should move towards removing HillasIntersection, but we should probably show the HillasReconstructor gives similar or better results at some point.

kosack · 2019-02-04T14:42:21Z

We should probably add a cross-check test between the two implementations, at least for some simple case (non-divergent for now I guess until both support it).

kosack · 2019-02-04T14:45:20Z

Also: I think we should just drop HorizonFrame and replace it by just using AltAz directly from astropy.coordinates

Agreed - if I recall correctly in HESS, there was a difference between AltAz and Horizon, but it was just related to the refraction correction (it was applied when going from Horizon to AltAz). I think in Astropy, that correction is applied when going from AltAz to ICRS (but we should verify), so there is no difference between Horizon and AltAz

kosack · 2019-02-04T14:46:34Z

yes, it looks like AltAz has an optional temperature and pressure option, which implies refraction gets corrected later. (http://docs.astropy.org/en/stable/api/astropy.coordinates.AltAz.html)

thomasgas · 2019-02-04T15:39:16Z

I run the analysis (to produce some angular resolution) on some point gamma produced with MSTs in divergent pointing and it seems it's working well with the 2D reconstruction method.

maxnoe · 2019-02-04T15:40:45Z

ctapipe/reco/hillas_intersection.py

@@ -82,7 +82,7 @@ def predict(self, hillas_parameters, tel_x, tel_y, array_direction):
        sky_pos = SkyCoord(
            az=src_x * u.deg,
            alt=src_y * u.deg,
-            frame=HorizonFrame()
+            frame=HorizonFrame(location=array_direction.location, obstime=array_direction.obstime)


Isn't it then SkyCoord(az=..., alt=..., frame=array_direction.frame)?

yes! in this way all the information in the frame should be passed.

maxnoe · 2019-02-07T15:25:29Z

So the tests are passing, but diff coverage is zero. So there are no tests covering HillasIntersection.

kosack · 2019-02-07T15:57:07Z

Yes, there need to be some unit tests if we want to keep this class - it's one of the reasons it has not been used. At least similar ones to those in HillasReconstuctor. Ideally, we could have common tests for all reconstructors, but so far the interfaces are not quite uniform.

thomasgas · 2019-02-07T16:53:49Z

So the tests are passing, but diff coverage is zero. So there are no tests covering HillasIntersection.

Yes I know...I have in my to-do list to add some tests to this class...if someone wants to help, that would be great!
Plus we need some tests to compare the output of the two reconstruction methods.

thomasgas · 2019-02-12T10:30:53Z

I'm testing the output of the two reconstructors and it seems that the 3d gives better results that the 2d one (which is what is expected).
I'm ok for dropping the 2d as soon as we are sure about this and if I find a way of implementing the reconstruction for divergent pointing with the 3d method.
It's also easier to use the 3D method than the 2D one, since in the 3d method there's no need to use particular coordinate frames.

ParsonsRD · 2019-02-12T10:36:59Z

Great! So does it give a significantly better angular resolution or just small improvements? Either way is good, but if it's much better it might be nice to make a plot of this to show to consortium to show that we are able to improve on "standard method" even with a fast Hillas based analysis.

thomasgas · 2019-02-12T10:39:00Z

Great! So does it give a significantly better angular resolution or just small improvements? Either way is good, but if it's much better it might be nice to make a plot of this to show to consortium to show that we are able to improve on "standard method" even with a fast Hillas based analysis.

I tested it on few data and the 3D one is 50% to 100% better :D but i need more data and different pointings in order to be sure about this. And if someone else could do the same check, it would be great.

ParsonsRD · 2019-02-12T10:40:44Z

tested it on few data and the 3D one is 50% to 100% better

Wow! That is impressive, if that holds for all events we should definitely show this somewhere, as I'm not sure that this method has been used by IACTs before.

kosack · 2019-06-19T09:49:59Z

Is divergent_mode really needed as an option? I would guess that in the general case where telescope have a pointing error, the correction you make is the same as what you would do for divergent pointing, so perhaps you want to do the divergent correction always? We only ever have the case where the array_pointing == telescope_pointings in an ideal monte-Carlo, and there may soon be MCs with real pointing-errors included, so perhaps that special case should not be the main one? You could then even get rid of array_pointing and calculate it from some barycenter of the telescope pointings.

thomasgas · 2019-06-19T10:09:12Z

Is divergent_mode really needed as an option? I would guess that in the general case where telescope have a pointing error, the correction you make is the same as what you would do for divergent pointing, so perhaps you want to do the divergent correction always? We only ever have the case where the array_pointing == telescope_pointings in an ideal monte-Carlo, and there may soon be MCs with real pointing-errors included, so perhaps that special case should not be the main one? You could then even get rid of array_pointing and calculate it from some barycenter of the telescope pointings.

Yes I can remove the flag...it makes more sense to always do the divergent correction when the pointing is not exactly parallel (pointing corrections or divergent mode): I can just check that if the telescope_pointings is provided, the correction is applied.
Concerning the array_pointing I also thought at some point to get rid of that and use a barycenter of the telescope pointings, but it needs a bit of investigation. Having another reconstructor which also does the reconstruction of the impact point and of the position in the sky in one shot (and in 3D) would naturally remove the need for an array_pointing. That's material for separate PR.

thomasgas · 2019-06-19T10:10:03Z

More details on the fix for the divergent mode with the HillasReconstructor can be found here (slide 15): https://indico.cta-observatory.org/event/2114/contributions/19047/attachments/15148/19178/ASWG_Divergent_Montpellier.pdf

ctapipe/reco/HillasReconstructor.py

ctapipe/reco/tests/test_hillas_intersection.py

kosack

While you're at it, it would be nice to change all the user-configurable parameters to be Traits and ensure Reconstructors are all sub-classes of Component (just changing the base class should work). Then we would be able to store and retrieve the options like weighting method, etc.

ctapipe/reco/HillasReconstructor.py

ctapipe/reco/hillas_intersection.py

ctapipe/reco/tests/test_HillasReconstructor.py

…ithms

thomasgas · 2019-06-19T16:38:48Z

I implemented the suggested modifications (a part for the predict method which I think deserves some discussion).
I added some the warnings also to hillas_intersection and moved the warnings to the common reco_algorithms. Now both reconstructors import the warnings from there.
There is a test which loops over both reconstructors. We might clean a bit the duplicate tests and just keep the common one, moving the common tests there.

thomasgas · 2019-07-09T12:42:16Z

A part from codacy complaining, is there any other problem in merging this PR? could someone review it? Should I merge the master into this branch again before merging the PR?

kosack · 2019-07-15T09:45:50Z

I think we're just waiting for @maxnoe (who's probably quite busy) or @ParsonsRD to give an ok. I Don't see any major issues. merging in master shouldn't be necessary since it doesn't touch any other changed files, but you could do it if you like.

The codacy issues look ok - it's mainly complaining about missing docstrings in the tests, which aren't really critical, so I'm ok ignoring that.

ParsonsRD · 2019-07-15T09:47:40Z

OK I'll try to take a look today

ParsonsRD

All the ImPACT changes look good to me

kosack · 2019-07-17T09:41:50Z

Ok, since we have 2 reviews and Max's requests seem to be addressed, I'll merge this.

seems to be addressed

* upstream/master: Reduce numba signatures to improve import time (cta-observatory#1108) Remove unused utils (cta-observatory#1112) Add method to read bright star catalog and find bright stars in sky region (cta-observatory#1105) Add Fields to calibration containers (cta-observatory#1111) Reconstruction fix (also for Divergent Pointing) (cta-observatory#946) updated AUTHORS with latest mailmap

Few bugs corrected in HillasIntersection after #896

8a9b821

thomasgas added the workinprogress label Feb 1, 2019

thomasgas changed the title ~~first attempt for Divergent Pointing~~ Reconstruction fix for Divergent Pointing Feb 1, 2019

vuillaut changed the title ~~Reconstruction fix for Divergent Pointing~~ WIP: Reconstruction fix for Divergent Pointing Feb 1, 2019

thomasgas requested review from ParsonsRD and maxnoe February 3, 2019 11:26

Create HorizonFrame with location and obstime.

9b58769

maxnoe reviewed Feb 4, 2019

View reviewed changes

horizon frame derived from array_pointing.frame

e245120

Merge remote-tracking branch 'upstream/master' into small_coords_fix

f99ae3d

Thomas Gasparetto added 3 commits February 19, 2019 14:44

First change to HillasReconstructor for divergent

89ae1fb

Fixed coords in nominal frame ImPACT. small bug fix.

caddffa

Telescopes pointings fixed. Remove plotting for debug.

c8a7fd8

Thomas Gasparetto added 2 commits June 19, 2019 12:18

Remove divergent_mode flag from predict.

5432aaf

Update test

088a98c

maxnoe previously requested changes Jun 19, 2019

View reviewed changes

ctapipe/reco/HillasReconstructor.py Outdated Show resolved Hide resolved

ctapipe/reco/HillasReconstructor.py Outdated Show resolved Hide resolved

ctapipe/reco/tests/test_hillas_intersection.py Show resolved Hide resolved

kosack requested changes Jun 19, 2019

View reviewed changes

ctapipe/reco/HillasReconstructor.py Show resolved Hide resolved

kosack reviewed Jun 19, 2019

View reviewed changes

ctapipe/reco/hillas_intersection.py Outdated Show resolved Hide resolved

Verify test_reconstruction is tested and not skipped

d15a6af

maxnoe reviewed Jun 19, 2019

View reviewed changes

ctapipe/reco/tests/test_HillasReconstructor.py Outdated Show resolved Hide resolved

Thomas Gasparetto added 5 commits June 19, 2019 17:26

add single test for HillasReconstructor and HillasIntersection

0ea4ce8

Refactor warnings for both reconstructors. Warnings are in reco_algor…

161467b

…ithms

Added traitlets to hillas_intersection

8a440a9

Docstrings fix

2405e2d

tests for reconstructors updated.

f5c847f

thomasgas requested review from maxnoe and kosack June 21, 2019 14:47

kosack approved these changes Jun 26, 2019

View reviewed changes

ParsonsRD approved these changes Jul 15, 2019

View reviewed changes

kosack merged commit 63fe206 into cta-observatory:master Jul 17, 2019

thomasgas removed the ready for review label Jul 30, 2019

thomasgas deleted the small_coords_fix branch July 30, 2019 12:38

thomasgas mentioned this pull request Aug 3, 2019

Change HillasReconstructor to take a list of HorizonCoordinates #872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconstruction fix (also for Divergent Pointing) #946

Reconstruction fix (also for Divergent Pointing) #946

thomasgas commented Feb 1, 2019

codecov bot commented Feb 1, 2019 •

edited

Loading

maxnoe commented Feb 2, 2019

thomasgas commented Feb 3, 2019

kosack commented Feb 4, 2019

maxnoe commented Feb 4, 2019

ParsonsRD commented Feb 4, 2019

kosack commented Feb 4, 2019

kosack commented Feb 4, 2019

kosack commented Feb 4, 2019

thomasgas commented Feb 4, 2019 •

edited

Loading

maxnoe Feb 4, 2019

thomasgas Feb 4, 2019 •

edited

Loading

maxnoe commented Feb 7, 2019

kosack commented Feb 7, 2019

thomasgas commented Feb 7, 2019

thomasgas commented Feb 12, 2019 •

edited

Loading

ParsonsRD commented Feb 12, 2019

thomasgas commented Feb 12, 2019

ParsonsRD commented Feb 12, 2019

kosack commented Jun 19, 2019

thomasgas commented Jun 19, 2019

thomasgas commented Jun 19, 2019 •

edited

Loading

kosack left a comment

thomasgas commented Jun 19, 2019

thomasgas commented Jul 9, 2019

kosack commented Jul 15, 2019

ParsonsRD commented Jul 15, 2019

ParsonsRD left a comment

kosack commented Jul 17, 2019

Reconstruction fix (also for Divergent Pointing) #946

Reconstruction fix (also for Divergent Pointing) #946

Conversation

thomasgas commented Feb 1, 2019

codecov bot commented Feb 1, 2019 • edited Loading

Codecov Report

maxnoe commented Feb 2, 2019

thomasgas commented Feb 3, 2019

kosack commented Feb 4, 2019

maxnoe commented Feb 4, 2019

ParsonsRD commented Feb 4, 2019

kosack commented Feb 4, 2019

kosack commented Feb 4, 2019

kosack commented Feb 4, 2019

thomasgas commented Feb 4, 2019 • edited Loading

maxnoe Feb 4, 2019

Choose a reason for hiding this comment

thomasgas Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

maxnoe commented Feb 7, 2019

kosack commented Feb 7, 2019

thomasgas commented Feb 7, 2019

thomasgas commented Feb 12, 2019 • edited Loading

ParsonsRD commented Feb 12, 2019

thomasgas commented Feb 12, 2019

ParsonsRD commented Feb 12, 2019

kosack commented Jun 19, 2019

thomasgas commented Jun 19, 2019

thomasgas commented Jun 19, 2019 • edited Loading

kosack left a comment

Choose a reason for hiding this comment

thomasgas commented Jun 19, 2019

thomasgas commented Jul 9, 2019

kosack commented Jul 15, 2019

ParsonsRD commented Jul 15, 2019

ParsonsRD left a comment

Choose a reason for hiding this comment

kosack commented Jul 17, 2019

codecov bot commented Feb 1, 2019 •

edited

Loading

thomasgas commented Feb 4, 2019 •

edited

Loading

thomasgas Feb 4, 2019 •

edited

Loading

thomasgas commented Feb 12, 2019 •

edited

Loading

thomasgas commented Jun 19, 2019 •

edited

Loading