-
Notifications
You must be signed in to change notification settings - Fork 77
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
Update calibrator with pulse correction and gain selection #248
Update calibrator with pulse correction and gain selection #248
Conversation
Do not use events without TIB data
Changed the time correction in order to put time arouns zero as for drs4 time correction
pulse time correction to the calibrator
Check out this pull request on You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB. |
Hi @vuillaut, @rlopezcoto, I think that at this point we should fast review this code |
To make things more clear I will commit the correction of the reconstruction script in a different PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not yet check the notebook, but the rest looks fine to me, except the small clarification I added.
'from the peak_index (peak_index - shift)' | ||
).tag(config=True) | ||
charge_product = Unicode( | ||
'LocalPeakWindowSum', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall this be hardcoded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extractor is not suppose to change often, but why do you want to have it hardcoded?
|
||
# perform the gain selection if the threshold is defined | ||
if self.gain_threshold: | ||
waveforms, gain_mask = self.gain_selector(event.r1.tel[telid].waveform) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be calculated on r0?
r1 are in p.e. at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The r1 waveform is low-level corrected but still in ADC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this different from CTA data model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vuillaut,
thinking gain to this point I agree that it is better to use the r0 waveform (the fact that it is not low level corrected should not affect the result). I am going to change it.
You are right, our r1 is different than in the ctpipe data model. This is due to the fact that we have a level more in the data (ctapipe r0 is supposed to be already low-level corrected), we should solve this issue in the next (but not in this PR ;-))
event.dl1.tel[telid].image = charge | ||
event.dl1.tel[telid].pulse_time = pulse_corr_array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, the image and pulse_time will be of shape (1855,2)
instead of (1855,)
.
I don't think this is what we want (following algorithms expect images and pulse_time with 1 dimension).
Instead, I'd recommend a ManualGainSelection to select high gain by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then the notebook will need to be modified to plot images with single dimension only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vuillaut,
actually this is what I want: I would like to have the possibility to calibrate without performing any gain selection (mainly for debug purposes). The HG by default can be obtained just putting the default threshold to 4095 ;-)
By the way, here I fixed the gain selector to the Threshold one, should we give the possibility to choose other selectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the advantage of having this for debugging but here the default option is to not select gains (since None
is the default for gain_threshold
). Maybe the default should be to have a gain selection to make sure that image and pulse_time have the correct dimension by default? And keeping both gains be a "hidden feature" for expert and debug?
To your question, yes I think it would be better to have the gain selector be the one passed in the config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the advantage of having this for debugging but here the default option is to not select gains (since
None
is the default forgain_threshold
). Maybe the default should be to have a gain selection to make sure that image and pulse_time have the correct dimension by default? And keeping both gains be a "hidden feature" for expert and debug?
No problem, I can make 4094 as default
To your question, yes I think it would be better to have the gain selector be the one passed in the config file.
Then I must take out the threshold trailet and pass the gain selector and one must not forget to pass the selector Config. @rlopezcoto what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then the notebook will need to be modified to plot images with single dimension only.
which notebook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already working with images with single dimension
- Use the r0 waveform for the gain selection - Store the selected gain following the prescrition of K. Kosack's document "R1 & DL0 Telescope Event Interfaces and Prototype Evaluation"
Hi, |
@@ -29,6 +21,7 @@ | |||
--output_file Data_table.fits --pedestal_file pedestal_file_run446_0000.fits | |||
--calibration_file calibration.hdf5 | |||
--max_events 1000 | |||
--tel_id 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be 1 by default?
ok it is just an example...
@@ -74,7 +72,7 @@ def main(): | |||
print("max events: {}".format(args.max_events)) | |||
|
|||
# Camera geometry | |||
geom = CameraGeometry.from_name("LSTCam-002") | |||
geom = CameraGeometry.from_name("LSTCam-003") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is LSTCam-003
already part of current environment?
ctapipe v0.7 comes with what? ctapipe_resources v0.2.18 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LSTCam-003 was recently included in ctapipe_io_lst (see New time and geometry #19)
@FrancaCassol it seems that the latest commits made the code break on the calibration part, could you please check it out? Thanks |
Hi @FrancaCassol, did you have time to have a look at this? It would be great if we could move on with this one to get #241 also merged |
Hi @rlopezcoto, |
Thanks @FrancaCassol! |
Hi @ruben,
|
In this PR it has been added:
-- the pulse time correction: if the calibrator find the time calibration file it applies the drs4 corrections otherwise it just applies the flat-field time corrections
-- the gain selection based on the threshold method: if the threshold is set to None, no selection is performed. I add the new notebook apply_calibration_with_gain_selection.ipynb to show an example of application
The configuration for all the calibration is in onsite_camera_calibration_param.json
Next step is to better coordinate all the configuration files