Skip to content
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

More Configurable Pixel Tracks and HIon Template #41632

Merged
merged 2 commits into from
Jul 16, 2023

Conversation

AdrianoDee
Copy link
Contributor

@AdrianoDee AdrianoDee commented May 11, 2023

This PR proposes some modifications to the GPU (CUDA based) pixel track reconstruction in order, also, to be run smoothly for HIon events. The modifications include:

  1. definition of a new HIonPhase1 pixel topology;
  2. adding maxNumClustersPerModules to TrackerTraits and increasing it to 2048 for HIon operations (and exampanding the blockPrefixScan for the clusterChargeCut);
  3. making the constants in gpuCalibPixel (such as the gains and the readout modes) configurable at run-time extending SiPixelClusterThresholds struct. This is not strictly needed but I feel would ease the things in future if these constants need to be changed;
  4. templating also SiPixelRawToClusterCUDA in order to include the changes when using an HIon topology (note that the name change is needed since, as far as I have understood, HLT would need to have both module names defined in the same release to do the migration);
  5. making maxNumberOfDoublets, z0Cut, ptCut and phiCuts configurable at run time (this will be needed to run offline pixel tracking/seeding on GPU);
  6. adding workflows for pixel tracking only reconstruction for HIon conditions for both "legacy" (*.5) and "Patatrack" (*.501,*.502) together with Hydjet_Quenched_MinBias_5020GeV_cfi and Hydjet_Quenched_MinBias_5362GeV_cfi in the matrix;
  7. adding Patatrack variants (.501 and .502) for GPU-like hiConformalPixelTracks for 160 workflow;
  8. allows to run pixel vertexing without the splitting kernel (doSplitting flag), useless for HIon operations, given also that too much shared memory would be needed for the average number of tracks per vertices (MAXTK) in PbPb conditions;

Any comment on this by HIon people is very welcome, especially on the w.f.s introduced.

Solves #40623 #37425.

HI Physics performance plots

HIon pixel tracking performance for:

  • online-like setup on with w.f. 14950.502_HydjetQMinBias_5362GeV+2023HI_Patatrack_PixelOnlyGPU here;
  • offline hiConformalPixelTracks with w.f. 160.502_HydjetQ_MinBias_5362GeV_2023_ppReco here;

Note: these performance may be improved or changed and the sizes of the structures should be better tuned to fit the T4@P5 memory. But I'd leave this to HIon people that know better than me what they need. This PR is here to give the skeleton/template and to may way easier the validation and tuning.

Run2 2018 Data Throughput plots

The pp performance are basically untouched both in physics and throughput.

On /EphemeralHLTPhysics1/Run2018D-v1/RAW run=323775 lumi=53

zoom-136.885502.png


10 July 2023 Update

Addressed comments (from @fwyzard). Mostly:

  • restoring constant names for clustering thresholds under CUDADataFormats/SiPixelCluster/interface/gpuClusteringConstants.h;
  • clean up, removing duplicates, extra lines, adding explanatory comments, using static_cast<T> where needed;
  • allowing the gpuClusterChargeCut to run for more than 2*maxThreads clusters per module;
  • protecting HIon and Phase 2 conflicts with ~phase2_tracker;

List of modules for @Martin-Grunewald .

Under DQM/SiPixelHeterogeneous/

-SiPixelHIonPhase1CompareRecHitsSoA
-SiPixelHIonPhase1CompareTrackSoA
-SiPixelHIonPhase1MonitorRecHitsSoA
-SiPixelHIonPhase1MonitorTrackSoA

Under RecoLocalTracker/SiPixelClusterizer/
-SiPixelDigisClustersFromSoAHIonPhase1
-SiPixelRawToClusterCUDAHIonPhase1

Under RecoLocalTracker/SiPixelRecHits/
-PixelCPEFastESProducerHIonPhase1
-SiPixelRecHitCUDAHIonPhase1
-SiPixelRecHitFromCUDAHIonPhase1
-SiPixelRecHitSoAFromCUDAHIonPhase1
-SiPixelRecHitSoAFromLegacyHIonPhase1

Under RecoTracker/PixelSeeding/
-CAHitNtupletCUDAHIonPhase1

Under RecoTracker/PixelTrackFitting/
-PixelTrackDumpCUDAHIonPhase1
-PixelTrackProducerFromSoAHIonPhase1
-PixelTrackSoAFromCUDAHIonPhase1

Under RecoTracker/PixelVertexFinding/
-PixelVertexProducerCUDAHIonPhase1


12 July 2023 Update

As sanity check, MTV plots by @CesarBernardes for HIon Conformal Pixel Tracks here before and after the comments.

Updated throughput plots on /EphemeralHLTPhysics1/Run2018D-v1/RAW run=323775 lumi=53:

Jobs Threads (per job) Streams Ref This PR Diff
2 6 6 1155.9 ± 5.0 ev/s 1154.4 ± 6.6 ev/s -0.13%
2 8 8 1187.9 ± 6.0 ev/s 1191.5 ± 10.2 ev/s +0.30%
2 10 10 1217.5 ± 6.6 ev/s 1214.2 ± 8.1 ev/s -0.24%
2 12 12 1217.3 ± 6.6 ev/s 1212.9 ± 5.3 ev/s -0.37%

(FYI @denerslemos)

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41632/35517

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

@@ -0,0 +1 @@
tests passed, failed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not!

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41632/35519

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @AdrianoDee (Adriano Di Florio) for master.

It involves the following packages:

  • CUDADataFormats/SiPixelCluster (heterogeneous, reconstruction)
  • CUDADataFormats/Track (heterogeneous, reconstruction)
  • CUDADataFormats/TrackingRecHit (heterogeneous, reconstruction)
  • Configuration/PyReleaseValidation (pdmv, upgrade)
  • DQM/SiPixelHeterogeneous (dqm)
  • Geometry/CommonTopologies (geometry)
  • RecoHI/HiTracking (reconstruction)
  • RecoLocalTracker/SiPixelClusterizer (reconstruction)
  • RecoLocalTracker/SiPixelRecHits (reconstruction)
  • RecoTracker/Configuration (reconstruction)
  • RecoTracker/PixelSeeding (reconstruction)
  • RecoTracker/PixelTrackFitting (reconstruction)
  • RecoTracker/PixelVertexFinding (reconstruction)
  • Validation/RecoTrack (dqm)

@civanch, @Dr15Jones, @kskovpen, @pmandrik, @bsunanda, @makortel, @bbilin, @fwyzard, @mdhildreth, @mandrenguyen, @cmsbuild, @AdrianoDee, @srimanob, @clacaputo, @syuvivida, @nothingface0, @sunilUIET, @tjavaid, @micsucmed, @emanueleusai, @rvenditti can you please review it and eventually sign? Thanks.
@VourMa, @felicepantaleo, @wmtford, @kpedro88, @Martin-Grunewald, @bsunanda, @fioriNTU, @threus, @mmusich, @slomeo, @hdelanno, @makortel, @JanFSchulte, @dgulhan, @missirol, @yenjie, @ferencek, @mandrenguyen, @yetkinyilmaz, @GiacomoSguazzoni, @rovere, @VinInn, @mroguljic, @jandrea, @idebruyn, @ebrondol, @mtosi, @fabiocos, @jazzitup, @dkotlins, @kurtejung, @gpetruc, @tvami this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@AdrianoDee
Copy link
Contributor Author

test parameters:

  • enable_tests = gpu
  • workflows_gpu = 12434.502, 14950.502, 160.502
  • workflows = 12434.501, 14950.501, 160.501
  • relvals_opt = -w upgrade,gpu
  • relvals_opt_gpu = -w upgrade,gpu

hiPixelTracksCUDA = _pixelTracksCUDA.clone(pixelRecHitSrc="siPixelRecHitsPreSplittingCUDA", idealConditions = False,
ptmin = 0.25, hardCurvCut = 0.0756, doPtCut = False,
onGPU = True,
phiCuts = cms.vint32([900 for i in range(19)]), #19 pairs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
phiCuts = cms.vint32([900 for i in range(19)]), #19 pairs
phiCuts = cms.vint32(19*[900]), #19 pairs

should work as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And looks better: applied.

ptmin = 0.25, hardCurvCut = 0.0756, doPtCut = False,
onGPU = True,
phiCuts = cms.vint32([900 for i in range(19)]), #19 pairs
trackQualityCuts = cms.PSet(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be

Suggested change
trackQualityCuts = cms.PSet(
trackQualityCuts = dict(

and then drop all cms.sometype specifications

@@ -16,7 +16,7 @@ def _addNoFlow(module):
if not tmp[ind-1] in _noflowSeen:
module.noFlowDists.append(tmp[ind-1])

_defaultSubdirs = ["Tracking/Track/*", "Tracking/TrackTPPtLess09/*", "Tracking/TrackFromPV/*", "Tracking/TrackFromPVAllTP/*", "Tracking/TrackAllTPEffic/*", "Tracking/TrackBuilding/*","Tracking/TrackConversion/*", "Tracking/TrackGsf/*"]
_defaultSubdirs = ["Tracking/Track/*", "Tracking/TrackTPPtLess09/*", "Tracking/HIPixelTrack/*" , "Tracking/TrackFromPV/*", "Tracking/TrackFromPVAllTP/*", "Tracking/TrackAllTPEffic/*", "Tracking/TrackBuilding/*","Tracking/TrackConversion/*", "Tracking/TrackGsf/*"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this addition be era-dependent? 1/8 increment of folders is not that large, but still, seeing HI it in pp is not that expected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, done

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41632/35520

@civanch
Copy link
Contributor

civanch commented Jul 13, 2023

+1

@perrotta
Copy link
Contributor

@cms-sw/pdmv-l2 your signature is the only thing missing before merging this PR and finally build the already two weeks late CMSSW_13_2_0_pre3. Please either sign or state clearly which is the status of your review and when you plan to conclude it.

@sunilUIET
Copy link
Contributor

+pdmv

@cmsbuild
Copy link
Contributor

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

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 9f75c01 into cms-sw:master Jul 16, 2023
@AdrianoDee AdrianoDee deleted the more_configurable_ca_hion branch July 19, 2023 09:55
cmsbuild added a commit that referenced this pull request Jul 27, 2023
Clean Up for Pixel Tracks - Follow-up to #41632
missirol added a commit to missirol/cmssw that referenced this pull request Jul 28, 2023
missirol added a commit to missirol/cmssw that referenced this pull request Jul 28, 2023
cmsbuild added a commit that referenced this pull request Jul 31, 2023
…1632

fix configuration of GPU Pixel unpacker in Run-3 HLT menus post-#41632
cmsbuild added a commit that referenced this pull request Aug 1, 2023
…1632_132X

fix configuration of GPU Pixel unpacker in Run-3 HLT menus post-#41632 [`13_2_X`]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.