Fix APV shot cleaner for 10bit #24961
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24961/6933 |
|
please test |
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @pieterdavid (Pieter David) for master. It involves the following packages: RecoLocalTracker/SiStripClusterizer @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| @@ -133,9 +133,10 @@ void SiStripApvShotCleaner::subtractCM(){ | |||
| return; | |||
|
|
|||
| //Subtract the median | |||
| const bool is10bit = apvDigis[0].adc() > 255; // approximation; definitely 10bit in this case | |||
There was a problem hiding this comment.
Following your intentions explained in the PR description, I would expect here
const bool is10bit = apvDigis[0].adc() > 255;
for(size_t i=1; i<stripsForMedian;++i) is10bit |= apvDigis[i].adc() > 255;
Did I understand it correctly?
There was a problem hiding this comment.
Yes, but they are sorted by decreasing adc here (from https://github.com/cms-sw/cmssw/pull/24961/files#diff-af819317b5a71697a837cc86996841edL115 , to calculate the median) so the first is the largest (the implementation of this method could clearly be improved, but I wanted to keep the patch minimal).
|
+1
|
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
As discussed in https://hypernews.cern.ch/HyperNews/CMS/get/tier0-Ops/2029/1/1/1/2/1/1/1/1.html point 4: a fix for the APV shot cleaner (which performs an "emergency zero-suppression") for 10bit data.
The problem was that 254 and 255 have special meanings in 8-bit truncation, see https://github.com/cms-sw/cmssw/blob/master/RecoLocalTracker/SiStripZeroSuppression/interface/SiStripFedZeroSuppression.h#L38 (saturated strips, 254-1022 or even above), so values above 253 were not CM-subtracted here.
The fix exploits this by concluding that if any digi is larger than 255, the 8bit scheme is not used, and all digis can be CM-subtracted (for typical hybrid data the baseline is above 500). The hypothetical case of 10bit data with all digis below 254 is also handled correctly; only if the largest digi is 254 or 255 the detection doesn't work (but this should not happen in practice).
Changes expected: none, expect for data taken in hybrid mode on which the (hybrid-adjusted) zero-suppression and repacking was not run in the HLT (like 324623, possibly also cosmics during the HI run).
CC: @icali