Addition of saturated flag in SiStrip Approximate Clusters #38870

abaty · 2022-07-27T12:34:15Z

This PR slightly extends the SiStripApproximateCluster data format to include a boolean flag which marks clusters which are saturated. This change was motivated by the discussion at [1]. The definition of 'saturated' we use here is the same as what is used in the SubClusterShapeFilter, i.e. a cluster having 3 consecutive strips of ADC>=254. Note that this flag is not currently used in the reconstruction sequence; this PR is simply to ensure this information is not dropped in the new approx. cluster data format. This is expected to increase the approximate cluster collection size by ~1% and barely affect the timing performance [2].

[1] #38423 (comment)
[2] https://twiki.cern.ch/twiki/pub/Main/HIDetectorReadout2020/SaturatedStrips.pdf

PR validation:

This PR was tested by running workflow 140.58 step 2, and checking the approximated cluster collection in the output.

@mandrenguyen @icali

cmsbuild · 2022-07-27T12:40:07Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38870/31290

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/SiStripCluster/src/classes_def.xml modified in PR(s): CUDA implementation of RecoLocalTracker/SiStripCluster ClustersFromRawProducer #34618

cmsbuild · 2022-07-27T12:40:30Z

A new Pull Request was created by @abaty for master.

It involves the following packages:

DataFormats/SiStripCluster (reconstruction)
RecoLocalTracker/SiStripClusterizer (reconstruction)

@jpata, @cmsbuild, @clacaputo can you please review it and eventually sign? Thanks.
@echabert, @VourMa, @swertz, @robervalwalsh, @yduhm, @GiacomoSguazzoni, @JanFSchulte, @rovere, @gbenelli, @alesaggio, @threus, @felicepantaleo, @gpetruc, @mmusich, @VinInn, @mtosi this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

mmusich · 2022-07-27T14:57:30Z

test parameters:

workflow = 140.58

mmusich · 2022-07-27T14:57:37Z

please test

cmsbuild · 2022-07-27T19:43:47Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4cbff4/26484/summary.html
COMMIT: ecc1619
CMSSW: CMSSW_12_5_X_2022-07-27-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38870/26484/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

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

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4cbff4/140.58_RunHI2018+RunHI2018+RAWPRIMEHI18+RECOHID18APPROXCLUSTERS+HARVESTDHI18

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 52
DQMHistoTests: Total histograms compared: 3728935
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3728911
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 51 files compared)
Checked 214 log files, 47 edm output root files, 52 DQM output files
TriggerResults: no differences found

jpata · 2022-08-02T10:25:08Z

DataFormats/SiStripCluster/src/classes_def.xml

- <class name="SiStripApproximateCluster" ClassVersion="3">
-  <version ClassVersion="3" checksum="2041370183"/>
+ <class name="SiStripApproximateCluster" ClassVersion="9">
+  <version ClassVersion="9" checksum="2854791577"/>


why did it jump from 3 -> 9?

I have updated this file to be version '4' now.

jpata · 2022-08-02T11:59:29Z

The size increase overall seems negligible, so once the class version is sorted out, it's good to go from my side.

140.58, 500 events, file size in bytes
step2.root: 2553402430 -> 2559880253
step3.root: 1159401041 -> 1159380736

cmsbuild · 2022-08-04T05:06:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38870/31416

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/SiStripCluster/src/classes_def.xml modified in PR(s): CUDA implementation of RecoLocalTracker/SiStripCluster ClustersFromRawProducer #34618

cmsbuild · 2022-08-04T05:06:56Z

Pull request #38870 was updated. @jpata, @cmsbuild, @clacaputo can you please check and sign again.

jpata · 2022-08-04T08:41:36Z

@cmsbuild please test

cmsbuild · 2022-08-04T13:30:29Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4cbff4/26637/summary.html
COMMIT: 26931c1
CMSSW: CMSSW_12_5_X_2022-08-03-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38870/26637/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

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

/data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4cbff4/140.58_RunHI2018+RunHI2018+RAWPRIMEHI18+RECOHID18APPROXCLUSTERS+HARVESTDHI18

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 52
DQMHistoTests: Total histograms compared: 3752952
DQMHistoTests: Total failures: 2
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3752928
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 51 files compared)
Checked 216 log files, 49 edm output root files, 52 DQM output files
TriggerResults: no differences found

jpata · 2022-08-05T09:05:38Z

+reconstruction

changes the SiStrip approximate cluster dataformat for HI
file size increase is minimal
no reco changes

cmsbuild · 2022-08-05T09:06:05Z

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

qliphy · 2022-08-05T12:32:47Z

+1

makortel · 2022-08-05T13:30:13Z

DataFormats/SiStripCluster/src/classes_def.xml

- <class name="SiStripApproximateCluster" ClassVersion="3">
-  <version ClassVersion="3" checksum="2041370183"/>
+ <class name="SiStripApproximateCluster" ClassVersion="4">
+  <version ClassVersion="4" checksum="2854791577"/>


This should have kept the checksum for class version 3 in addition of 4.

Thanks Matti. Is there a way to detect this at compile time (or in the tests)?

Nothing straightforward comes to my mind, but maybe we could figure out something. A specific check would certainly help to catch such cases. Could you open an issue about it?

done! #39008

@makortel

As pointed out by @makortel cms-sw#38870 (comment), we forgot to keep the old class version in cms-sw#38870.

added isSaturated boolean for approximatedClusters

ecc1619

cmsbuild added this to the CMSSW_12_5_X milestone Jul 27, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 27, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 27, 2022

cmsbuild added tests-started and removed tests-pending labels Jul 27, 2022

cmsbuild added tests-approved and removed tests-started labels Jul 27, 2022

jpata reviewed Aug 2, 2022

View reviewed changes

fixed class version for approx strip cluster

26931c1

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Aug 4, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 4, 2022

cmsbuild added tests-started and removed tests-pending labels Aug 4, 2022

cmsbuild added tests-approved and removed tests-started labels Aug 4, 2022

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Aug 5, 2022

cmsbuild added orp-approved and removed orp-pending labels Aug 5, 2022

cmsbuild merged commit eb2d3a5 into cms-sw:master Aug 5, 2022

makortel reviewed Aug 5, 2022

View reviewed changes

jpata added a commit to jpata/cmssw that referenced this pull request Aug 8, 2022

Readd class version SiStripApproximateCluster

6a99e95

As pointed out by @makortel cms-sw#38870 (comment), we forgot to keep the old class version in cms-sw#38870.

This was referenced Aug 8, 2022

Re-add class version SiStripApproximateCluster #38992

Merged

test for ClassVersion backwards compatibility #39008

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addition of saturated flag in SiStrip Approximate Clusters #38870

Addition of saturated flag in SiStrip Approximate Clusters #38870

abaty commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

mmusich commented Jul 27, 2022

mmusich commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

jpata Aug 2, 2022

abaty Aug 4, 2022

jpata commented Aug 2, 2022

cmsbuild commented Aug 4, 2022

cmsbuild commented Aug 4, 2022

jpata commented Aug 4, 2022

cmsbuild commented Aug 4, 2022

jpata commented Aug 5, 2022

cmsbuild commented Aug 5, 2022

qliphy commented Aug 5, 2022

makortel Aug 5, 2022 •

edited

jpata Aug 8, 2022 •

edited

makortel Aug 8, 2022

jpata Aug 9, 2022

Addition of saturated flag in SiStrip Approximate Clusters #38870

Addition of saturated flag in SiStrip Approximate Clusters #38870

Conversation

abaty commented Jul 27, 2022

PR validation:

cmsbuild commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

mmusich commented Jul 27, 2022

mmusich commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

Comparison Summary

jpata Aug 2, 2022

Choose a reason for hiding this comment

abaty Aug 4, 2022

Choose a reason for hiding this comment

jpata commented Aug 2, 2022

cmsbuild commented Aug 4, 2022

cmsbuild commented Aug 4, 2022

jpata commented Aug 4, 2022

cmsbuild commented Aug 4, 2022

Comparison Summary

jpata commented Aug 5, 2022

cmsbuild commented Aug 5, 2022

qliphy commented Aug 5, 2022

makortel Aug 5, 2022 • edited

Choose a reason for hiding this comment

jpata Aug 8, 2022 • edited

Choose a reason for hiding this comment

makortel Aug 8, 2022

Choose a reason for hiding this comment

jpata Aug 9, 2022

Choose a reason for hiding this comment

makortel Aug 5, 2022 •

edited

jpata Aug 8, 2022 •

edited