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

Drs4 time corr #236

Merged
merged 7 commits into from
Nov 30, 2019
Merged

Drs4 time corr #236

merged 7 commits into from
Nov 30, 2019

Conversation

pawel21
Copy link
Collaborator

@pawel21 pawel21 commented Nov 27, 2019

Hi,

I have created code to DRS4 time correction.
We have two class TimeCorrectionCalculate to create file with coefficients for time correction curve
of chip DRS4 and PulseTimeCorrection to make time correction on pulse time from extractor.
I added notebook time_calibration.ipynb which show how to create h5py file and how to apply correction to camera calibration run and cosmic events.

@FrancaCassol Please look on my code and tell what do you think?
I think, we should ,,connect" time correction with flatfield calibration

What is the status of gain selection? Because, now code is designed for waveform with both gains.

@FrancaCassol
Copy link
Collaborator

Hi @pawel21,

I can not test the code before this evening, some questions for the moment:

  • could you please describe the method and algorithms you use for deriving and applying the corrections?
  • In the notebook I see you modify not only the width but also the mean value of the time, why?
  • is your correction dependent from the charge extractor you use?
  • you speak of h5py, but I guess it is a normal hdf5 file, isn't it? (in this case better you speak of hdf5)

@pawel21
Copy link
Collaborator Author

pawel21 commented Nov 28, 2019

Hi @pawel21,

I can not test the code before this evening, some questions for the moment:

No problem.

* could you please describe the method and algorithms you use for deriving and applying the corrections?

,,DRS4 chips exhibit an delay of typically 1 ns (up to 4 ns), depending on the absolute location of the signal pulse in the domino ring. The differences can be calibrated with the use of calibration pulses. The computed mean arrival time of a pulse in a channel is a rather complicated function of the time sample position in the ring and differs from one DRS chip to another. For each channel we expand this function into a Fourier series to obtain the correction function." (from Analysis techniques and performance of the Domino Ring Sampler version 4 based readout for the MAGIC telescopes ) To computed mean arrival time of a pulse I am using LocalPeakWindowSum (window_width and window_shift user can set up), then I fit data using Fourier series, I obtain coefficients, which I save to hdf5 file. Then, I am using "correction curve" (which coefficients from previous step) to correct arrival time (pulse time) from extractor in each pixel. For single event time resolution is decreased from 1.6 ns to 0.4 ns.

* In the notebook I see you modify not only the width but also the mean value of the time, why?

The idea of time correction is the following: we got arrival time from calibration pulse (charge, pulse_time = extractor(ev.r1.tel[1].waveform) and then using "correction curve" in class PulseTimeCorrection we got arrival time after correction pulse_corr_array = PulseTimeCorrection.get_corr_pulse(ev, pulse_time). So, my code modify pulse_time

* is your correction dependent from the charge extractor you use?

Yes, this is not big differences. User can set up parameters for LocalPeakWindowSum (I added example in notebook"). It is best to use the same parameters for calibration and for processing data.

* you speak of h5py, but I guess it is a normal hdf5 file, isn't it? (in this case better you speak of hdf5)

Yes, I use normal hdf5 file. Thank you.

@FrancaCassol
Copy link
Collaborator

Hi @pawel21,

thanks for your explanation. Could you please add the link to the MAGIC document to the doc of the of the classes?

Also, I wonder, is this correction compatible with the sampling time correction of Yuto? Do you know if the correction proposed by Yuto is applied in Magic?

@pawel21
Copy link
Collaborator Author

pawel21 commented Nov 29, 2019

Hi @pawel21,

thanks for your explanation. Could you please add the link to the MAGIC document to the doc of the of the classes?

Yes, sure

Also, I wonder, is this correction compatible with the sampling time correction of Yuto? Do you know if the correction proposed by Yuto is applied in Magic?

Yes, should be compatible. My method will be applied to time, and "sampling time correction" meted will be applied to the value of the reproduced signal, then they should not bite.
Sampling time correction of Yuko is used to improve the extracted signal. This method is not used in MAGIC.

Copy link
Collaborator

@FrancaCassol FrancaCassol left a comment

Choose a reason for hiding this comment

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

Hi @pawel,
thanks for your reply. I just realised that I didn't submit these more specifics questions

allow_none=True).tag(config=True)


r1_sample_end = Int(default_value=38,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pawel21,
why do you need to give the r1_sample_start and r1_sample_end?
This is already defined in the R0 calibrator. At this level you should be able to work with the full r1 and forget r0. Isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, You are right. Thank you

event : `ctapipe` event-container
"""
if event.r0.tel[self.tel_id].trigger_type == 1:
for nr_module in prange(0, n_modules):
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you use r0 trigger and not r1 trigger?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is the difference between trigger and r1 trigger? But, I agree use r1 trigger will be more convenient

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @pawel21,
there is not difference, at least for the moment. But, after the r0 calibration, I think it is better to work only with r1 elements


pixel_ids = event.lst.tel[self.tel_id].svc.pixel_ids
charge, pulse_time = self.extractor(event.r1.tel[self.tel_id].waveform[:, :, self.r1_sample_start:self.r1_sample_end])
self.calib_pulse_time_jit(charge,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as before, after the LSTR0Corrections r1 samples are 36 and we should be able to use all of them, why do you select a range to be given to the extractor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed. Now, my code use r1 samples. Thank you


n_gain = 2
n_channel = 7
n_modules = 265
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this compatible with eventually missing modules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This wasn't compatible. I have modified code to make correction only for working modules ( n_modules_from_event = event.lst.tel[self.tel_id].svc.num_modules line 69 in pulse_time_correction.py)

@rlopezcoto
Copy link
Contributor

Thanks @pawel21 for the code and @FrancaCassol for the review. This PR is very necessary to improve signal extraction.

@rlopezcoto rlopezcoto merged commit 4a8732d into cta-observatory:master Nov 30, 2019
@pawel21 pawel21 deleted the drs4_time_corr branch July 10, 2020 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants