Change unit of log_e_reco/log_mc_energy from GeV to TeV

[rlopezcoto]

Solves #320

[codecov]

Codecov Report

Merging #325 into master will increase coverage by 0.94%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
+ Coverage   41.73%   42.67%   +0.94%     
==========================================
  Files          69       68       -1     
  Lines        4771     4691      -80     
==========================================
+ Hits         1991     2002      +11     
+ Misses       2780     2689      -91     

Impacted Files	Coverage Δ
lstchain/visualization/plot_dl2.py	18.14% <0.00%> (ø)
lstchain/io/lstcontainers.py	90.06% <100.00%> (ø)
lstchain/reco/dl0_to_dl1.py	69.60% <100.00%> (-0.61%)	⬇️
lstchain/reco/dl1_to_dl2.py	64.70% <100.00%> (ø)
lstchain/reco/tests/test_utils.py	100.00% <0.00%> (ø)
lstchain/calib/camera/time_correction_calculate.py	0.00% <0.00%> (ø)
...stchain/scripts/lstchain_data_muon_analysis_dl1.py	0.00% <0.00%> (ø)
lstchain/scripts/lstchain_muon_reconstruction.py
lstchain/reco/utils.py	65.83% <0.00%> (+0.87%)	⬆️

---

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 62d122d...617aa80. Read the comment docs.

[vuillaut]

Hi @rlopezcoto
Thanks for addressing this issue.
I think the changes should be done at DL1 level, meaning, in the description of the DL1Parameters container and when log_mc_energy is computed. Then the prediction will be in log(E/TeV) already and mc and reco will be consistent.

[rlopezcoto]

that is going to make a bit messy with the models already produced when applied with newer lstchain versions, right?

My proposal was to change only the log_e_reco written in the DL2 file so that it is consistent with the unit of e_reco

[vuillaut]

that is going to make a bit messy with the models already produced when applied with newer lstchain versions, right?

My proposal was to change only the log_e_reco written in the DL2 file so that it is consistent with the unit of e_reco

Yes it will, but models should not be used between two main releases of lstchain anyway.
So we will just produce new models and IRF with the new release.
Having an inconsistency between mc and reco will be even worse than the current situation in my opinion.

[rlopezcoto]

ok, thanks @vuillaut!
So we should keep this in mind when re-processing new data (shall the next one be a major release then?)

[vuillaut]

ok, thanks @vuillaut!
So we should keep this in mind when re-processing new data (shall the next one be a major release then?)

Yes, I think it should - we have introduced major changes recently, no?

I think there are more places where changes should be introduced concerning this PR, for example (but not exhaustive):

• filtering on log_reco_energy in build_models by default should now be -np.inf
• some labels in plots

[vuillaut]

It seems all the changes have been addressed.
Thanks.

[maxnoe]

@vuillaut @rlopezcoto there were no new commits since I reviewed, did you forget to push?

[vuillaut]

@vuillaut @rlopezcoto there were no new commits since I reviewed, did you forget to push?

I saw that @rlopezcoto said "done" to each comment and assumed (too quickly) they have been addressed.

[rlopezcoto]

sorry, weird... I was left with no space on my computer and files were modified but commit/push did not work...