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

Backport to 12_3_X of Patatrack pixel-reco updates (pT-clamping in vertex sorting + stabilized Fishbone) #37444

Merged
merged 14 commits into from Apr 5, 2022

Conversation

missirol
Copy link
Contributor

@missirol missirol commented Apr 2, 2022

backport of #37281
backport of #37398

PR description:

I open the backport of #37281 and #37398 to 12_3_X as per TSG's request (see #37281 (comment); cc: @silviodonato @Martin-Grunewald).

Those PRs change the outputs of the HLT pixel reconstruction, and TSG would like to have these updates in 12_3_X (release cycle to be used at HLT at the start of Run 3).

Below, a copy of the description of the original PRs by @VinInn.

Description of #37281 ("Patatrack: introduce pt-clamping in vertex sorting, add #tracks in track SOA"):

This PR introduces some long standing improvements in Patatrack that can be useful to POG and PAG using directly SOA products.

  1. Track Pt-clamping is introduced in vertex sorting.
    This is supposed to mitigate the effect of spurious tracks with large pt due to miss-measurement, resolution or inaccurate calibration/alignment.

The effect has been tested in Z->tau-tau events that are supposed to be among those who most suffer from signal-vertex miss-identification.
The effect is small if not negligible. see: http://innocent.home.cern.ch/innocent/RelVal/ztt_clamp/plots_selectedPixelVertices/pvtagging.pdf
where we compare clamping at 20, 75 (the default) and 1000 GeV.
This is not unexpected given the resolution of HP quadruplets, those used to build vertices, that reaches ~30% at ~70 GeV.
It may be different with initial calibrations or large inefficiency.

Results for ttbar can be found in
http://innocent.home.cern.ch/innocent/RelVal/gpuMTVclamp/plots_selectedPixelVertices/pvtagging.pdf

  1. The number of tracks is stored in the track SOA.
    This should make the use of the SOA more user-friendly.

Default has changed, so expect small regressions.

Excerpt of the description of #37398 ("Stabilize Fishbone"):

The fishbone has been made deterministic and order independent.
The main change is (of course) removing the break in the combinatorial double loop.

results:
Timing: negligible effect.

MTV: slight increase of duplicate (as more fishbone cells are created) for Loose tracks. no effect on HP
http://innocent.home.cern.ch/innocent/RelVal/gpuMTVstableFB/

detailed comparison of reproducibility
[..]
In particular with this PR (no break) comparing 10 events at track level
NO difference in HP quadruplets, only one HP triplet differs.
the rest are loose triplets (and a couple of loose quadruplets)
REMINDER:
loose tracks are in the collection ONLY to be used for seeding or for algorithms that perform some sort of pre-cleaning
(often a simple association to trimmed-vertices is enough).

Making the various ambiguity solvers deterministic and order independent will be much harder and costly

PR validation:

Relies on the validation done for the original PRs.

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

#37281
#37398

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2022

A new Pull Request was created by @missirol (Marino Missiroli) for CMSSW_12_3_X.

It involves the following packages:

  • CUDADataFormats/Track (heterogeneous, reconstruction)
  • RecoPixelVertexing/PixelTrackFitting (reconstruction)
  • RecoPixelVertexing/PixelTriplets (reconstruction)
  • RecoPixelVertexing/PixelVertexFinding (reconstruction)

@makortel, @slava77, @clacaputo, @cmsbuild, @fwyzard, @jpata 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

@VinInn
Copy link
Contributor

VinInn commented Apr 2, 2022

I though #37398 was more urgent (and could have been backported together)

@missirol
Copy link
Contributor Author

missirol commented Apr 2, 2022

Indeed, I was waiting for #37398 to be signed, before asking if TSG wanted a backport of it (most likely, yes). I can then append that backport here, if it helps.

@fwyzard
Copy link
Contributor

fwyzard commented Apr 2, 2022

+heterogeneous

@fwyzard
Copy link
Contributor

fwyzard commented Apr 2, 2022

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-df3cc3/23621/summary.html
COMMIT: 6a039d8
CMSSW: CMSSW_12_3_X_2022-04-02-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37444/23621/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3697381
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3697357
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@clacaputo
Copy link
Contributor

test parameters:

  • enable_test = gpu

@clacaputo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2022

Pull request #37444 was updated. @makortel, @slava77, @clacaputo, @cmsbuild, @fwyzard, @jpata can you please check and sign again.

@missirol missirol changed the title Backport to 12_3_X of "Patatrack: introduce pt-clamping in vertex sorting, add #tracks in track SOA" Backport to 12_3_X of Patatrack pixel-reco updates (pT-clamping in vertex sorting + stabilized Fishbone) Apr 2, 2022
@missirol
Copy link
Contributor Author

missirol commented Apr 2, 2022

urgent

Repurposed this PR to a backport of both #37281 and #37398, as I assume TSG also wants the latter backported in time for 12_3_0 (@silviodonato and @fwyzard will correct me if that's not the case).

("urgent" here means "in time for 12_3_0")

@cmsbuild cmsbuild added the urgent label Apr 2, 2022
@missirol
Copy link
Contributor Author

missirol commented Apr 2, 2022

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 3, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-df3cc3/23623/summary.html
COMMIT: 3c15c2f
CMSSW: CMSSW_12_3_X_2022-04-02-1100/slc7_amd64_gcc10
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37444/23623/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: 24 differences found in the comparisons
  • DQMHistoTests: Total files compared: 4
  • DQMHistoTests: Total histograms compared: 19874
  • DQMHistoTests: Total failures: 3601
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 16273
  • 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: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3697381
  • DQMHistoTests: Total failures: 3841
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3693518
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 45 edm output root files, 49 DQM output files
  • TriggerResults: found differences in 1 / 48 workflows

@clacaputo
Copy link
Contributor

+reconstruction

@missirol
Copy link
Contributor Author

missirol commented Apr 5, 2022

Kind ping for the signature of @cms-sw/heterogeneous-l2 (this PR had already been signed, but then I added the backport of #37398).

@fwyzard
Copy link
Contributor

fwyzard commented Apr 5, 2022

+heterogeneous

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2022

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_3_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_4_X is complete. 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 Apr 5, 2022

+1

@cmsbuild cmsbuild merged commit 1ed9513 into cms-sw:CMSSW_12_3_X Apr 5, 2022
@missirol missirol deleted the backport_37281_to_123X branch July 27, 2023 09:13
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

6 participants