Enable processing of interleaved calibration events via the process tool #2522
Conversation
This commit enables the processing of flatfield and sky pedestal events using the ctapipe-process tool.
| @@ -483,16 +483,12 @@ class DL1CameraContainer(Container): | |||
|
|
|||
| image = Field( | |||
| None, | |||
| "Numpy array of camera image, after waveform extraction." "Shape: (n_pixel)", | |||
| dtype=np.float32, | |||
There was a problem hiding this comment.
Since ndim is removed, the container complains when two gain images are filled:
ctapipe.core.container.FieldValidationError: DL1CameraContainer Field 'image': the value '[[76.30804 76.080025 76.71846 ... 84.10605 67.9628 84.71408 ]
[65.03804 65.34207 74.57242 ... 82.70825 62.36258 73.0401 ]]' (<class 'numpy.ndarray'>) is invalid: Should have numpy dtype float32
There was a problem hiding this comment.
Hmm. Perhaps we then need to break this into a parent Container and two children that inherit from it, adding the image field. That way it's clear that DL1CameraContainer for Calibration events look different than calibrated ones. Maybe DL1CameraContainer (contains all common fields) → CalibratedDL1CameraContainer (contains float DL1 image with ndims=1), UncalibratedDL1CameraContainer (contains int dl1 image with ndims=2?)
Or something along those lines...
There was a problem hiding this comment.
What is the drawback of having it flexible as it is in the PR?
One problem with having two inheritance containers would be that if we want to store the calibration dl1 images for a single gain camera, we can not store them in UncalibratedDL1CameraContainer because of the dimension restrictions. Though, it would also not belong in CalibratedDL1CameraContainer. A semantic hardcoding structure, i.e. OneGainDL1CameraContainer; TwoGainDL1CameraContainer, would also not be ideal since then we don't have information if the OneGainDL1CameraContainer is gain_selected/calibrated or not.
| ) | ||
| peak_time = Field( | ||
| None, | ||
| "Numpy array containing position of the peak of the pulse as determined by " | ||
| "the extractor. Shape: (n_pixel, )", | ||
| dtype=np.float32, |
| dl1 = extractor( | ||
| waveforms, | ||
| tel_id=tel_id, | ||
| selected_gain_channel=no_gain_selection, |
There was a problem hiding this comment.
Can you please explain the logic behind this? I see that if n_channels is 1, you pass 1d array of zeros, and if n_channel is 2 it is a 2d array of zeros and ones...
There was a problem hiding this comment.
I think it's just a workaround because the ImageExtractors are currently designed to work with gainselected images. Therefore this issue should be addressed in a separate PR.
There was a problem hiding this comment.
I think we should just add an API to the extractors for not gain selected waveforms.
I.e. allow selected_gain_channel=None which should mean no gain selection happened and that the waveform has shape n_channels, n_pixels, n_samples
There was a problem hiding this comment.
yes, I'm trying to solve it right now. I will let you know.
There was a problem hiding this comment.
Im also currently working on #1836 which would solve it - ill create a draft PR today or tomorrow, maybe we can discuss it there.
There was a problem hiding this comment.
great! thank you @Hckjs I have also contacted you on the CTA North slack channel with the changes I had in mind so far. Maybe they can be helpful.
| # TODO: See #1836 (process all provided gains in the ImageExtractor) | ||
| no_gain_selection = np.zeros((n_channels, n_pixels), dtype=np.int64) | ||
|
|
||
| if n_channels == 2: |
There was a problem hiding this comment.
should we maybe force to have two channels here since we need both for calibrations? i.e. raising an error if n_channels != 2 ?
There was a problem hiding this comment.
But then it will not work with a single gain camera, which is currently supported (I guess)
There was a problem hiding this comment.
I think we should add an API back to the ImageExtractor for non-gain-selected events instead of special casing two event types here.
See #1836
|
Hi @TjarkMiener, |
|
Ciao @FrancaCassol, following the suggestion from @kosack in #2519, this PR enables the charge integration and arrival time extraction of the calibration data stream. The DL1-like images and peak times are stored following the current |
Hi @TjarkMiener, I see. However, in the case the flatfield and pedestal events are in a file separated by cosmics (which is our case) and if we solve #1836, flatfield and pedestal events could not just be processed with the standard _calibrate_dl1 function? We need just setting the correct configuration cards (as we do for the other cases). The only thing to provide is to select the correct charge extractor for each event_type (which, by the way it seems to me it is missing in this PR?), this is necessary only in the case flatfields and pedestals are in the same file and we want to process them together, otherwise there is no problem. |
We want to actually change it a bit and use the TableLoader.
The charge extractor type is an input parameter and so far is restricted to two options - LocalPeakWindowSum and FixedWindowSum. The door is open for other extractors in case other telescopes/cameras will need them.
See the part of the answer above. |
|
Redundant due to #2529 |
This commit enables the processing of flatfield and sky pedestal events up to dl1-like using the
ctapipe-processtool. Code changes are tested with interleaved calibration events from LST-1. I needed to tweak the pointing frame inhdf5eventsourcein order to make it work with the standardlstchainR1 interleaved files (not produced with the latestctapipeversion).Currently only tested with the ImageExtractors (LocalPeakWindowSum and FixedWindowSum), which are the default ones.