Waveform cleaner #973
Waveform cleaner #973
Conversation
…tracted from the average of some selected samples in the waveform
ctapipe/image/waveform_cleaning.py
Outdated
| estimated in a window of "baseline_width" samples, shifted of | ||
| "window_shift" samples | ||
| """ | ||
| baseline_width = Int(10, help='Define then number of samples for estimating the ' |
There was a problem hiding this comment.
If window_shift is not equal to zero then it does not represents the baseline_width anymore right ?.
I would suggest to call the two parameters : sample_start sample_end
ctapipe/image/waveform_cleaning.py
Outdated
| @@ -78,7 +78,8 @@ class BaselineWaveformCleaner(WaveformCleaner): | |||
|
|
|||
| def apply(self, waveforms): | |||
| # Subtract baseline | |||
| baseline_sub = waveforms - np.mean(waveforms[:, :, self.sample_start:self.sample_end], | |||
| baseline_sub = waveforms - \ | |||
There was a problem hiding this comment.
Please don't use explicit line contiuations with \, use parenthese.
Maybe even split like this:
baseline = np.mean(
waveforms...,
axis=2,
)
return waveforms - baseline[:, :, None]
|
It would be very helpful to me if this very simple PR could be rapidly accepted (it would avoid the copy of the waveform_cleaning.py module in lstchain). Thanks! |
ctapipe/image/waveform_cleaning.py
Outdated
|
|
||
| def apply(self, waveforms): | ||
| # Subtract baseline | ||
| baseline = waveforms - np.mean( |
There was a problem hiding this comment.
Very minor note, but its not really "baseline" you obtain here, but rather "waveform_corrected" instead
There was a problem hiding this comment.
Hi Jason, actually it seems to me that the name an the description of this very simple and short function is enough clear.
I have now a question. Who is supposed to accept PRs in ctapipe? For such simple type of PRs (reviewed by at least 3 persons) it would be correct to avoid long delay times.
There was a problem hiding this comment.
Let me be clearer. The variable you obtain on line 81 is currently called baseline. But it is not baseline, it is the waveform subtracted by the baseline, and is misleading.
Indeed, this PR should be accepted very quickly.
There was a problem hiding this comment.
Ah, you are right, I misunderstood :-)
Yes, l change it rapidly
|
Please, could this PR be taken into account? |
Codecov Report
@@ Coverage Diff @@
## master #973 +/- ##
=========================================
Coverage ? 78.19%
=========================================
Files ? 194
Lines ? 11313
Branches ? 0
=========================================
Hits ? 8846
Misses ? 2467
Partials ? 0
Continue to review full report at Codecov.
|
Add simple waveform-cleaner to get a baseline subtracted waveform