Bug fix: truncate when copying digis in hybrid zero-suppression

[pieterdavid]

This change should fix most of the differences observed between standard zero-suppression and the hybrid mode in #24597 (e.g. in the cluster charge distribution). I'm sorry for figuring it out just after the other PR got merged.
In the currently planned scenario (10bit hybridZS data taking, 8bit repacking in the HLT), the following happens: for the "non-flagged" APVs (without modified baseline), baseline subtraction and zero-suppression are performed, and digis saved with 10bit precision (in the emulation as well as in the FED firmware). When running the remaining zero-suppression in software afterwards, the "flagged" APVs are zero-suppressed and their digis brought in the range 0-255 ([255,1022]-> 254, 1023->255), and repacked in the 8bit ZS raw format.
The problem is that for the non-flagged APVs the 10bit digis were simply copied, but the packer assumes they are also already 8bit-truncated and simply discards the two most significant bits (so adc % 256 instead of 254 (or 255)). Therefore, clusters with adc>255 strips got a lower total charge or were split.

The following plots show the number of digis and clusters, and cluster charge and width, in the hybrid zero-suppressed scenario (red) compared to standard zero-suppression with process.siStripZeroSuppression.Algorithms.CommonModeNoiseSubtractionMode = "Median" (black) to maximally align the zero-suppression settings, to be compared with those in #24597 (comment) .

CC: @icali @CesarBernardes @erikbutz @mmusich @echabert

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24654/6559

[cmsbuild]

A new Pull Request was created by @pieterdavid (Pieter David) for master.

It involves the following packages:

RecoLocalTracker/SiStripZeroSuppression

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@echabert, @makortel, @felicepantaleo, @forthommel, @yduhm, @GiacomoSguazzoni, @gbenelli, @rovere, @VinInn, @nickmccoll, @alesaggio, @gpetruc, @ebrondol, @threus, @mmusich this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

[perrotta]

please test

[cmsbuild]

The tests are being triggered in jenkins.

[slava77]

@cmsbuild please abort

[cmsbuild]

Jenkins tests are aborted.

[slava77]

@cmsbuild please test workflow 140.55

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30608/console Started: 2018/09/25 14:37

[slava77]

@pieterdavid
please share some plots of incremental differences from just this PR.
Thank you.

[cmsbuild]

+1
Tested at: a4bc48d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24654/30608/summary.html

[cmsbuild]

Comparison job queued.

[pieterdavid]

@slava77 the plots below have the same distributions as above, zoomed to the previously most discrepant regions. I changed normal ZS with Median CMN from black to green - but it is mostly hidden by the red histogram (hybrid ZS with this PR) anyway, and added hybrid ZS without this PR (in blue), and in black also the standard ZS with IteratedMedian (in response to your question in #24597 (comment)).
The difference between Median and IteratedMedian is mostly visible in the number of digis and cluster size/charge distributions: more digis and fewer small clusters. The hybrid scenario is much closer to both of these with (red) than without (blue) this PR (the last plot, with the cluster charge, is the clearest). The ratio compares the new hybrid case to the standard (IteratedMedian) ZS.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24654/30608/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

• /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-24654/140.55_RunHI2015VR+RunHI2015VR+HYBRIDRepackHI2015VR+HYBRIDZSHI2015+RECOHID15+HARVESTDHI15

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 32
• DQMHistoTests: Total histograms compared: 3146876
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 3146677
• DQMHistoTests: Total skipped: 197
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
• Checked 133 log files, 14 edm output root files, 32 DQM output files

[slava77]

comparing 140.55 CMSSW_10_3_X_2018-09-19-1100+#24597 as a baseline (black) and with this PR on top in red.

there are a few % fewer short clusters (total number of clusters is down by 0.5%),

this, OTOH comes with a fraction of a % more tracks and the tracks look longer

this appears to match well the expected effects of the fix

[slava77]

type bugfix

[slava77]

+1

for #24654 a4bc48d

• the code change is as described; this affects only the SiStrip repacking with the hybrid mode.
• jenkins tests pass and comparisons with the baseline show no differences (jenkins tests do not include the HI repacking wf).
• local tests with wf 140.55 confirm expected behavior also illustrated with the plots provided with this PR.

[cmsbuild]

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)

[fabiocos]

+1

[slava77]

strip cluster size plot