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

PMAPs code confuses RMS and standard deviation #928

Open
atrettin opened this issue Feb 19, 2025 · 3 comments
Open

PMAPs code confuses RMS and standard deviation #928

atrettin opened this issue Feb 19, 2025 · 3 comments
Assignees
Labels
easy enhancement good first issue Issues for people willing to get into IC

Comments

@atrettin
Copy link

When the rms attribute of a Peak is calculated, the code actually returns the standard deviation. The repr of the Peak prints out RMS: {self.rms / units.mus} µs, which is an incorrect labeling. The given quantity is actually the standard deviation. The RMS should correspond to the square of the standard deviation and have units of µs^2, accordingly.

I propose that the attribute rms should contain the square of the standard deviation, and that a separate std attribute is added to contain the standard deviation.

self.rms = self. rms_above_threshold(0)

def rms_above_threshold(self, thr):
i_above_thr = self.pmts.where_above_threshold(thr)
times_above_thr = self.times[i_above_thr]
wf_above_thr = self.pmts.sum_over_sensors[i_above_thr]
if np.size(i_above_thr) < 2 or np.sum(wf_above_thr) == 0:
return 0
return weighted_mean_and_std(times_above_thr, wf_above_thr)[1]

RMS: {self.rms / units.mus} µs

@atrettin atrettin added bug easy good first issue Issues for people willing to get into IC labels Feb 19, 2025
@gonzaponte
Copy link
Collaborator

RMS stands for root mean square, i.e. the square root of the variance. RMS and std coincide when the average of the population is 0 and, in an abuse of the language, the terms are usually used interchangeably. Here we push these definitions a bit further by applying it to a waveform where time is the variable and the waveform amplitude its weight.

That being said, I agree that the correct term would be std and should replace RMS.

@atrettin atrettin added enhancement and removed bug labels Feb 24, 2025
@atrettin
Copy link
Author

Oh, this is actually a bit embarrassing, yes, you're right. I've removed the "bug" flag, because this isn't one. It would still be good to replace RMS with "std" to avoid this confusion, and while we are at it we could also add a "var" property containing the variance. Even though the difference is just a square root, I think that it would be useful because we have a direct linear relationship between the variance and the drift time, so the variance is physically more interesting IMO.

@gonzaponte
Copy link
Collaborator

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy enhancement good first issue Issues for people willing to get into IC
Projects
None yet
Development

No branches or pull requests

3 participants