sort by pt only good-quality tracks #42428

silviodonato · 2023-07-31T13:03:06Z

PR description:

Solve #42374 (@slava77 @mmusich @Dr15Jones).
In RecoPixelVertexing/PixelTrackFitting/plugins/PixelTrackProducerFromSoA.cc tracks are sorted by pT in the attempt to improve the reproducibility of the GPU results #38065.
However, track with bad quality (< minQuality) might have pT undefined. Bad-quality tracks are discarded later on ( https://github.com/cms-sw/cmssw/blob/CMSSW_13_1_0_pre2/RecoPixelVertexing/PixelTrackFitting/plugins/PixelTrackProducerFromSoA.cc#L190 )
This PR sorts by pT only tracks with good quality.
This should avoid the risk of crash due to sorting tracks with uninitialized parameters.

As bad-quality tracks are discarded, this PR should have no effects on physics results.

PR validation:

Added a debug printout in storeTracks(iEvent, tracks, httopo); and no changes are observed, as expected.

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:

I'm not sure if a backport is necessary. Perhaps it would good to have it for 13_2_X (HIon).

cmsbuild · 2023-07-31T13:08:53Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42428/36432

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoTracker/PixelTrackFitting/plugins/PixelTrackProducerFromSoA.cc modified in PR(s): Porting Pixel Tracks to Alpaka [Not to Merge] #41117

cmsbuild · 2023-07-31T13:09:15Z

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

It involves the following packages:

RecoTracker/PixelTrackFitting (reconstruction)

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

Dr15Jones · 2023-07-31T13:10:58Z

RecoTracker/PixelTrackFitting/plugins/PixelTrackProducerFromSoA.cc

+    if (quality[i1] >= minQuality_ && quality[i2] >= minQuality_)
+      return tsoa.view()[i1].pt() > tsoa.view()[i2].pt();
+    else
+      return quality[i1] > quality[i2];


without thinking about this a lot more, I'm not 100% sure this forms a 'stable sort'. For all possible values, does this guarantee 'if A < B and B < C then A < C'?

if A and B have both good quality, they are sorted as usual

if only one of the two has good quality, that track will be sorted first

if A and B have both bad quality, they are sorted according to their quality (they will remain unsorted among them if they have the same quality)

The outcome should be unique:

quality outcome

A B C

good good good sorted by pT

good good bad AB sorted by pT, and then C

good bad good AC sorted by pT, and then B

bad good good BC sorted by pT, and then A

good bad bad A, and then BC sorted by quality

bad good bad B, and then AC sorted by quality

bad bad good C, and then AB sorted by quality

bad bad bad sorted by quality

Trying to making a more formal demonstration...

Demonstration that it is impossible having "A < B" and "B < C" and "A > C"

if all good qualities: pt(A)<pt(B) and pt(B)<pt(C) and pt(A)>pt(C) --> impossible because pt(A)<pt(B) and pt(B)<pt(C) implies pt(A)<pt(C)

if all bad qualities: quality(A)<quality(B) and quality(B)<quality(C) and quality(A)>quality(C) --> impossible, see above

if A has bad quality, A > C implies that C has bad quality, B < C implies that C has bad quality --> all bad qualities case

if B has bad quality, A < B implies that A has bad quality, A > C implies that C has bad quality --> all bad qualities case

if C has bad quality, B < C implies that B has bad quality, A < B implies that C has bad quality --> all bad qualities case

Is it difficult to scan the pT and quality in a few events with bad quality tracks just to confirm the sort behaves as expected?

I checked that the persistent tracks in storeTracks are sorted by pT, as expected, which is the behavior of the current code. I can also check the transient track collection.
Anyway, the main point was to not get accessed to pt() for tracks with bad quality, which is exactly what this PR is doing

mmusich · 2023-07-31T17:03:56Z

enable gpu

mmusich · 2023-07-31T17:08:23Z

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 · 2023-07-31T17:08:30Z

@cmsbuild, please test

cmsbuild · 2023-07-31T19:40:52Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2f1911/33991/summary.html
COMMIT: a7c4a22
CMSSW: CMSSW_13_3_X_2023-07-31-1100/el8_amd64_gcc11
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42428/33991/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3166771
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3166743
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

GPU Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 72 differences found in the comparisons
DQMHistoTests: Total files compared: 7
DQMHistoTests: Total histograms compared: 112752
DQMHistoTests: Total failures: 3026
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 109726
DQMHistoTests: Total skipped: 0
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 6 files compared)
Checked 24 log files, 30 edm output root files, 7 DQM output files
TriggerResults: no differences found

silviodonato · 2023-08-01T09:23:58Z

fyi @cms-sw/hlt-l2

mandrenguyen · 2023-08-02T08:14:18Z

+reconstruction
Sorts only high quality tracks by pT and puts the low quality one at the end.
Small changes to pixel tracks are observed in GPU workflows, consistent with expectation.
Author checked that sort gives the desired behavior, see:
#42428 (comment)

cmsbuild · 2023-08-02T08:14:42Z

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 · 2023-08-02T16:23:10Z

+1

Good quality tracks are sorted as before
Bad quality tracks are not sorted among themselves, they mantain the same order
This looks like being the desired behaviour

Port from CUDA to Alpaka the changes in cms-sw#42428 by Silvio Donato.

sort by pt only good-quality tracks

a7c4a22

cmsbuild added this to the CMSSW_13_3_X milestone Jul 31, 2023

cmsbuild added reconstruction-pending pending-signatures tests-pending orp-pending code-checks-pending tracking labels Jul 31, 2023

silviodonato mentioned this pull request Jul 31, 2023

uninit condition in PixelTrackProducerFromSoAPhase1 #42374

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 31, 2023

Dr15Jones reviewed Jul 31, 2023

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Jul 31, 2023

cmsbuild added tests-approved and removed tests-started labels Jul 31, 2023

cmsbuild added reconstruction-approved fully-signed and removed reconstruction-pending pending-signatures labels Aug 2, 2023

cmsbuild added orp-approved and removed orp-pending labels Aug 2, 2023

cmsbuild merged commit a41eee2 into cms-sw:master Aug 2, 2023

This was referenced Aug 3, 2023

[Do not merge] Test cmssw build image cms-sw/cmsdist#8626

Closed

AVXVec: replace 2*insertf128 with 1*_mm256_set_m128d #42446

Merged

fwyzard added a commit to fwyzard/cmssw that referenced this pull request Mar 17, 2024

Sort by pT only good-quality tracks

7268687

Port from CUDA to Alpaka the changes in cms-sw#42428 by Silvio Donato.

fwyzard mentioned this pull request Mar 17, 2024

Fix track sort order in PixelTrackProducerFromSoAAlpaka #44441

Merged

fwyzard added a commit to fwyzard/cmssw that referenced this pull request Mar 17, 2024

Sort by pT only good-quality tracks

1170ffc

Port from CUDA to Alpaka the changes in cms-sw#42428 by Silvio Donato.

fwyzard mentioned this pull request Mar 17, 2024

Fix track sort order in PixelTrackProducerFromSoAAlpaka [14.0.x] #44442

Merged

fwyzard added a commit to fwyzard/cmssw that referenced this pull request Mar 18, 2024

Sort by pT only good-quality tracks

b9cd422

Port from CUDA to Alpaka the changes in cms-sw#42428 by Silvio Donato.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sort by pt only good-quality tracks #42428

sort by pt only good-quality tracks #42428

silviodonato commented Jul 31, 2023

cmsbuild commented Jul 31, 2023

cmsbuild commented Jul 31, 2023

Dr15Jones Jul 31, 2023

silviodonato Jul 31, 2023

mandrenguyen Aug 2, 2023

silviodonato Aug 2, 2023

mmusich commented Jul 31, 2023

mmusich commented Jul 31, 2023

mmusich commented Jul 31, 2023

cmsbuild commented Jul 31, 2023

silviodonato commented Aug 1, 2023

mandrenguyen commented Aug 2, 2023

cmsbuild commented Aug 2, 2023

perrotta commented Aug 2, 2023

quality			outcome
A	B	C
good	good	good	sorted by pT
good	good	bad	AB sorted by pT, and then C
good	bad	good	AC sorted by pT, and then B
bad	good	good	BC sorted by pT, and then A
good	bad	bad	A, and then BC sorted by quality
bad	good	bad	B, and then AC sorted by quality
bad	bad	good	C, and then AB sorted by quality
bad	bad	bad	sorted by quality

sort by pt only good-quality tracks #42428

sort by pt only good-quality tracks #42428

Conversation

silviodonato commented Jul 31, 2023

PR description:

PR validation:

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 31, 2023

cmsbuild commented Jul 31, 2023

Dr15Jones Jul 31, 2023

Choose a reason for hiding this comment

silviodonato Jul 31, 2023

Choose a reason for hiding this comment

mandrenguyen Aug 2, 2023

Choose a reason for hiding this comment

silviodonato Aug 2, 2023

Choose a reason for hiding this comment

mmusich commented Jul 31, 2023

mmusich commented Jul 31, 2023

mmusich commented Jul 31, 2023

cmsbuild commented Jul 31, 2023

Comparison Summary

GPU Comparison Summary

silviodonato commented Aug 1, 2023

mandrenguyen commented Aug 2, 2023

cmsbuild commented Aug 2, 2023

perrotta commented Aug 2, 2023