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

How to add peak arrival times? #1021

Closed
LukasNickel opened this issue Mar 21, 2019 · 14 comments
Closed

How to add peak arrival times? #1021

LukasNickel opened this issue Mar 21, 2019 · 14 comments

Comments

@LukasNickel
Copy link
Member

LukasNickel commented Mar 21, 2019

I was wondering how one could go ahead adding actual arrival times to the DL1 Container.
From my understanding ctapipe only offers peakpos as integer for the position of the extracted peak(?).

As this work is done in image/charge_extractors.py I should probably add a method to at least one of the classes there.

Now the Question becomes: Where would i put such a function? Put an abstract method in ChargeExtractor, implementation in the most specialized classes (-> Neighbourpeak, AverageWF) or do I follow the same scheme as peakpos (defining tons of intermediate functions for the classes between) or could I just add it to a more abstract class as it would be the same for each Integrator (although it probably wouldn't, I dont know)?

Thanks for all kind of input :)

@watsonjj
Copy link
Contributor

watsonjj commented Mar 21, 2019

I am refactoring the ChargeExtractor classes at the moment (PR coming soon). So this is a great time to have this discussion.

How is peak arrival time often defined? Is it calculated as part of the charge extraction routine (a byproduct like peakpos), or is it an independently calculated value?

@kosack
Copy link
Contributor

kosack commented Mar 21, 2019

The fact that peakpos is an integer is just an artefact of the simple charge integration method used so far (which is just an integer sum of waveform samples). If you want a float value (also for the integral), then we need a fancier charge integration method with interpolation, proper integration and peak detection. Alternately, we could apply some waveform filtering to up-sample and filter the waveforms, and your "integer" peakpos becomes a fraction (which is easy, but maybe not the fastest method).

@LukasNickel
Copy link
Member Author

LukasNickel commented Mar 21, 2019

So we dont really two separate values but replace peakpos with an actual float value?

From my limited understanding (and from what @maxnoe told me a while ago) you could define an arrival time as weighted mean over the all samples where the weights would be the values for each sample.
This would not require interpolation of the waveforms as far as I understand it.

@LukasNickel
Copy link
Member Author

LukasNickel commented Mar 21, 2019

So instead of doing sum_data.argmax(2).astype(np.int) to return the index of the sample with the highest value you could instead take the weighted mean over the sample indices and receive a float.
(sum_data.shape = n_channels, n_pixels, n_samples)

@thomasgas
Copy link

more here maybe (#907)

@maxnoe
Copy link
Member

maxnoe commented Mar 21, 2019

AFAIK, MAGIC uses a weighted average, so np.average(time, weights=charge)

FACT-Tools fits the rising edge of the pulse using a third order polynomial, and defines arrival time as its point of inflection. (Don't ask me how we ended here. I guess it's rather robust against noise).

@kpfrang
Copy link
Member

kpfrang commented Mar 21, 2019

To my knowledge the weighted average is also used in EventDisplay.

@watsonjj
Copy link
Contributor

Great. So my intention is to replace the ChargeExtractor class with a WaveformReducer class, which returns an extracted charge and a peak time.

The inheritance approach of the ChargeExtractor will be abandoned for a composition design, where a WaveformReducer is built up from some base functions existing in the module.

The peak time returned will be a float calculated from a method such as weighted average. Some WaveformReducers can use this technique for extracting the time, other WaveformReducers may implement a different technique for extracting the peak time (and charge).

@watsonjj
Copy link
Contributor

This would also remove the waveform cleaners module, as the waveform cleaning will be part of the charge/time extraction in the WaveformReducer

@watsonjj
Copy link
Contributor

This was previously discussed in #907

@watsonjj
Copy link
Contributor

When calculating the pulse time using the weighted average, should it be done for the samples inside the integration window defined by the peak finding, or should it be done using all samples in the waveform?

@kosack
Copy link
Contributor

kosack commented Mar 25, 2019

Probably depends on the peak-finding algorithm: for some methods a window that is larger than the integration window may be needed (especially if the integration only covers a sub-set of the full pulse).

@watsonjj
Copy link
Contributor

And would one expect the pulse time from the NeighborPeakIntegrator to be calculated from the average waveform of the neighbours to the pixel? Or would it be calculated from the waveform of the pixel?

@watsonjj
Copy link
Contributor

Addressed in #1033

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants