Compute and record shower's core position also in the TiltedFrame when using HillasReconstructor

[HealthyPear]

This makes things easier for calculating the impact parameters for each telescope in the pipeline which shouldn't bother with frames and transformations outside what will be ctapipe-process

[codecov]

Codecov Report

Merging #1745 (9cbdafa) into master (d7f8a58) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1745   +/-   ##
=======================================
  Coverage   92.22%   92.23%           
=======================================
  Files         190      190           
  Lines       15196    15201    +5     
=======================================
+ Hits        14015    14020    +5     
  Misses       1181     1181           

Impacted Files	Coverage Δ
ctapipe/io/hdf5eventsource.py	91.48% <ø> (ø)
ctapipe/reco/hillas_intersection.py	95.75% <ø> (ø)
ctapipe/containers.py	100.00% <100.00%> (ø)
ctapipe/io/datawriter.py	88.88% <100.00%> (ø)
ctapipe/reco/hillas_reconstructor.py	98.47% <100.00%> (ø)

---

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 d7f8a58...9cbdafa. Read the comment docs.

[HealthyPear]

I think there is a problem with the current test files, which do not support the new data model version.
How did you solve this the last time?

[HealthyPear]

I think there is a problem with the current test files, which do not support the new data model version.
How did you solve this the last time?

Oh, I guess by adding the bumped version also to e.g. dl1eventsource.py in COMPATIBLE_DL1_VERSIONS...

[maxnoe]

Yes, we you need to implement the necessary changes in the dl1 event source, which should be no changes needed as only dl2 changed

[HealthyPear]

Due to some failing unit-tests I noticed that in the HillasIntersection reconstructor we return the error on the reocnstructed core in 1D as

core_uncert=u.Quantity(np.sqrt(core_err_x ** 2 + core_err_y ** 2), u.m)

should I split it in the 2 components we deicded in this PR or we add anyway the 1D error?
I think the 2 components are sufficient now as one can obtain the 1D quantity easily from the data model if needed.

[HealthyPear]

Also, HillasIntersection was returning already the reconstructed core in the TiltedFrame, so I will change the return Fields in the ReconstructedGeometryContainer accordingly.

[HealthyPear]

The docs systematically fail with the following error,

Configuration error:
There is a programmable error in your configuration file:

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/sphinx/config.py", line 348, in eval_config_file
    execfile_(filename, namespace)
  File "/usr/local/lib/python3.8/site-packages/sphinx/util/pycompat.py", line 81, in execfile_
    exec(code, _globals)
  File "/github/workspace/docs/conf.py", line 32, in <module>
    import ctapipe
ModuleNotFoundError: No module named 'ctapipe'

As far as I know, I didn't change anything related to the documentation or its configuration and the branch is up-to-date with master

Locally it works

[HealthyPear]

Following the log backwards I even see OSError: 'git' was not found so it is probably a problem on the server side...

[HealthyPear]

UPDATE: I get the same behavior also with newer protopipe PRs, and also ctapipe's master is failing (see #1779)

still need to do some tests of io

[maxnoe]

Why is adding more fields a major version change?

[kosack]

I think because it renamed an existing field, so that would break reading previously written data. Probably we should update the version just before release in any case, rather than using values in each PR.

[maxnoe]

Ah, right, core_uncert to core_uncert_x / core_uncert_y.

So this is not only about the tilted / non-tilted frame?

[HealthyPear]

Hi Max,
this specific change it's because when I was working on this, I noticed that the other reconstructors (intersection and Impact) were recording core_x and core_y (in the ground frame), but computing and recording the error on the 2D quantity which is inconsistent.
Either we record core and core_uncer, or (as I chose to do) we record core_x, core_uncert_x, core_y, core_uncert_y and then subsequent analysis steps or benchmarks can use the 1D quantities to get the 2D quantity with a simple operation.

[maxnoe]

Yes, agreed, better have 1d quantities.

[maxnoe]

This makes things easier for calculating the impact parameters for each telescope in the pipeline which shouldn't bother with frames and transformations outside what will be ctapipe-process

I kind of disagree. We shouldn't store redundant data if not absolutely necessary.

We could talk about which is the more sensible information to be stored, which I guess would be the tilted frame? However for distance to the telescope, isn't the distance in the ground frame more relevant, since that decides the Cherenkov density on the ground?

[HealthyPear]

This makes things easier for calculating the impact parameters for each telescope in the pipeline which shouldn't bother with frames and transformations outside what will be ctapipe-process

I kind of disagree. We shouldn't store redundant data if not absolutely necessary.

We could talk about which is the more sensible information to be stored, which I guess would be the tilted frame? However for distance to the telescope, isn't the distance in the ground frame more relevant, since that decides the Cherenkov density on the ground?

Both are important, as impact distance is defined in the tilted frame.
I find it very weird for benchmarking to have to deal with low-level ctapipe API to make coordinate transformation for a quantity that is important for the energy and classification estimation.

As we are already working on the ML module and ctapipe is expected to estimate itself these quantities later on for DL2, I think having the core recorded in both frames is more convenient than leaving someone else to do it later everytime it is needed.