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

Upgrade of data training #58

Merged

Conversation

HealthyPear
Copy link
Member

@HealthyPear HealthyPear commented Jul 30, 2020

Requirements

  • fix Travis error finding config file
  • Update documentation
  • Update the rest of the pipeline enough to not crash
  • Enable CI on Github Actions (Enable CI from GitHub actions #84)

Description

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

@HealthyPear HealthyPear mentioned this pull request Oct 31, 2020
2 tasks
@HealthyPear HealthyPear marked this pull request as ready for review November 27, 2020 17:39


def CTAMARS_radii(camera_name):

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably doesn't even need to be a function, right? Nothing inside is computed or changes. Why not just make it a module-level variable called CTAMARS_RADII?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right.
Would you prefer I do it here or I keep it a small fix for later?

@kosack
Copy link
Contributor

kosack commented Nov 30, 2020

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
Copy link
Member Author

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
Copy link
Member Author

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

Pipeline features and enhancements automation moved this from In progress to Reviewer approved Dec 1, 2020
@HealthyPear HealthyPear merged commit 3225536 into cta-observatory:master Dec 1, 2020
Pipeline features and enhancements automation moved this from Reviewer approved to Done Dec 1, 2020
@HealthyPear HealthyPear deleted the upgrade-towards_protopipe_03 branch December 1, 2020 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation or services hosting it enhancement New feature or request fix A PR that fixes a bug or a wrong behaviour. maintenance
2 participants