fwhm, rise/fall time #2151
fwhm, rise/fall time #2151
Conversation
|
a good unit test here might be to cross-check with width, width_height, l_ips, r_ips = peak_widths(
signal,
peaks=[peak,],
rel_height=0.5,
)Just a quick test on some waveforms shows the scipy function works reasonably well, as long as the peak is not at the edges: |
|
Alternatively to this implementation for fwhm, one could do the same as for time over threshold with a slightly worst result but quicker and maybe interpolate before. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2151 +/- ##
==========================================
- Coverage 92.55% 92.32% -0.23%
==========================================
Files 234 234
Lines 20006 20073 +67
==========================================
+ Hits 18516 18532 +16
- Misses 1490 1541 +51 ☔ View full report in Codecov by Sentry. |
|
Do you know why some tests, unrelated to the implementation, are failing? |
Yes, it's due to a change in scikit learn that causes a problem with astropy. @maxnoe opened a PR to fix it, so shortly you should be able to just merge or rebase the main branch and your tests should pass again. #2496 Feel free to review it! We need a second reviewer to commit (and it's a minimal change)... |
| nopython=True, | ||
| cache=True, | ||
| ) | ||
| def time_parameters(waveform, fwhm_arr, rise_time_arr, fall_time_arr): |
There was a problem hiding this comment.
I think this new method is missing in __all__ at the top.
|
|
||
| """ | ||
| peak_index = np.argmax(waveform) | ||
| amplitude = np.max(waveform) |
There was a problem hiding this comment.
This is wasteful, you don't need to find the maximum again, just do amplitude = waveform[peak_index]
| rt_10 = ti - (ampl_10percent - yi) / (yj - yi) | ||
|
|
||
| fwhm = 0.0 | ||
| if None not in (fwhm_right, fwhm_left): |
There was a problem hiding this comment.
You initialize these two with np.nan above, so None will never be in (fwhm_right, fwhm_left).
I think you can just subtract the two anyways, and the result will be nan if either is nan. Check if there is a warning and maybe silence it in case we don't want to have a warning like "invalid valid encountered in subtract".
| fwhm = fwhm_right - fwhm_left | ||
|
|
||
| rise_time = 0.0 | ||
| if None not in (rt_90, rt_10): |
| rise_time = rt_90 - rt_10 | ||
|
|
||
| fall_time = 0.0 | ||
| if None not in (ft_90, ft_10): |
| fall_time_arr[0] = fall_time | ||
|
|
||
|
|
||
| def time_over_threshold(waveforms, thr): |
There was a problem hiding this comment.
Technically, this is "samples above threshold", for "time over threshold you would have to divide by the sampling frequency.
No description provided.