Fix pixel CPE position #45471

fwyzard · 2024-07-16T12:52:57Z

PR description:

Rewrite the pixel CPE position calculation to improve the reproducibility of the results.

Fix the includes used in DataFormats/TrackingRecHitSoA to avoid the dependency on RecoLocalTracker/SiPixelRecHits.

Thanks to @missirol for spotitng the source of the discrepancy.

PR validation:

Tested on top of CMSSW 14.0.11 running the HLT over 100k events:

the CPU-only results are reproducible, with and without these changes;
the GPU vs CPU results are in general slightly more reproducible;
the biggest improvement is fixing the reproducibility of HLT_SingleEle8_v7, going from 2% of the accepted events to being fully reproducible.

GPU vs CPU results, without these changes

+4/-5 events in HLT_SingleEle8_v7 (2% of the accepted events)
+/-4 events in 1 path
+/-3 events in 2 paths
+/-2 events in 6 paths
+/-1 event in 59 paths

GPU vs CPU results, with these changes

+/-3 events in 2 paths
+/-2 events in 12 paths
+/-1 event in 46 paths

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

To be backported to 14.0.x for data taking.

cmsbuild · 2024-07-16T12:53:19Z

cms-bot internal usage

fwyzard · 2024-07-16T12:53:56Z

type bugfix

fwyzard · 2024-07-16T12:54:02Z

+heterogeneous

fwyzard · 2024-07-16T12:54:15Z

@VinInn does this looks reasonable to you ?

fwyzard · 2024-07-16T12:54:27Z

enable gpu

cmsbuild · 2024-07-16T12:55:22Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45471/40930

There are other open Pull requests which might conflict with changes you have proposed:
- File RecoLocalTracker/SiPixelRecHits/interface/pixelCPEforDevice.h modified in PR(s): Alpaka Pixel: Reading layerStart at Runtime and Variable thePitch* #45421

cmsbuild · 2024-07-16T12:55:42Z

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

It involves the following packages:

DataFormats/TrackingRecHitSoA (heterogeneous, reconstruction)
RecoLocalTracker/SiPixelRecHits (reconstruction)
RecoTracker/PixelSeeding (reconstruction)

@jfernan2, @mandrenguyen can you please review it and eventually sign? Thanks.
@GiacomoSguazzoni, @JanFSchulte, @VinInn, @VourMa, @dgulhan, @dkotlins, @felicepantaleo, @ferencek, @gpetruc, @missirol, @mmusich, @mroguljic, @mtosi, @rovere, @threus, @tsusa, @tvami this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

VinInn · 2024-07-16T12:57:15Z

RecoLocalTracker/SiPixelRecHits/interface/pixelCPEforDevice.h

@@ -234,17 +234,30 @@ namespace pixelCPEforDevice {
    if (cp.minCol[ic] == 0 || cp.maxCol[ic] == uint32_t(detParams.nCols - 1))
      cp.ysize[ic] = -cp.ysize[ic];

+    /* original code, kept for documentation
+


the new code is much less readable...

maybe add "make sure optimization does not produce different code on different architectures "
(or similar)

fwyzard · 2024-07-16T12:57:50Z

test parameters:

enable = gpu
workflows_gpu = 12634.402,12634.403,12634.404

fwyzard · 2024-07-16T12:58:20Z

please test

RecoLocalTracker/SiPixelRecHits/interface/pixelCPEforDevice.h

jfernan2 · 2024-07-16T16:39:33Z

+1

cmsbuild · 2024-07-16T16:52:34Z

Pull request #45471 was updated. @jfernan2, @mandrenguyen can you please check and sign again.

fwyzard · 2024-07-16T16:52:43Z

+heterogeneous

fwyzard · 2024-07-16T16:52:49Z

please test

cmsbuild · 2024-07-16T16:53:51Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45471/40935

There are other open Pull requests which might conflict with changes you have proposed:
- File RecoLocalTracker/SiPixelRecHits/interface/pixelCPEforDevice.h modified in PR(s): Alpaka Pixel: Reading layerStart at Runtime and Variable thePitch* #45421

fwyzard · 2024-07-16T20:59:03Z

Sorry @jfernan2, could you sign again ?
I think I only updated the comments since you signed earlier.
Thanks !

jfernan2 · 2024-07-16T22:09:05Z

+1

cmsbuild · 2024-07-16T22:09:30Z

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @mandrenguyen, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

cmsbuild · 2024-07-16T22:55:37Z

+1

Size: This PR adds an extra 20KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6e7fa/40437/summary.html
COMMIT: 785ff5d
CMSSW: CMSSW_14_1_X_2024-07-16-1100/el8_amd64_gcc12
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/45471/40437/install.sh to create a dev area with all the needed externals and cmssw changes.

DAS Queries: The DAS query tests failed, see the summary page for details.

Comparison Summary

Summary:

You potentially added 30 lines to the logs
Reco comparison results: 235 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3451616
DQMHistoTests: Total failures: 8711
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3442885
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 206 log files, 170 edm output root files, 49 DQM output files
TriggerResults: no differences found

GPU Comparison Summary

Summary:

You potentially added 1 lines to the logs
Reco comparison results: 3 differences found in the comparisons
DQMHistoTests: Total files compared: 5
DQMHistoTests: Total histograms compared: 71830
DQMHistoTests: Total failures: 3661
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 68169
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 4 files compared)
Checked 19 log files, 22 edm output root files, 5 DQM output files
TriggerResults: no differences found

rappoccio · 2024-07-17T13:05:48Z

+1

cmsbuild added this to the CMSSW_14_1_X milestone Jul 16, 2024

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending code-checks-pending heterogeneous-pending tracking trk labels Jul 16, 2024

fwyzard mentioned this pull request Jul 16, 2024

Fix pixel CPE position [14.0.x] #45472

Merged

cmsbuild added tests-started bug-fix heterogeneous-approved and removed tests-pending heterogeneous-pending labels Jul 16, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 16, 2024

VinInn reviewed Jul 16, 2024

View reviewed changes

slava77 reviewed Jul 16, 2024

View reviewed changes

RecoLocalTracker/SiPixelRecHits/interface/pixelCPEforDevice.h Outdated Show resolved Hide resolved

slava77 reviewed Jul 16, 2024

View reviewed changes

RecoLocalTracker/SiPixelRecHits/interface/pixelCPEforDevice.h Show resolved Hide resolved

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 16, 2024

fwyzard force-pushed the fix_pixel_CPE_poisition_141x branch from 02e04ac to 785ff5d Compare July 16, 2024 16:52

cmsbuild added code-checks-pending and removed code-checks-approved labels Jul 16, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 16, 2024

cmsbuild added reconstruction-approved fully-signed and removed reconstruction-pending pending-signatures labels Jul 16, 2024

cmsbuild added tests-approved and removed tests-started labels Jul 16, 2024

cmsbuild added orp-approved and removed orp-pending labels Jul 17, 2024

cmsbuild merged commit 80f86b5 into cms-sw:master Jul 17, 2024
15 checks passed

cmsbuild mentioned this pull request Jul 17, 2024

Alpaka Pixel: Reading layerStart at Runtime and Variable thePitch* #45421

Merged

fwyzard deleted the fix_pixel_CPE_poisition_141x branch July 17, 2024 14:43

cmsbuild mentioned this pull request Jul 17, 2024

Update GCC 12.4.1 cms-sw/cmsdist#9303

Open

makortel mentioned this pull request Jul 19, 2024

Added TestSourceProcessor helper #45443

Merged

fwyzard changed the title ~~Fix pixel CPE poisition~~ Fix pixel CPE position Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pixel CPE position #45471

Fix pixel CPE position #45471

fwyzard commented Jul 16, 2024

cmsbuild commented Jul 16, 2024 •

edited

Loading

fwyzard commented Jul 16, 2024

fwyzard commented Jul 16, 2024

fwyzard commented Jul 16, 2024

fwyzard commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

VinInn Jul 16, 2024 •

edited

Loading

fwyzard Jul 16, 2024

VinInn Jul 16, 2024

fwyzard commented Jul 16, 2024

fwyzard commented Jul 16, 2024

jfernan2 commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

fwyzard commented Jul 16, 2024

fwyzard commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

fwyzard commented Jul 16, 2024

jfernan2 commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

rappoccio commented Jul 17, 2024

Fix pixel CPE position #45471

Fix pixel CPE position #45471

Conversation

fwyzard commented Jul 16, 2024

PR description:

PR validation:

GPU vs CPU results, without these changes

GPU vs CPU results, with these changes

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

cmsbuild commented Jul 16, 2024 • edited Loading

fwyzard commented Jul 16, 2024

fwyzard commented Jul 16, 2024

fwyzard commented Jul 16, 2024

fwyzard commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

VinInn Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

fwyzard Jul 16, 2024

Choose a reason for hiding this comment

VinInn Jul 16, 2024

Choose a reason for hiding this comment

fwyzard commented Jul 16, 2024

fwyzard commented Jul 16, 2024

jfernan2 commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

fwyzard commented Jul 16, 2024

fwyzard commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

fwyzard commented Jul 16, 2024

jfernan2 commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

cmsbuild commented Jul 16, 2024

Comparison Summary

GPU Comparison Summary

rappoccio commented Jul 17, 2024

cmsbuild commented Jul 16, 2024 •

edited

Loading

VinInn Jul 16, 2024 •

edited

Loading