Use phase1PixelTopology::numberOfLayers where appropriate #33244

czangela · 2021-03-22T10:30:30Z

PR description:

Change hardcoded number of layer values.

Related to the issues listed in #32483, and this comment.

PR validation:

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

cmsbuild · 2021-03-22T10:37:56Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33244/21696

This PR adds an extra 16KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33244/21696/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33244/21696/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2021-03-22T10:52:32Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33244/21697

This PR adds an extra 16KB to repository

cmsbuild · 2021-03-22T10:52:54Z

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

It involves the following packages:

Geometry/TrackerGeometryBuilder
RecoLocalTracker/SiPixelRecHits

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

cms-bot commands are listed here

perrotta · 2021-03-22T11:05:41Z

please test

cmsbuild · 2021-03-22T11:18:13Z

-1

Failed Tests: Build
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9446e7/13658/summary.html
COMMIT: 4d1f89a
CMSSW: CMSSW_11_3_X_2021-03-21-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33244/13658/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

Migrated Validation/*Tracker* to esConsumes #33238 @mmusich: Migrated Validation/Tracker to esConsumes
HFNose: ticl updates #33001 @mariadalfonso: HFNose: ticl updates
Bump of CLHEP::Hep3Vector ClassVersion needed for GEANT4 10.7.1 #33234 @silviodonato: Bump of CLHEP::Hep3Vector ClassVersion needed for GEANT4 10.7.1
Adding Phase2 tracker T25 and T26 (for IT sensor technology studies) #33222 @adewit: Adding Phase2 tracker T25 and T26 (for IT sensor technology studies)
migrate Validation/RecoTrack to esConsumes #33235 @mmusich: migrate Validation/RecoTrack to esConsumes

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9446e7/13658/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9446e7/13658/git-merge-result

Build

I found compilation error when building:

>> Building LCG reflex dict from header file src/DataFormats/CLHEP/src/classes.h
>> Compiling LCG dictionary: tmp/slc7_amd64_gcc900/src/DataFormats/CLHEP/src/DataFormatsCLHEP/a/DataFormatsCLHEP_xr.cc
>> Building  shared library tmp/slc7_amd64_gcc900/src/DataFormats/CLHEP/src/DataFormatsCLHEP/libDataFormatsCLHEP.so
Copying tmp/slc7_amd64_gcc900/src/DataFormats/CLHEP/src/DataFormatsCLHEP/libDataFormatsCLHEP.so to productstore area:
>> Checking EDM Class Version for src/DataFormats/CLHEP/src/classes_def.xml in libDataFormatsCLHEP.so
error: class 'CLHEP::Hep3Vector' has a different checksum for ClassVersion 11. Increment ClassVersion to 12 and assign it to checksum 3040806103
Suggestion: You can run 'scram build updateclassversion' to generate src/DataFormats/CLHEP/src/classes_def.xml.generated with updated ClassVersion
gmake: *** [tmp/slc7_amd64_gcc900/src/DataFormats/CLHEP/src/DataFormatsCLHEP/libDataFormatsCLHEP.so] Error 1
Leaving library rule at DataFormats/CLHEP
>> Leaving Package DataFormats/CLHEP
>> Package DataFormats/CLHEP built

mmusich · 2021-03-22T11:24:30Z

the build failure looks related to #33234

perrotta · 2021-03-22T11:36:32Z

the build failure looks related to #33234

yeah: let wait for the next IB, to re-launch the tests when that PR will have been already merged

perrotta · 2021-03-22T15:14:29Z

please test

cmsbuild · 2021-03-22T15:27:58Z

-1

Failed Tests: Build
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9446e7/13667/summary.html
COMMIT: 4d1f89a
CMSSW: CMSSW_11_3_X_2021-03-21-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33244/13667/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

Migrated Validation/*Tracker* to esConsumes #33238 @mmusich: Migrated Validation/Tracker to esConsumes
HFNose: ticl updates #33001 @mariadalfonso: HFNose: ticl updates
Bump of CLHEP::Hep3Vector ClassVersion needed for GEANT4 10.7.1 #33234 @silviodonato: Bump of CLHEP::Hep3Vector ClassVersion needed for GEANT4 10.7.1
Adding Phase2 tracker T25 and T26 (for IT sensor technology studies) #33222 @adewit: Adding Phase2 tracker T25 and T26 (for IT sensor technology studies)
migrate Validation/RecoTrack to esConsumes #33235 @mmusich: migrate Validation/RecoTrack to esConsumes

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9446e7/13667/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9446e7/13667/git-merge-result

Build

I found compilation error when building:

>> Building LCG reflex dict from header file src/DataFormats/CLHEP/src/classes.h
>> Compiling LCG dictionary: tmp/slc7_amd64_gcc900/src/DataFormats/CLHEP/src/DataFormatsCLHEP/a/DataFormatsCLHEP_xr.cc
>> Building  shared library tmp/slc7_amd64_gcc900/src/DataFormats/CLHEP/src/DataFormatsCLHEP/libDataFormatsCLHEP.so
Copying tmp/slc7_amd64_gcc900/src/DataFormats/CLHEP/src/DataFormatsCLHEP/libDataFormatsCLHEP.so to productstore area:
>> Checking EDM Class Version for src/DataFormats/CLHEP/src/classes_def.xml in libDataFormatsCLHEP.so
error: class 'CLHEP::Hep3Vector' has a different checksum for ClassVersion 11. Increment ClassVersion to 12 and assign it to checksum 3040806103
Suggestion: You can run 'scram build updateclassversion' to generate src/DataFormats/CLHEP/src/classes_def.xml.generated with updated ClassVersion
gmake: *** [tmp/slc7_amd64_gcc900/src/DataFormats/CLHEP/src/DataFormatsCLHEP/libDataFormatsCLHEP.so] Error 1
Leaving library rule at DataFormats/CLHEP
>> Leaving Package DataFormats/CLHEP
>> Package DataFormats/CLHEP built

perrotta · 2021-03-22T15:29:36Z

(Still building on top of CMSSW_11_3_X_2021-03-21-2300: let wait for some more time...)

smuzaffar · 2021-03-22T16:00:16Z

please test
newer IB is available now

cmsbuild · 2021-03-22T18:31:03Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9446e7/13670/summary.html
COMMIT: 4d1f89a
CMSSW: CMSSW_11_3_X_2021-03-22-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33244/13670/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2639935
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2639900
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

cvuosalo · 2021-03-22T21:34:56Z

RecoLocalTracker/SiPixelRecHits/plugins/SiPixelRecHitSoAFromLegacy.cc

+                                output->iphi(),
+                                output->hitsLayerStart(),
+                                numberOfHits,
+                                256,


256 should probably be a declared constant.

Yes, but not just here. 256 as the number of threads is hardcoded in many places.
Should this be added as an issue to #32483 ? @fwyzard @mmusich @VinInn
edit: By "this" I meant making the number of threads / threadsPerBlock constants configurable, or setting them in one place? Also, if there were previous discussions about this, could you guide me to them?

I'd say that this is a more general issue, that applies to all CUDA kernels, not just the Pixel ones

When/if anybody will come with a good reason to make configurable it will be done. Until then no reason to name it.

cvuosalo · 2021-03-22T21:35:05Z

+1

perrotta · 2021-03-23T06:35:10Z

+1

Use (already existing) named constants instead of hardcoded number of layers
Jenkins tests pass, with no (relevant) differences)

cmsbuild · 2021-03-23T06:35: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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

silviodonato · 2021-03-23T09:57:09Z

+1

Use phase1PixelTopology::numberOfLayers where appropriate

ad97afe

cmsbuild added this to the CMSSW_11_3_X milestone Mar 22, 2021

cmsbuild added code-checks-pending geometry-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Mar 22, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels Mar 22, 2021

clang format

4d1f89a

cmsbuild added code-checks-pending and removed code-checks-rejected labels Mar 22, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 22, 2021

cmsbuild added tests-started and removed tests-pending labels Mar 22, 2021

cmsbuild added tests-rejected and removed tests-started labels Mar 22, 2021

cmsbuild added tests-started and removed tests-rejected labels Mar 22, 2021

cmsbuild added tests-rejected and removed tests-started labels Mar 22, 2021

cmsbuild added tests-started and removed tests-rejected labels Mar 22, 2021

cmsbuild added tests-approved and removed tests-started labels Mar 22, 2021

cvuosalo reviewed Mar 22, 2021

View reviewed changes

cmsbuild added geometry-approved and removed geometry-pending labels Mar 22, 2021

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Mar 23, 2021

cmsbuild added orp-approved and removed orp-pending labels Mar 23, 2021

cmsbuild merged commit 8c93a85 into cms-sw:master Mar 23, 2021

czangela deleted the pixel_local_reco_refactorings_22_03_2021 branch March 23, 2021 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use phase1PixelTopology::numberOfLayers where appropriate #33244

Use phase1PixelTopology::numberOfLayers where appropriate #33244

czangela commented Mar 22, 2021 •

edited

Loading

cmsbuild commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

perrotta commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

mmusich commented Mar 22, 2021

perrotta commented Mar 22, 2021

perrotta commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

perrotta commented Mar 22, 2021

smuzaffar commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

cvuosalo Mar 22, 2021

czangela Mar 23, 2021 •

edited

Loading

fwyzard Mar 23, 2021

VinInn Mar 23, 2021

cvuosalo commented Mar 22, 2021

perrotta commented Mar 23, 2021

cmsbuild commented Mar 23, 2021

silviodonato commented Mar 23, 2021

Use phase1PixelTopology::numberOfLayers where appropriate #33244

Use phase1PixelTopology::numberOfLayers where appropriate #33244

Conversation

czangela commented Mar 22, 2021 • edited Loading

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 Mar 22, 2021

cmsbuild commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

perrotta commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

Build

mmusich commented Mar 22, 2021

perrotta commented Mar 22, 2021

perrotta commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

Build

perrotta commented Mar 22, 2021

smuzaffar commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

Comparison Summary

cvuosalo Mar 22, 2021

Choose a reason for hiding this comment

czangela Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

fwyzard Mar 23, 2021

Choose a reason for hiding this comment

VinInn Mar 23, 2021

Choose a reason for hiding this comment

cvuosalo commented Mar 22, 2021

perrotta commented Mar 23, 2021

cmsbuild commented Mar 23, 2021

silviodonato commented Mar 23, 2021

czangela commented Mar 22, 2021 •

edited

Loading

czangela Mar 23, 2021 •

edited

Loading