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

Sort pixel tracks in the SoA converter #38065

Merged
merged 1 commit into from Jun 10, 2022

Conversation

silviodonato
Copy link
Contributor

@silviodonato silviodonato commented May 24, 2022

PR description:

This PR helps to reduce the CPU-GPU differences. Testing on 10k of events of 900 GeV collisions, the hltAK4PFJets with difference in pt greater than 0.35 GeV passed from 5 to 2.

Moreover, having the pixel tracks sorted by pT helps the CPU-GPU comparison.

PR validation:

I checked on 10k of events that this PR does not change the CPU reconstruction, while it changes 3 jets reconstructed using GPU, reducing the differences from CPU.

root [2] Events->Scan("EventAuxiliary.event():EventAuxiliary.run():recoPFJets_hltAK4PFJets__CPU3.obj.pt():recoPFJets_hltAK4PFJets__GPU3.obj.pt():recoPFJets_hltAK4PFJets__CPU4.obj.pt():recoPFJets_hltAK4PFJets__GPU4.obj.pt()","abs(recoPFJets_hltAK4PFJets__CPU4.obj.pt()-recoPFJets_hltAK4PFJets__GPU4.obj.pt())>0.35 || abs(recoPFJets_hltAK4PFJets__CPU3.obj.pt()-recoPFJets_hltAK4PFJets__GPU3.obj.pt())>0.35")
***********************************************************************************************
*    Row   * Instance * EventAuxi * EventAuxi * recoPFJet * recoPFJet * recoPFJet * recoPFJet *
***********************************************************************************************
*     2787 *        1 * 249032616 *    346512 * 1.9907082 * 1.6183525 * 1.9907082 * 1.6183525 *
*     4010 *        2 * 251712216 *    346512 * 2.0095655 * 2.0095655 * 2.0095655 * 2.6015811 *
*     4010 *        3 * 251712216 *    346512 * 1.6040698 * 1.6040675 * 1.6040698 * 2.0095655 *
*     7289 *        2 * 260018739 *    346512 * 1.7360442 * 2.2783870 * 1.7360442 * 2.2783870 *
*     7962 *        1 * 261754240 *    346512 * 0.8782627 * 0.8782627 * 0.8782627 * 1.2651753 *
***********************************************************************************************

CPU3 and GPU3 are after the PR and CPU4 and GPU4 before the PR

Backport:

I will open soon the backport PRs to use this PR in the HLT at P5:
#38067 #38066

cc @cms-sw/hlt-l2 @fwyzard @VinInn

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38065/30150

  • This PR adds an extra 12KB to repository

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

@mmusich
Copy link
Contributor

mmusich commented May 24, 2022

@silviodonato, Silvio can you please apply code checks so that the integration tests could be started?

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38065/30154

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @silviodonato (Silvio Donato) for master.

It involves the following packages:

  • RecoPixelVertexing/PixelTrackFitting (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @mmusich, @mtosi, @dgulhan 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

@mmusich
Copy link
Contributor

mmusich commented May 24, 2022

enable gpu

@mmusich
Copy link
Contributor

mmusich commented May 24, 2022

test parameters:

  • enable_tests = gpu
  • workflows_gpu = 11634.503, 11634.506, 11634.583, 11634.587
  • workflow = 11634.501
  • relvals_opt= -w standard,highstats,pileup,generator,extendedgen,production,upgrade,cleanedupgrade,ged

@mmusich
Copy link
Contributor

mmusich commented May 24, 2022

please test

@mmusich
Copy link
Contributor

mmusich commented May 24, 2022

type tracking

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f3216b/24962/summary.html
COMMIT: a130c46
CMSSW: CMSSW_12_5_X_2022-05-24-1100/el8_amd64_gcc10
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38065/24962/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-f3216b/24962/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f3216b/24962/git-merge-result

GPU Comparison Summary

Summary:

  • You potentially added 20878 lines to the logs
  • Reco comparison results: 44 differences found in the comparisons
  • DQMHistoTests: Total files compared: 7
  • DQMHistoTests: Total histograms compared: 76363
  • DQMHistoTests: Total failures: 260
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 76103
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 6 files compared)
  • Checked 24 log files, 18 edm output root files, 7 DQM output files
  • TriggerResults: found differences in 6 / 6 workflows

Comparison Summary

Summary:

  • You potentially added 365465 lines to the logs
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3666987
  • DQMHistoTests: Total failures: 1216
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3665749
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 212 log files, 48 edm output root files, 51 DQM output files
  • TriggerResults: found differences in 3 / 50 workflows

@mmusich
Copy link
Contributor

mmusich commented May 25, 2022

@silviodonato

GPU: You potentially added 20878 lines to the logs
CPU: You potentially added 365465 lines to the logs

as you can see from the logs, this PR adds a flood of:

%MSG-w PixelVertexProducer:  PixelVertexProducerFromSoA:hltPixelVertices  24-May-2022 22:40:09 CEST Run: 1 Event: 1
oops track 179 does not exists on CPU 65535
%MSG
%MSG-w PixelVertexProducer:  PixelVertexProducerFromSoA:hltPixelVertices  24-May-2022 22:40:09 CEST Run: 1 Event: 1
oops track 180 does not exists on CPU 65535
%MSG
%MSG-w PixelVertexProducer:  PixelVertexProducerFromSoA:hltPixelVertices  24-May-2022 22:40:09 CEST Run: 1 Event: 1
oops track 181 does not exists on CPU 65535
%MSG
%MSG-w PixelVertexProducer:  PixelVertexProducerFromSoA:hltPixelVertices  24-May-2022 22:40:09 CEST Run: 1 Event: 1
oops track 182 does not exists on CPU 65535
%MSG
%MSG-w PixelVertexProducer:  PixelVertexProducerFromSoA:hltPixelVertices  24-May-2022 22:40:09 CEST Run: 1 Event: 1
oops track 183 does not exists on CPU 65535
%MSG
%MSG-w PixelVertexProducer:  PixelVertexProducerFromSoA:hltPixelVertices  24-May-2022 22:40:09 CEST Run: 1 Event: 1
oops track 184 does not exists on CPU 65535
%MSG

both on CPU and GPU workflows:

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f3216b/25107/summary.html
COMMIT: 62af9d4
CMSSW: CMSSW_12_5_X_2022-05-31-1100/el8_amd64_gcc10
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38065/25107/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 7
  • DQMHistoTests: Total histograms compared: 76363
  • DQMHistoTests: Total failures: 58
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 76305
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 6 files compared)
  • Checked 24 log files, 18 edm output root files, 7 DQM output files
  • TriggerResults: found differences in 6 / 6 workflows

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3664317
  • DQMHistoTests: Total failures: 1110
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3663184
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 50 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 212 log files, 48 edm output root files, 51 DQM output files
  • TriggerResults: found differences in 3 / 50 workflows

@missirol
Copy link
Contributor

missirol commented Jun 2, 2022

I'm trying to understand the differences in the HLT outputs from the PR tests (e.g. wfs 11634.*).
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_5_X_2022-05-31-1100+f3216b/50618/dqm-histo-comparison-summary.html

  • There are several (generally small) differences in both CPU and GPU wfs, and I understand this is to be expected. The differences are mainly coming from HLT (full) tracks and HLT PFlow-related quantities.

  • The wf which customises the HLT step to use MkFit at HLT (i.e. 11634.7) shows less differences compared to its standard counterpart (11634.0). Correct? Expected?

  • The DD4hep wf (11634.911) shows more differences wrt 11634.0, while the DDD wf (11634.914) shows no differences at all. I don't know what could explain this. Am I missing something?

Feedback from tracking experts is likely needed (and appreciated).

@mmusich
Copy link
Contributor

mmusich commented Jun 2, 2022

@missirol

There are several (generally small) differences in both CPU and GPU wfs, and I understand this is to be expected. The differences are mainly coming from HLT (full) tracks and HLT PFlow-related quantities.

I agree this is expected.

The wf which customises the HLT step to use MkFit at HLT (i.e. 11634.7) shows less differences compared to its standard counterpart (11634.0). Correct? Expected?

the mkFit customization uses the same seeds as the default HLT workflow customizeHLTIter0ToMkFit.py#L44, so the patatrack pixel tracks, but mkFit also performs an internal sorting and cleaning of the seeds, so probably part of what is implemented here is overridden anyway donwstream (@mmasciov)

The DD4hep wf (11634.911) shows more differences wrt 11634.0, while the DDD wf (11634.914) shows no differences at all. I don't know what could explain this. Am I missing something?

As far as I understand the standard wf 11634.0 IS using DD4hep (but reading the simulation / reconstruction geometry from DataBase), while 11634.911 is theoretically using the same geometry, but fetching if from XML. Looking in the past few month of cmssw integration history the 11634.911 workflow is not very reproducible (see e.g.: #35109), so I am not entirely surprised about that. About why the DDD doesn't show any change, I have no clue (tagging also @cms-sw/geometry-l2 )

@missirol
Copy link
Contributor

missirol commented Jun 2, 2022

Thanks for the info, @mmusich .

@cvuosalo
Copy link
Contributor

cvuosalo commented Jun 2, 2022

@missirol @mmusich Wf 11634.911 is DD4hep using geometry from XML files. 11634.0 is DD4hep with DB geometry. 11634.914 is DDD with DB geometry. The other 11634 wfs are DD4hep with DB geometry.
I cannot explain the pattern of differences between these workflows due to the change in sort order of the pixel tracks. There are tiny numerical differences (~1e-10) among the Tracker reco geometries from DD4hep XML, DD4hep DB, and DDD DB. I am not sure how they would interact with the sorting of pixel tracks.

@jpata
Copy link
Contributor

jpata commented Jun 8, 2022

Folks, how do you want to take this forward, and with what timescale?

As far as I understand, minor differences in outputs can be expected given a different sorting, but the exact pattern of differences with respect to different geometry implementations is not understood. From the reco side, this is not a blocker.

@missirol
Copy link
Contributor

missirol commented Jun 8, 2022

+hlt

@jpata
Copy link
Contributor

jpata commented Jun 9, 2022

+reconstruction

  • no relevant differences in reco (some in HLT)
  • addresses CPU-GPU differences at HLT

@qliphy
Copy link
Contributor

qliphy commented Jun 10, 2022

@cms-sw/heterogeneous-l2 Do you have any further comment? Or can you sign this PR?

@fwyzard
Copy link
Contributor

fwyzard commented Jun 10, 2022

+1

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

@qliphy
Copy link
Contributor

qliphy commented Jun 10, 2022

+1

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.

None yet

9 participants