move `pixelTopology.h` from `Geometry/TrackerGeometryBuilder` to `Geometry/CommonTopologies` #36773

mmusich · 2022-01-21T18:07:05Z

resolves #36302

PR description:

As suggested in #36302 (comment)

PR validation:

cmssw compiles

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

N/A

Note to reviewers:

I chose Geometry/CommonTopologies among the ones indicated in Placement of phase1PixelTopology.h #36302 (comment), but could be any other. Please confirm it's OK.
in the same folder Geometry/CommonTopologies there is also a PixelTopology.h file.

cmssw/Geometry/CommonTopologies/interface/PixelTopology.h

Line 1 in 131e8b4

#ifndef Geometry_CommonTopologies_PixelTopology_H

To avoid confusions, this one could be named siPixelTopology.h or simplePixelTopology.h or something like that. Suggestions are welcome.

cmsbuild · 2022-01-21T18:15:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36773/27910

This PR adds an extra 60KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoLocalTracker/SiPixelClusterizer/plugins/gpuClustering.h modified in PR(s): Use cooperative groups to populate Associations (Histograms) in Pixel Patatrack #35713
- File RecoLocalTracker/SiPixelRecHits/plugins/SiPixelRecHitSoAFromLegacy.cc modified in PR(s): Use cooperative groups to populate Associations (Histograms) in Pixel Patatrack #35713

cmsbuild · 2022-01-21T18:15:49Z

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

CUDADataFormats/Track (heterogeneous, reconstruction)
CUDADataFormats/TrackingRecHit (heterogeneous, reconstruction)
CalibTracker/SiPixelLorentzAngle (alca)
CondFormats/SiPixelObjects (db, alca)
Geometry/CommonTopologies (geometry)
Geometry/TrackerGeometryBuilder (geometry)
RecoLocalTracker/SiPixelClusterizer (reconstruction)
RecoLocalTracker/SiPixelRecHits (reconstruction)
RecoPixelVertexing/PixelTriplets (reconstruction)

@malbouis, @civanch, @yuanchao, @makortel, @cvuosalo, @fwyzard, @ianna, @mdhildreth, @cmsbuild, @ggovi, @Dr15Jones, @clacaputo, @slava77, @jpata, @tvami, @francescobrivio can you please review it and eventually sign? Thanks.
@tvami, @fabiocos, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @bsunanda, @OzAmram, @tocheng, @ferencek, @mtosi, @gpetruc, @mmusich, @dkotlins, @threus, @dgulhan, @seemasharmafnal, @venturia 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

tvami · 2022-01-21T19:36:47Z

@cmsbuild , please test with cms-sw/cmsdist#7575

cmsbuild · 2022-01-21T23:07:15Z

-1

Failed Tests: RelVals-INPUT
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e6b9f8/21909/summary.html
COMMIT: 97eff56
CMSSW: CMSSW_12_3_X_2022-01-21-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36773/21909/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:

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

RelVals-INPUT

4.764.76_ZMuSkim2012D+ZMuSkim2012D+HLTDSKIM2+RECODR1reHLT2+HARVESTDR1reHLT/step2_ZMuSkim2012D+ZMuSkim2012D+HLTDSKIM2+RECODR1reHLT2+HARVESTDR1reHLT.log

Comparison Summary

Summary:

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

mmusich · 2022-01-24T09:43:12Z

enable gpu

mmusich · 2022-01-25T13:57:19Z

please test

cmsbuild · 2022-01-25T18:08:45Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e6b9f8/21980/summary.html
COMMIT: 97eff56
CMSSW: CMSSW_12_3_X_2022-01-24-2300/slc7_amd64_gcc10
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36773/21980/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: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 4
DQMHistoTests: Total histograms compared: 19811
DQMHistoTests: Total failures: 1530
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 18281
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: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3449324
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3449302
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

clacaputo · 2022-01-26T17:15:06Z

I don't have comments on the code, but I've just a curiosity about the GPU test.
I've noticed that there are a few differences in some reco quantities for WF 11634.506 that seem to be non-reproducible. Do you know what's causing this behaviour?

cvuosalo · 2022-01-26T21:01:22Z

+1

tvami · 2022-01-26T22:10:12Z

+1

mmusich · 2022-01-27T07:57:33Z

@clacaputo

I've noticed that there are a few differences in some reco quantities for WF 11634.506 that seem to be non-reproducible. Do you know what's causing this behaviour?

I think a certain degree of non-reproducibility has been known to affect the pixel-related GPU workflows since ever. In my understanding, part of it, (especially for the vertexing related quantities) stems for the algorithm itself. On the other hand there might be other sources, which are incidentally being explored e.g. in this PR (#36390).
Having said that, said non-reproducibility AFAIK has not been a showstopper for the reco sign-off for several past GPU-related PRs and I think in any case it should be discussed elsewhere than this purely technical change which is not expected to bring any further regression. Thank you.

fwyzard · 2022-01-27T08:13:20Z

Hi @clacaputo, yes, I can confirm @mmusich's answer: some level of non-reproducibility is intrinsic in the use of parallel algorithms, due to the non-associative nature of floating point operations (a + (b + c) is not guaranteed to be the same as (a + b) + c) and the fact that parallel algorithm perform such reduction operations in a non-deterministic order.

Of course, there can be non-reproducibility issues that are due to, or amplified by, underlying bugs - both in the new GPU-based algorithms and in the existing non-GPU ones - so if you spot anything suspicious in the code, please do let us know !

clacaputo · 2022-01-27T09:15:57Z

Hi @mmusich , @fwyzard , thanks a lot for the explanation. I was just curious about the behaviour that I've seen in the GPU WF and you fulfilled my curiosity.

clacaputo · 2022-01-27T09:19:11Z

+reconstruction

technical
minor reco changes in GPU WF, not related to the PR (see move pixelTopology.h from Geometry/TrackerGeometryBuilder to Geometry/CommonTopologies #36773 (comment) )

mmusich · 2022-02-01T09:50:30Z

ping @cms-sw/heterogeneous-l2

fwyzard · 2022-02-01T09:53:02Z

thanks for the ping, and apologies for the delay, I had simply missed that it required the heterogeneous signature :-(

fwyzard · 2022-02-01T09:53:09Z

+heterogeneous

cmsbuild · 2022-02-01T09:53:26Z

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)

perrotta · 2022-02-01T10:10:49Z

+1

File move implemented as planned, and propagated where needed

cmsbuild added this to the CMSSW_12_3_X milestone Jan 21, 2022

cmsbuild added alca-pending code-checks-pending db-pending geometry-pending heterogeneous-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jan 21, 2022

mmusich mentioned this pull request Jan 21, 2022

Placement of phase1PixelTopology.h #36302

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 21, 2022

cmsbuild added requires-external tests-started and removed tests-pending labels Jan 21, 2022

cmsbuild added tests-rejected and removed tests-started labels Jan 21, 2022

cmsbuild added tests-started and removed requires-external tests-rejected labels Jan 25, 2022

cmsbuild added tests-approved and removed tests-started labels Jan 25, 2022

cmsbuild added geometry-approved and removed geometry-pending labels Jan 26, 2022

cmsbuild added alca-approved db-approved and removed alca-pending db-pending labels Jan 26, 2022

cmsbuild added reconstruction-approved and removed reconstruction-pending labels Jan 27, 2022

cmsbuild added fully-signed heterogeneous-approved and removed pending-signatures heterogeneous-pending labels Feb 1, 2022

cmsbuild added orp-approved and removed orp-pending labels Feb 1, 2022

cmsbuild merged commit 4f060eb into cms-sw:master Feb 1, 2022

cmsbuild mentioned this pull request Feb 1, 2022

fix bug in NANOAOD Pileup_nTrueInt variable (int instead of float) #36850

Merged

missirol mentioned this pull request Feb 1, 2022

Merge HLT DeepJet models for Run3 cms-data/RecoBTag-Combined#46

Merged

cmsbuild mentioned this pull request Feb 2, 2022

Clean DQMOffline/MuonDPG/BuildFile.xml and remove UBSAN comments #36855

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move `pixelTopology.h` from `Geometry/TrackerGeometryBuilder` to `Geometry/CommonTopologies` #36773

move `pixelTopology.h` from `Geometry/TrackerGeometryBuilder` to `Geometry/CommonTopologies` #36773

mmusich commented Jan 21, 2022

cmsbuild commented Jan 21, 2022

cmsbuild commented Jan 21, 2022

tvami commented Jan 21, 2022

cmsbuild commented Jan 21, 2022

mmusich commented Jan 24, 2022

mmusich commented Jan 25, 2022

cmsbuild commented Jan 25, 2022

clacaputo commented Jan 26, 2022 •

edited

cvuosalo commented Jan 26, 2022

tvami commented Jan 26, 2022

mmusich commented Jan 27, 2022

fwyzard commented Jan 27, 2022

clacaputo commented Jan 27, 2022

clacaputo commented Jan 27, 2022

mmusich commented Feb 1, 2022

fwyzard commented Feb 1, 2022 •

edited

fwyzard commented Feb 1, 2022

cmsbuild commented Feb 1, 2022

perrotta commented Feb 1, 2022

move pixelTopology.h from Geometry/TrackerGeometryBuilder to Geometry/CommonTopologies #36773

move pixelTopology.h from Geometry/TrackerGeometryBuilder to Geometry/CommonTopologies #36773

Conversation

mmusich commented Jan 21, 2022

PR description:

PR validation:

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

Note to reviewers:

cmsbuild commented Jan 21, 2022

cmsbuild commented Jan 21, 2022

tvami commented Jan 21, 2022

cmsbuild commented Jan 21, 2022

RelVals-INPUT

Comparison Summary

mmusich commented Jan 24, 2022

mmusich commented Jan 25, 2022

cmsbuild commented Jan 25, 2022

GPU Comparison Summary

Comparison Summary

clacaputo commented Jan 26, 2022 • edited

cvuosalo commented Jan 26, 2022

tvami commented Jan 26, 2022

mmusich commented Jan 27, 2022

fwyzard commented Jan 27, 2022

clacaputo commented Jan 27, 2022

clacaputo commented Jan 27, 2022

mmusich commented Feb 1, 2022

fwyzard commented Feb 1, 2022 • edited

fwyzard commented Feb 1, 2022

cmsbuild commented Feb 1, 2022

perrotta commented Feb 1, 2022

move `pixelTopology.h` from `Geometry/TrackerGeometryBuilder` to `Geometry/CommonTopologies` #36773

move `pixelTopology.h` from `Geometry/TrackerGeometryBuilder` to `Geometry/CommonTopologies` #36773

clacaputo commented Jan 26, 2022 •

edited

fwyzard commented Feb 1, 2022 •

edited