Iss407 pedestal refactor - #455
Conversation
tomeichlersmith
left a comment
There was a problem hiding this comment.
This is just a first pass - I have not tried to run it myself yet, but the actual algorithm looks good. I just want to avoid one pitfall and drop one extra local variable that I don't believe is necessary.
| auto [i_erx, i_ch] = mapping.toErxChannel(i_roc, ch); | ||
| adcs[i] = data[i].soi().channel(i_erx, i_ch).adc(); | ||
| } | ||
| stdevs[ch] = pflib::utility::stdev(adcs); |
There was a problem hiding this comment.
This is silently converting the double into an int, probably just truncating off everything after the decimal point. This is a problem because a channel could be quiet and have a noise of 0.9 ADC which then gets truncated to 0 and ignored later in the tuning.
My suggestion is to never do the sqrt, just use the variance so that we never convert to double. That way, we can check for dead channels by exact equality with zero.
| daq_run(tgt, "PEDESTAL", buffer, n_events, 100); | ||
| pflib_log(trace) << "baseline run done, getting channel medians"; | ||
| std::map<int, std::array<int, 72>> medians; | ||
| std::map<int, std::array<int, 72>> stdevs; |
There was a problem hiding this comment.
While it is probably optimized out, there is no reason to go through this extra stdevs local variable. The only reason I have the local medains variable is so that we can compute the target later (which requires sorting) after copying out the values on a per-channel basis (where we want to keep the original, channel-indexed order).
66d513e to
9393cb4
Compare
|
I couldn't avoid running into the same issue with variance as standard deviation, so I just attempted to keep is as double the whole way rather than adding new code for a variance function. Does this work? |
Opening a PR but may need to do some extra work before merging.
For local pedestal leveling, change baseline
TRIM_INVfrom0to32, add adaptive support for bothECALandHCALROCs, and mask dead channels (i.e., those with 0 standard deviation). Seeing some difficulty leveling for ECAL (see image). May just be running out of range withTRIM_INV, but someone should try to run this on their own to verify behavior.Note: did not yet test for HCAL