cta-observatory / protopipe

Prototype data analysis pipeline for the Cherenkov Telescope Array Observatory

https://protopipe.readthedocs.io/en/latest/

Other

5 stars 13 forks source link

Update CameraFrame-to-TelescopeFrame transformation and HillasReconstructor #151

Closed HealthyPear closed 3 years ago

HealthyPear commented 3 years ago

Closes #149

One of the improvements seems to be at least the H_max estimation.

Line 1 protopipe before this PR 1 simtel run Line 2 protopipe after this PR 1 simtel run Line 3 CTAMARS full gamma1 sample

Update

Latest result using full gamma1 sample

codecov[bot] commented 3 years ago

Codecov Report

Merging #151 (29aaed8) into master (ab2b44f) will increase coverage by 0.59%. The diff coverage is 86.95%.

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
+ Coverage   66.25%   66.85%   +0.59%     
==========================================
  Files          24       24              
  Lines        2291     2359      +68     
==========================================
+ Hits         1518     1577      +59     
- Misses        773      782       +9

Impacted Files	Coverage Δ
protopipe/pipeline/event_preparer.py	`65.19% <80.00%> (+0.01%)`	:arrow_up:
protopipe/pipeline/temp.py	`91.50% <87.61%> (+1.87%)`	:arrow_up:
protopipe/perf/temp.py	`87.50% <0.00%> (-12.50%)`	:arrow_down:

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 ab2b44f...29aaed8. Read the comment docs.

kosack commented 3 years ago

Can you explain what the bug was? It is hard to tell with all the changes.

HealthyPear commented 3 years ago

Can you explain what the bug was? It is hard to tell with all the changes.

My best guess is that it was some code divergence in the camera transformation part between the old development code in protopipe.pipeline.temp and the PR in ctapipe

kosack commented 3 years ago

I mainly ask because the code to do the H_max reconstruction did not change, but what did change was perhaps the HillasPlane initialization. If H_max was wrong before, it is also possible there were other bugs (or new ones) that are fixed (or broken) in this new version since the direction and core reconstruction also depend on the HillasPlane definition. Therefore it would be nice to see what was the original problem that caused h_max to be incorrect so we don't repeat it, or so we can add a unit test.

HealthyPear commented 3 years ago

I'll approve it so we can try a full production, but would still like to undertstand what was fixed

No need to approve and merge the PR yet, I can just use it from a dev branch (I should have opened it as a draft PR sorry)