Consolidate camera calibration in a single class #1048
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Also the |
Codecov Report
@@ Coverage Diff @@
## master #1048 +/- ##
=========================================
Coverage ? 83.26%
=========================================
Files ? 181
Lines ? 10410
Branches ? 0
=========================================
Hits ? 8668
Misses ? 1742
Partials ? 0
Continue to review full report at Codecov.
|
kosack
previously approved these changes
Apr 23, 2019
* master: Create numba ufunc for sum of samples within charge extraction window (cta-observatory#1038) Implement nan-handling like matplotlib high-level api (cta-observatory#1050) Fix See Also docs for sphinx 2 (cta-observatory#1051) Correct function name Create extract_pulse_time_around_peakpos Correct min to max Fix test Update bokeh plotters to handle nan
|
Do we no longer need the check_x_exists functionality to prevent containers being overwritten? |
You are right, I've restored them now |
thomasarmstrong
approved these changes
Apr 26, 2019
kosack
approved these changes
Apr 29, 2019
24 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As #1046 was well received, I have proceeded with further simplifying the calibration chain. This PR consolidates the calibration chain into a single class. The
CameraCalibratorclass is repurposed from a "helper" class, into the main calibration chain class.The
CameraDL0ReducerandCameraDL1Calibratorare deleted. The individual calibration levels are still apparent inside the class (as individual functions), providing structure and direction.The arguments to
CameraCalibratornow operate similar to how theCameraDL1Calibratorworked, where the classes responsible for each of the calibration steps are individually constructed outside theCameraCalibrator, before being passed to theCameraCalibratorto be applied in the correct order to fill the event container. If a calibration-step-class is not passed to theCameraCalibratorthe default classes are used.By changing to this approach, it makes the process of adding additional steps in the calibration chain more easier and clear. Two additional steps that I intend to be add in future PRs are a gain selector and a charge calibrator (flat-field and absolute calibration), for which additional arguments to the class will be added.
Some TODOs exist in the
CameraCalibratorindicating changes I intend for future PRs. I wanted to approach this in smaller steps to prevent errors in the results of the calibration chain, and to make reviews more digestible. This PR therefore keeps the calibration operations identical to the existing chain for the time being.