Hybrid zero suppression fixes #24597

pieterdavid · 2018-09-19T17:09:37Z

This PR collects a few fixes related to the hybrid zero-suppression (and emulation):

a configuration error: the "Hybrid" inspection method of the APV restorer should be used for hybrid data (this needs to be propagated to the HLT menu for HI; there was also a check to enfoce "BaselineFollower" as inspect method when it was used as restore method, that has been relaxed accordingly).
hybrid emulation needs the complete pedestals, but the global tag used in the relval to test it did not include these, so changed "auto:run2_data" to "auto:run2_hlt_hi" there
for the hybrid emulation, the "x->(x+1024)/2" transformation is applied to the MeanCM parameter (as it is to the VR digis), to align the interpretation with its online equivalent

@icali @CesarBernardes @erikbutz @mmusich

…use it in hybrid customisation)

…on relval step

… MeanCM parameter

cmsbuild · 2018-09-19T17:09:55Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-09-19T17:11:39Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-24597/6487

cmsbuild · 2018-09-19T17:11:55Z

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

It involves the following packages:

Configuration/PyReleaseValidation
RecoLocalTracker/SiStripZeroSuppression

@perrotta, @cmsbuild, @prebello, @zhenhu, @kpedro88, @pgunnell, @slava77 can you please review it and eventually sign? Thanks.
@echabert, @makortel, @felicepantaleo, @forthommel, @yduhm, @GiacomoSguazzoni, @gbenelli, @rovere, @VinInn, @nickmccoll, @alesaggio, @Martin-Grunewald, @gpetruc, @OlivierBondu, @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

slava77 · 2018-09-19T17:15:37Z

@cmsbuild please test workflow 140.55

cmsbuild · 2018-09-19T17:15:52Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/30517/console Started: 2018/09/19 20:22

cmsbuild · 2018-09-19T20:38:09Z

+1
Tested at: 4932552
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24597/30517/summary.html

cmsbuild · 2018-09-19T20:38:13Z

Comparison job queued.

cmsbuild · 2018-09-19T22:51:37Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24597/30517/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-24597/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: 3146746
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3146548
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

kpedro88 · 2018-09-20T16:45:17Z

+upgrade

cmsbuild · 2018-09-21T18:10:11Z

+1
Tested at: 77d2eda
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24597/30558/summary.html

cmsbuild · 2018-09-21T18:10:14Z

Comparison job queued.

cmsbuild · 2018-09-21T20:22:27Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-24597/30558/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-24597/140.55_RunHI2015VR+RunHI2015VR+HYBRIDRepackHI2015VR+HYBRIDZSHI2015+RECOHID15+HARVESTDHI15

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3146746
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3146547
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

pieterdavid · 2018-09-24T13:28:11Z

I'm adding a few more comparison plots between the hybrid scenario (in red; settings as in this PR) and the standard zero-suppression (in black; with CommonModeNoiseSubtractionMode set to "Median" to be as close as possible to the hybrid case). I produced the histograms with this config and the plugins in the same package.
Independent tests by @CesarBernardes show the same overall picture: only small changes, and a slightly modified cluster charge distribution (towards smaller average cluster charges).

CesarBernardes · 2018-09-24T14:29:15Z

Hi All, I also attach some plots for tracking, doing same comparison as Pieter did, i.e., standard ZS vs Hybrid ZS with Median method. I check the tracks with no selection and with a simple selection (High-purity, as defined in 2015 PbPb data-taking and pTerror/pT<0.3). Thanks!
trackingPlots_StandardZS_vs_Hybrid_24Sept2018.pdf

kpedro88 · 2018-09-24T14:46:41Z

+upgrade

slava77 · 2018-09-24T18:13:58Z

I'm comparing 200 events using

140.55 out of the box: in the baseline IB (a), a version with baseline+GT update (b), and IB+this PR (c)
"old ZS" version using step3 --step RAW2DIGI,REPACK:DigiToRawRepack --conditions auto:run2_hlt_hi --data --era Run2_HI --processName REHLT --scenario HeavyIons --datatier RAW --eventcontent RAW -n 200 --customise_command "process.load(\"RecoLocalTracker.SiStripZeroSuppression.SiStripZeroSuppression_cfi\")\nprocess.DigiToRawRepack.insert(0, process.siStripZeroSuppression)\n" with the same variations in the baseline IB (a), a version with baseline+GT update (b), and IB+this PR (c).
- @pieterdavid please confirm that this is a correct way of getting the reference to match your plots

2(b) is the same as 2(c): this is expected because the code changes are affecting only the hybrid mode. OK.
1(c) vs 2(c) are now closer, but we still have more short clusters and ~1.4% fewer tracks
[1(c) is in red and 2(c) is in black]

IIUC, the last plot is supposed to match the upper middle plot in https://github.com/cms-sw/cmssw/files/2411158/trackingPlots_StandardZS_vs_Hybrid_24Sept2018.pdf
@CesarBernardes your plots are in log scale and it's unclear if what I see appears also in your plots.

slava77 · 2018-09-24T19:57:11Z

+1

for #24597 77d2eda

code changes are in line with the PR description; changes are expected only in workflow(s) emulating/applying hybrid ZS in siStrips (e.g. wf 140.55, not a part of jenkins tests)
jenkins tests pass
local tests show moderate changes in HI tracking in line with the slides provided with this PR (see Hybrid zero suppression fixes #24597 (comment)). This achieves a better agreement between the two zero suppression schemes.

zhenhu · 2018-09-24T19:59:25Z

+1

cmsbuild · 2018-09-24T20:01:36Z

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 · 2018-09-24T21:19:33Z

+1

pieterdavid · 2018-09-25T12:09:30Z

@slava77 for the reference in my plots I also set process.siStripZeroSuppression.Algorithms.CommonModeNoiseSubtractionMode = "Median" (which is the setting for the emulation because that is what the firmware does, so I took the same for the reference to isolate the difference due to the hybrid scheme).
On another point: while investigating the difference in the cluster charge distribution I found another bug. With that fixed the low-level distributions are again nearly identical, I posted the details and comparison plots in the PR with the fix, #24654 .

slava77 · 2018-09-25T12:49:38Z

On 9/25/18 5:09 AM, Pieter David wrote: @slava77 for the reference in my plots I also set |process.siStripZeroSuppression.Algorithms.CommonModeNoiseSubtractionMode = "Median"| (which is the setting for the emulation because that is what the firmware does, so I took the same for the reference to isolate the difference due to the hybrid scheme). On another point: while investigating the difference in the cluster charge distribution I found another bug. With that fixed the low-level distributions are again nearly identical, I posted the details and comparison plots in the PR with the fix, #24654 .

OK. I see that the default in the 103X is otherwise siStripZeroSuppression.Algorithms.CommonModeNoiseSubtractionMode = 'IteratedMedian' I was looking at /cdaq/physics/Run2015HI/HeavyIon/500Bunches/v9.0/HLT/V1 the table used for run 263400 in wf 140.55. there in the HLTHIDoStripZeroSuppression sequence we also have hltSiStripZeroSuppression.Algorithms.CommonModeNoiseSubtractionMode = 'IteratedMedian' So far, I was considering this as the "old ZS". Similarly, for 304906 in /cdaq/physics/Run2017XeXe/HeavyIon/v1.2/HLT/V3 the same IteratedMedian is used. What is the reason for using Median?

icali · 2018-09-25T15:23:50Z

Just to clarify the workflow:

ZS from VR (old):
VR data->IteratedMedian->BaselineFollower->ZS data

ZS with hybrid:

inside the FED: good APVs: VR data ->Median ->ZS data
bad APVs: VR data are sent to output

in HLT: VR data -> IteratedMedian -> BaselineFollower -> ZS data (merged with good APVs ZS data)

While for the final zero suppression we want the IteratedMeadian, in order to compare old and new ZS we need to make a comparison with Median. The Median is indeed the less performing algorithm compared to the IteratedMedian.

pieterdavid added 3 commits September 19, 2018 16:58

APVRestorer: allow 'Hybrid' inspect mode with baseline follower (and …

dd1e28e

…use it in hybrid customisation)

Use auto:run2_hlt_hi (with complete pedestals) for the hybrid emulati…

16656cc

…on relval step

Hybrid emulation: correct for the x->(x+1024)/2 transformation in the…

4932552

… MeanCM parameter

cmsbuild added this to the CMSSW_10_3_X milestone Sep 19, 2018

cmsbuild added code-checks-pending comparison-pending orp-pending pdmv-pending pending-signatures reconstruction-pending tests-pending upgrade-pending labels Sep 19, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 19, 2018

cmsbuild added tests-started and removed tests-pending labels Sep 19, 2018

cmsbuild added tests-approved and removed tests-started labels Sep 19, 2018

cmsbuild added comparison-available and removed comparison-pending labels Sep 19, 2018

cmsbuild removed the upgrade-pending label Sep 20, 2018

cmsbuild added tests-approved and removed tests-started labels Sep 21, 2018

cmsbuild added comparison-available and removed comparison-pending labels Sep 21, 2018

cmsbuild added upgrade-approved and removed upgrade-pending labels Sep 24, 2018

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Sep 24, 2018

cmsbuild added fully-signed pdmv-approved and removed pdmv-pending pending-signatures labels Sep 24, 2018

cmsbuild added orp-approved and removed orp-pending labels Sep 24, 2018

cmsbuild merged commit 4f2e47a into cms-sw:master Sep 24, 2018

pieterdavid mentioned this pull request Sep 25, 2018

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hybrid zero suppression fixes #24597

Hybrid zero suppression fixes #24597

pieterdavid commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

slava77 commented Sep 19, 2018

cmsbuild commented Sep 19, 2018 •

edited

Loading

cmsbuild commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

kpedro88 commented Sep 20, 2018

cmsbuild commented Sep 21, 2018

cmsbuild commented Sep 21, 2018

cmsbuild commented Sep 21, 2018

pieterdavid commented Sep 24, 2018

CesarBernardes commented Sep 24, 2018

kpedro88 commented Sep 24, 2018

slava77 commented Sep 24, 2018

slava77 commented Sep 24, 2018

zhenhu commented Sep 24, 2018

cmsbuild commented Sep 24, 2018

fabiocos commented Sep 24, 2018

pieterdavid commented Sep 25, 2018

slava77 commented Sep 25, 2018 via email

icali commented Sep 25, 2018

Hybrid zero suppression fixes #24597

Hybrid zero suppression fixes #24597

Conversation

pieterdavid commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

slava77 commented Sep 19, 2018

cmsbuild commented Sep 19, 2018 • edited Loading

cmsbuild commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

cmsbuild commented Sep 19, 2018

kpedro88 commented Sep 20, 2018

cmsbuild commented Sep 21, 2018

cmsbuild commented Sep 21, 2018

cmsbuild commented Sep 21, 2018

pieterdavid commented Sep 24, 2018

CesarBernardes commented Sep 24, 2018

kpedro88 commented Sep 24, 2018

slava77 commented Sep 24, 2018

slava77 commented Sep 24, 2018

zhenhu commented Sep 24, 2018

cmsbuild commented Sep 24, 2018

fabiocos commented Sep 24, 2018

pieterdavid commented Sep 25, 2018

slava77 commented Sep 25, 2018 via email

icali commented Sep 25, 2018

cmsbuild commented Sep 19, 2018 •

edited

Loading