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

Upgrade of data training #58

Closed HealthyPear closed 3 years ago

HealthyPear commented 3 years ago

Requirements

Description

This is a big upgrade, targeting various parts of the pipeline up to DL2/a.

codecov[bot] commented 3 years ago

Codecov Report

Merging #58 (5ccb21d) into master (fefc251) will decrease coverage by 2.60%. The diff coverage is 46.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
- Coverage   34.27%   31.66%   -2.61%     
==========================================
  Files          19       20       +1     
  Lines        2267     2201      -66     
==========================================
- Hits          777      697      -80     
- Misses       1490     1504      +14     
Impacted Files Coverage Δ
protopipe/mva/__init__.py 100.00% <ø> (ø)
protopipe/perf/__init__.py 100.00% <ø> (ø)
protopipe/perf/irf_maker.py 14.49% <0.00%> (ø)
protopipe/perf/utils.py 20.00% <0.00%> (ø)
protopipe/scripts/build_model.py 15.21% <0.00%> (-1.09%) :arrow_down:
protopipe/scripts/model_diagnostic.py 6.76% <0.00%> (ø)
protopipe/perf/cut_optimisation.py 9.59% <1.16%> (ø)
protopipe/scripts/make_performance.py 10.75% <4.44%> (ø)
protopipe/mva/train_model.py 15.68% <11.11%> (ø)
protopipe/mva/utils.py 11.34% <16.66%> (+0.88%) :arrow_up:
... and 13 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 fefc251...5ccb21d. Read the comment docs.

kosack commented 3 years ago

It's a bit hard to separate the main changes from re-formatting, but I guess the biggest changes are just to support the ctapipe-0.9.1 container name changes. This can be considered a first step on the way to fully using the new features of ctapipe-0.9.1 and above, like ImageProcessor and DL1Writer, which can come at a later time (or with the standardized DL1/DL2 tools in ctapipe).

Otherwise, I don't see any big problems so far, but obviously this will be a bit superficial review, as can't easily test that the results are good.

HealthyPear commented 3 years ago

It's a bit hard to separate the main changes from re-formatting, but I guess the biggest changes are just to support the ctapipe-0.9.1 container name changes. This can be considered a first step on the way to fully using the new features of ctapipe-0.9.1 and above, like ImageProcessor and DL1Writer, which can come at a later time (or with the standardized DL1/DL2 tools in ctapipe).

Otherwise, I don't see any big problems so far, but obviously this will be a bit superficial review, as can't easily test that the results are good.

Yes, the upgrade to 0.9.1 was the main reason as it changed the API in some places. The integration test for DL1 works though (it is part of the CI pipeline)

HealthyPear commented 3 years ago

From the point of view of physical performance, this PR updates the calibration and image extraction (from which the latest results come - the ones that got temporarily reverted during between ctapipe 0.9.1 and 0.10.1) plus it enables the "optical aberration" correction as it is done in CTAMARS.

From the data point of view, the variables related to image parameters are the same as in ctapipe (even though the format is still a single table) and the (optional) images file is merged with the rest so 1 run from data_training will produce 1 file (like ctapipe)

plus other minor changes