-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GPU pixel local reconstruction: make the pixel cluster thresholds configurable #33259
GPU pixel local reconstruction: make the pixel cluster thresholds configurable #33259
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33259/21742
|
A new Pull Request was created by @czangela for master. It involves the following packages: RecoLocalTracker/SiPixelClusterizer @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
enable gpu |
RecoLocalTracker/SiPixelClusterizer/plugins/gpuClusterThresholds.h
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SiPixelClusterizer/plugins/gpuClusterThresholds.h
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SiPixelClusterizer/plugins/gpuClusterThresholds.h
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SiPixelClusterizer/plugins/SiPixelRawToClusterCUDA.cc
Outdated
Show resolved
Hide resolved
On Mar 24, 2021, at 5:01 PM, Vincenzo Innocente ***@***.***> wrote:
@VinInn commented on this pull request.
In RecoLocalTracker/SiPixelClusterizer/plugins/gpuClusterThresholds.h:
> @@ -0,0 +1,11 @@
+#ifndef RecoLocalTracker_SiPixelClusterizer_plugins_gpuClusterThresholds_h
why?
Its used somewhere other than this directory
…
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
In practice we allow header-only code from |
Ok - I didn’t realize. Thx
… On Mar 24, 2021, at 5:08 PM, Matti Kortelainen ***@***.***> wrote:
why?
Its used somewhere other than this directory
In practice we allow header-only code from plugins to be included in test of the same package.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33259/21744 |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33259/21745
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems there is a lot of unnecessary formatting going on.
RecoLocalTracker/Phase2TrackerRecHits/plugins/Phase2TrackerRecHits.cc
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SubCollectionProducers/interface/PixelClusterSelectorTopBottom.h
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SubCollectionProducers/src/HITrackClusterRemover.cc
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SubCollectionProducers/src/HLTTrackClusterRemoverNew.cc
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SubCollectionProducers/src/JetCoreClusterSplitter.cc
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SubCollectionProducers/src/PixelClusterSelectorTopBottom.cc
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SubCollectionProducers/src/SeedClusterRemover.cc
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SubCollectionProducers/src/SeedClusterRemoverPhase2.cc
Outdated
Show resolved
Hide resolved
RecoLocalTracker/SubCollectionProducers/src/StripClusterSelectorTopBottom.cc
Outdated
Show resolved
Hide resolved
…sFromSoA.cc Make cluster charge cut thresholds configurable for the function gpuClusterChargeCut Add inline function for threshold choosing change local includes, use phase1PixelTopology constant
566baed
to
822c264
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33259/21899
|
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bca299/14009/summary.html GPU Comparison SummarySummary:
Comparison SummarySummary:
|
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) |
+1 |
Validation plots/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW
/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW
/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAW
Validation plots (CPU vs GPU)/RelValTTbar_14TeV/CMSSW_11_2_0-PU_112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW/RelValZMM_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v14-v1/GEN-SIM-DIGI-RAW/RelValZEE_14/CMSSW_11_2_0-112X_mcRun3_2021_realistic_v13-v1/GEN-SIM-DIGI-RAWThroughput plots/EphemeralHLTPhysics1/Run2018D-v1/RAW run=323775 lumi=53logs and
|
PR description:
This is an effort towards solving one of the issues in #32483.
Hardcoded charge cuts on pixel clusters were used in:
RecoLocalTracker/SiPixelClusterizer/plugins/gpuClusterChargeCut.h
andRecoLocalTracker/SiPixelClusterizer/plugins/SiPixelDigisClustersFromSoA.cc
Since the GPU version uses these thresholds in the free function
gpuClusterChargeCut
nested in thegpuClustering
namespace, they are propagated through function parameters, passing them by value. They are initially set in theSiPixelRawToClusterCUDA
class.The
SiPixelDigisClustersFromSoA
has these parameters as private members (as does the legacy version).PR validation:
These changes should not affect validation results.
Performance should not be affected.
if this PR is a backport please specify the original PR and why you need to backport that PR: