New waveform analysis class to interpolate waveforms using tapered sinc kernel #208
Conversation
There was a problem hiding this comment.
Hi Yashwanth, thanks again for the updated PR. I've left some comments on general code restructuring. In general, this looks good!
I've mentioned in one of the comments, but it would be great if there's a writeup detailing the math being done here for users in the future.
Thanks again!
| int N = 8; // number of interpolated points per data point | ||
| double T = 30.0; // tapering constant | ||
| for (int k = -48; k < 49; k++) { | ||
| double val = k * 3.1415 / (float)N; |
There was a problem hiding this comment.
should use c++ style casting (static_cast in this case) instead of c-style casting. ditto for all cases.
There was a problem hiding this comment.
plus, N is only used as a float. Perhaps it should just be defined as a float.
Also, if this kernel is not refactored in a separate scope, please rename the single-letter variables to make them more searchable.
| } | ||
|
|
||
| // Calculating tapered sinc kernel | ||
| std::vector<double> tsinc_kernel; |
There was a problem hiding this comment.
I believe this kernel calculation is independent of the input waveform parameters. Meaning that the kernel calculation should be placed in a static function, and should only be computed once, not during every single DoAnalysis call.
Furthermore, currently it seems all kernel parameters are hard-coded. I would suggest one of the following two changes:
- Expose relevant parameters (for example, ones that adjust the Fourier-Domain bandwidth of the sinc kernel) as a ratdb table entry
- convert this kernel calculation to a pre-calculated array, placing it in a ratdb entry.
While option 2 may seem inferior at first glance, it would actually make this fitter a general "convolution" analyzer. I am not sure how useful this would be since the charge integration is quite simple, but perhaps people would want to run kernels with better time-domain behaviors.
| fFitPeak = peakSampleVolt.second; | ||
| fFitTime = bf + peakSampleVolt.first * fTimeStep / (float)N; | ||
| fFitCharge = 0; | ||
| for (auto& vlt : interp_wfm) { |
There was a problem hiding this comment.
do not use auto, unless we are looping through maps with pairs being the type.
| return result; | ||
| } | ||
|
|
||
| void WaveformAnalysisSinc::InterpolateWaveform(const std::vector<double>& voltWfm) { |
There was a problem hiding this comment.
I have a couple issues with the name of this function, as well as accommodating documentations.
- No "interpolation" is actually performed here. Interpolation would suggest that we are going in between data samples. As far as I can tell, this is not happening. What is happening is a FIR convolution / filtering. So it should probably named as such.
- Charge and time calculation is actually done at the end of this method. This is not obvious that this is happening.
There was a problem hiding this comment.
We are interpolating the waveform by convolving the waveform with the tsinc kernel! The convolution introduces 7 new points between two sampled data points. This is done in the convolve_wfm function. Sorry for the confusion with the function names. I will update the function names to better reflect what they are doing.
| std::pair<int, double> peakSampleVolt = WaveformUtil::FindHighestPeak(interp_wfm); | ||
|
|
||
| fFitPeak = peakSampleVolt.second; | ||
| fFitTime = bf + peakSampleVolt.first * fTimeStep / (float)N; |
There was a problem hiding this comment.
My apologies if I'm missing the math here. But this line seems to be indexing an upsampled waveform (therefore dividing sampling period by N) but I don't think any upsampling is done here (the convolution output waveform is N+M-1).
Again, I could be misunderstanding what you're trying to do. It would be nice if there's a brief explanation of the math being done as a writeup somewhere:)
There was a problem hiding this comment.
You are correct. I went back and looked at the code. There is an error in my implementation while convolving the kernel and the sampled waveform. I will redo the code and push the changes.
|
Thanks for the comments, James! I will work on these. |
Added a new WaveformAnalysisSinc class (based on the existing WaveformAnalysis class and the WaveformAnalysisLognormal class) to interpolate waveforms using tapered sinc kernel.