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

Fixing core reconstruction #777

Merged
merged 1 commit into from
Sep 21, 2018

Conversation

kpfrang
Copy link
Member

@kpfrang kpfrang commented Sep 7, 2018

Discussing with @TarekHC, @mackaiver and @moralejo we found that there was a bug in the calculation of the core position. I've tracked this down to being a wrong sign in one component of the norm vector in the HillasPlanes. In order to not break the direction reconstruction I added two lines which only change the y-component's sign of the norm vector in the estimation of the core position and should not affect anything else.

In this plot right here, you can clearly see that the old reconstruction of the position (blue diamond) was nonsense and the fixed estimate in red is clearly getting close to the true position (black).
attachment

@TarekHC TarekHC added the bug label Sep 7, 2018
@codecov
Copy link

codecov bot commented Sep 7, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #777      +/-   ##
==========================================
+ Coverage   71.54%   71.54%   +<.01%     
==========================================
  Files         199      199              
  Lines       10824    10826       +2     
==========================================
+ Hits         7744     7746       +2     
  Misses       3080     3080
Impacted Files Coverage Δ
ctapipe/reco/HillasReconstructor.py 96.55% <100%> (+0.06%) ⬆️

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 72d9b90...05188e4. Read the comment docs.

@kbruegge kbruegge mentioned this pull request Sep 7, 2018
@kbruegge
Copy link
Member

kbruegge commented Sep 7, 2018

Nice one. We should check if thats actually a bigger problem. Maybe connected to #721

@kosack kosack merged commit 5d4cd37 into cta-observatory:master Sep 21, 2018
@maxnoe
Copy link
Member

maxnoe commented Sep 21, 2018

I don't think this should have been merged, as Kai found the underlying reason which was the azimuth rotation, not a minus sign for y.

@kosack
Copy link
Contributor

kosack commented Sep 21, 2018

Ah, that wasn't clear from the discussion. I could revert it, or just wait for a general fix. Which would you prefer?

@maxnoe
Copy link
Member

maxnoe commented Sep 21, 2018

I think it's better to revert, as #778 fixes the problem at the azimuth level. Even better would be to fix it inside the coordinates module, not outside before using the functions.

kosack added a commit that referenced this pull request Sep 21, 2018
kosack added a commit that referenced this pull request Sep 21, 2018
* Revert "better leakage parameter calculation (#783)"

This reverts commit 20c74be.

* Revert "Fixing core reconstruction (#777)"

This reverts commit 5d4cd37.
@kosack kosack mentioned this pull request Sep 21, 2018
@kpfrang kpfrang deleted the fix_core_reconstruction branch October 11, 2018 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants