Increase SiPixel L1 cluster threshold from 2000 to 4000e, synchronize channel threshold between HTL and offline #35518

ferencek · 2021-10-03T21:42:24Z

PR description:

Increase the SiPixel L1 cluster threshold from 2000 to 4000e making it the same as all other layers.
With the new L1 detector the lower thresholds should not be needed anymore as there is a clear separation between real clusters and junk (see the attached plot). The change will affect the cluster charge distribution for L1, the lower edge will shift from 2000
to 4000e.

In addition, the PR sets the SiPixelClusterProducer ChannelThreshold at the HLT to 10 to make it in sync with the offline reconstruction.

This is a resubmission of #35506 with the additional Run 3 modifiers for the GPU code and the related HLT modifiers.

PR validation:

Run RECO on raw data from the cosmic run 344420.

if this PR is a backport please specify the original PR and why you need to backport that PR:

No backport needed.

@dkotlins @mmusich

cmsbuild · 2021-10-03T21:51:14Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35518/25725

This PR adds an extra 12KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoLocalTracker/SiPixelClusterizer/python/SiPixelClusterizer_cfi.py modified in PR(s): Tracker phase2 bricked pixel localreco cmssw 12 1 x #35441

cmsbuild · 2021-10-03T21:51:40Z

A new Pull Request was created by @ferencek (Dinko F.) for master.

It involves the following packages:

RecoLocalTracker/SiPixelClusterizer (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@mtosi, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @OzAmram, @ferencek, @dkotlins, @gpetruc, @mmusich, @threus, @tvami this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

mmusich · 2021-10-03T22:38:53Z

Hi @ferencek, I think also the "legacy" HLT configuration should be updated at
HLTrigger/Configuration/python/customizeHLTforCMSSW.py

VinInn · 2021-10-04T08:23:15Z

Take also the opportunity to lower the ChannelThreshold to 10?
MInd: there are at least 4 clusterizers in HLT config that I know...

VinInn · 2021-10-04T08:24:52Z

@cmsbuild , please test

VinInn · 2021-10-04T08:25:00Z

enable gpu

mmusich · 2021-10-04T08:26:41Z

MInd: there are at least 4 clusterizers in HLT config that I know...

I think there's a way to reconfigure them all:

def customiseForXXXX(process): 
   for producer in producers_by_type(process, "SiPixelClusterProducer"): 
      producer.ChannelThreshold = cms.int32(10)
   return process

tsusa · 2021-10-04T08:36:28Z

MInd: there are at least 4 clusterizers in HLT config that I know...

I think there's a way to reconfigure them all:
def customiseForXXXX(process): 
   for producer in producers_by_type(process, "SiPixelClusterProducer"): 
      producer.ChannelThreshold = cms.int32(10)
   return process 

I am preparing PR for this part

ferencek · 2021-10-04T08:59:58Z

@tsusa, so you will take care of lowering the ChannelThreshold to 10 in a separate PR, right?

tsusa · 2021-10-04T09:38:49Z

@tsusa, so you will take care of lowering the ChannelThreshold to 10 in a separate PR, right?
rigth

cmsbuild · 2021-10-04T10:48:46Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35518/25731

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HLTrigger/Configuration/python/customizeHLTforCMSSW.py modified in PR(s): [WIP] add fillPSetDescription to CkfTrackCandidateMakerBase and propagate it to CkfTrackCandidateMaker #35385
- File RecoLocalTracker/SiPixelClusterizer/python/SiPixelClusterizer_cfi.py modified in PR(s): Tracker phase2 bricked pixel localreco cmssw 12 1 x #35441

mmusich · 2021-10-05T17:40:29Z

please test

cmsbuild · 2021-10-05T20:57:41Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce6491/19418/summary.html
COMMIT: 29b167f
CMSSW: CMSSW_12_1_X_2021-10-05-1100/slc7_amd64_gcc900
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35518/19418/install.sh to create a dev area with all the needed externals and cmssw changes.

GPU Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 4
DQMHistoTests: Total histograms compared: 19735
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 19729
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 3 files compared)
Checked 12 log files, 9 edm output root files, 4 DQM output files
TriggerResults: no differences found

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 557 differences found in the comparisons
DQMHistoTests: Total files compared: 40
DQMHistoTests: Total histograms compared: 3219394
DQMHistoTests: Total failures: 26186
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3193186
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 39 files compared)
Checked 169 log files, 37 edm output root files, 40 DQM output files
TriggerResults: found differences in 5 / 39 workflows

slava77 · 2021-10-05T22:06:36Z

I see that in the reco comparisons the differences are in the Run-3 workflows (expected); they start from the number of clusters and then occasionally propagate downstream to tracking and further higher level quantities.

In 2021 workflows (no PU) the number of clusters goes down by around 0.02%, which I guess is negligibly small
In the 2023 workflow 12434.0 (also no PU) there are 0.3% fewer clusters and 1.7% in BPIX1

are these noise clusters or is this going to lead to efficiency losses?

@cms-sw/trk-dpg-l2 please check/comment.
Thank you.

tsusa · 2021-10-06T15:57:57Z

@slava77, from the on-track cluster plot for the same wf (12434.0), which shows the difference in 1/486 entries, it seems that those are mostly noise clusters

slava77 · 2021-10-06T18:17:46Z

+reconstruction

for #35518 29b167f

code changes in the reco code are in line with the PR description
jenkins tests pass and comparisons with the baseline show expected rather small differences in the run3 workflows as described in Increase SiPixel L1 cluster threshold from 2000 to 4000e, synchronize channel threshold between HTL and offline #35518 (comment); the larger differences in the 2023 workflow was confirmed to be from noise hits
- there are also some unrelated small differences in 1325.81 (2017 nano) and 136.731 (2016 data reco) visible for ML inference, which is known to be not exactly reproducible on different architectures (SSE4.1 SSE4.2 AVX AVX2 FMA in the baseline vs SSE4.1 SSE4.2 AVX in this PR tests)

missirol · 2021-10-06T19:48:56Z

+hlt

non-technical updates
these two parameter changes will enter the next HLT menus in 12.1.X (once the menus are recreated and PR'd; until then, the changes are done via a customisation):
- ChannelThreshold = 10 is a fix which resyncs HLT to Offline (see HN thread)
- ClusterThreshold_L1 = 4000 is an update for the new Layer-1 of BPIX
- both changes apply to all instances of SiPixelClusterProducer at HLT
documented in CMSHLT-2165

cmsbuild · 2021-10-06T19:49:20Z

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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

fwyzard · 2021-10-07T09:19:58Z

HLTrigger/Configuration/python/customizeHLTforCMSSW.py

+        if hasattr(producer,"ChannelThreshold"):
+            producer.ChannelThreshold = 10


to clarify, this one change is independent and unrelated to the other ones, right ?

Yes, technically the ClusterThreshold_L1 change is decoupled from the ChannelThreshold change. The former is driven by an actual change in the detector (replace pixel L1) while the latter is fixing a historical oversight where the HLT and offline were not using the same ChannelThreshold value.

fwyzard · 2021-10-07T09:29:34Z

which parts of these changes should be backported to 12.0.x ?

fwyzard · 2021-10-07T10:16:36Z

Please note that this PR should be merged at the same time as #35570 , to make sure the GPU customisation of the HLT remains coherent with the changes introduced here.

ferencek · 2021-10-07T11:33:40Z

which parts of these changes should be backported to 12.0.x ?

Good question. Danek originally indicated no backports are needed for the ClusterThreshold_L1 change. On the other hand, I guess it would be nice to have these changes incorporated for the LHC beam test.

Are we even formally allowed to backport these changes as they would modify the local reco in a closed release? The impact on the high-level quantities should be tiny, though.

fwyzard · 2021-10-08T10:55:38Z

Good point - let's keep this only in 12.1.x then.

perrotta · 2021-10-09T07:48:24Z

+1

Small reduction of the number of pixels at Run3, mostly from noise and following the expectations

dkotlins added 2 commits October 2, 2021 16:02

increase l1 cluster threshold from 2000 to 4000e

f7af3d2

propagate changes of L1 threshold in pixel GPU code

be15488

cmsbuild added this to the CMSSW_12_1_X milestone Oct 3, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 3, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 3, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 4, 2021

introducer HLT modifiers for L1 cluster threshold

c129800

ferencek changed the title ~~Increase l1 cluster threshold from 2000 to 4000e~~ Increase L1 cluster threshold from 2000 to 4000e Oct 4, 2021

cmsbuild added code-checks-pending hlt-pending tests-pending and removed tests-started code-checks-approved labels Oct 4, 2021

cmsbuild added tests-started and removed tests-pending labels Oct 5, 2021

cmsbuild added tests-approved and removed tests-started labels Oct 5, 2021

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Oct 6, 2021

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Oct 6, 2021

fwyzard reviewed Oct 7, 2021

View reviewed changes

fwyzard mentioned this pull request Oct 7, 2021

Make the CUDA/SoA pixel clusterizer use the same thresholds as the legacy module #35570

Merged

fwyzard mentioned this pull request Oct 7, 2021

Make the CUDA/SoA pixel clusterizer use the same thresholds as the legacy module [12.0.x] #35571

Merged

cmsbuild added orp-approved and removed orp-pending labels Oct 9, 2021

cmsbuild merged commit 93ae6fc into cms-sw:master Oct 9, 2021

ferencek deleted the pix_clu_thr_l1_change2 branch October 11, 2021 09:50

cmsbuild mentioned this pull request Oct 12, 2021

rootmodule: Updated root to tip of branch master cms-sw/cmsdist#7382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase SiPixel L1 cluster threshold from 2000 to 4000e, synchronize channel threshold between HTL and offline #35518

Increase SiPixel L1 cluster threshold from 2000 to 4000e, synchronize channel threshold between HTL and offline #35518

ferencek commented Oct 3, 2021 •

edited

cmsbuild commented Oct 3, 2021

cmsbuild commented Oct 3, 2021

mmusich commented Oct 3, 2021

VinInn commented Oct 4, 2021

VinInn commented Oct 4, 2021

VinInn commented Oct 4, 2021

mmusich commented Oct 4, 2021

tsusa commented Oct 4, 2021

ferencek commented Oct 4, 2021

tsusa commented Oct 4, 2021

cmsbuild commented Oct 4, 2021

mmusich commented Oct 5, 2021

cmsbuild commented Oct 5, 2021

slava77 commented Oct 5, 2021

tsusa commented Oct 6, 2021 •

edited

slava77 commented Oct 6, 2021

missirol commented Oct 6, 2021

cmsbuild commented Oct 6, 2021

fwyzard Oct 7, 2021

ferencek Oct 7, 2021

fwyzard commented Oct 7, 2021

fwyzard commented Oct 7, 2021

ferencek commented Oct 7, 2021

fwyzard commented Oct 8, 2021

perrotta commented Oct 9, 2021

		if hasattr(producer,"ChannelThreshold"):
		producer.ChannelThreshold = 10

Increase SiPixel L1 cluster threshold from 2000 to 4000e, synchronize channel threshold between HTL and offline #35518

Increase SiPixel L1 cluster threshold from 2000 to 4000e, synchronize channel threshold between HTL and offline #35518

Conversation

ferencek commented Oct 3, 2021 • edited

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Oct 3, 2021

cmsbuild commented Oct 3, 2021

mmusich commented Oct 3, 2021

VinInn commented Oct 4, 2021

VinInn commented Oct 4, 2021

VinInn commented Oct 4, 2021

mmusich commented Oct 4, 2021

tsusa commented Oct 4, 2021

ferencek commented Oct 4, 2021

tsusa commented Oct 4, 2021

cmsbuild commented Oct 4, 2021

mmusich commented Oct 5, 2021

cmsbuild commented Oct 5, 2021

GPU Comparison Summary

Comparison Summary

slava77 commented Oct 5, 2021

tsusa commented Oct 6, 2021 • edited

slava77 commented Oct 6, 2021

missirol commented Oct 6, 2021

cmsbuild commented Oct 6, 2021

fwyzard Oct 7, 2021

Choose a reason for hiding this comment

ferencek Oct 7, 2021

Choose a reason for hiding this comment

fwyzard commented Oct 7, 2021

fwyzard commented Oct 7, 2021

ferencek commented Oct 7, 2021

fwyzard commented Oct 8, 2021

perrotta commented Oct 9, 2021

ferencek commented Oct 3, 2021 •

edited

tsusa commented Oct 6, 2021 •

edited