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

Patatrack: introduce pt-clamping in vertex sorting, add #tracks in track SOA #37281

Merged
merged 8 commits into from Mar 28, 2022

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Mar 20, 2022

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.

@VinInn
Copy link
Contributor Author

VinInn commented Mar 20, 2022

@cmsbuild, please test

@VinInn
Copy link
Contributor Author

VinInn commented Mar 20, 2022

enable gpu

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37281/28908

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @VinInn (Vincenzo Innocente) for master.

It involves the following packages:

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

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

@VinInn
Copy link
Contributor Author

VinInn commented Mar 21, 2022

what is waiting for?

@mmusich
Copy link
Contributor

mmusich commented Mar 21, 2022

@smuzaffar tests seem stuck here. Can you verify? Thank you.

@smuzaffar
Copy link
Contributor

@mmusich , jenkins was very bus today that is why there was not enough resources to run tests, I see that https://cmssdt.cern.ch/jenkins/job/compare-root-files-short-matrix/49033/console is still running for last 2 hours. Hopefully it will finish soon.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d7e9ee/23240/summary.html
COMMIT: a375a54
CMSSW: CMSSW_12_4_X_2022-03-20-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/37281/23240/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: 19811
  • DQMHistoTests: Total failures: 2095
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 17716
  • 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

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-d7e9ee/39434.911_TTbar_14TeV+2026D88_DD4hep+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

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: 3695650
  • DQMHistoTests: Total failures: 51619
  • DQMHistoTests: Total nulls: 67
  • DQMHistoTests: Total successes: 3643942
  • 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

@jpata
Copy link
Contributor

jpata commented Mar 22, 2022

assign tracking
FYI @cms-sw/tracking-pog-l2

@mmusich
Copy link
Contributor

mmusich commented Mar 28, 2022

assign tracking-pog

assuming that was the intention of #37281 (comment)

@cmsbuild
Copy link
Contributor

New categories assigned: tracking-pog

@mmusich,@mtosi,@vmariani you have been requested to review this Pull request/Issue and eventually sign? Thanks

@mmusich
Copy link
Contributor

mmusich commented Mar 28, 2022

+1

  • @cms-sw/reconstruction-l2 what is being waited upon here?

@jpata
Copy link
Contributor

jpata commented Mar 28, 2022

Thanks Marco, I wanted to ask tracking to take a look (and make sure it was discussed in the POG) as I didn't really have any comments. I will sign now.

@jpata
Copy link
Contributor

jpata commented Mar 28, 2022

+reconstruction

auto ntracks = apc->get().m;
if (0 == first)
tracks.setNTracks(ntracks);
for (int idx = first, nt = ntracks; idx < nt; idx += gridDim.x * blockDim.x) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just checking -- why is nt needed ? wouldn't idx < ntracks equivalent ?

@fwyzard
Copy link
Contributor

fwyzard commented Mar 28, 2022

+heterogeneous

@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)

@missirol
Copy link
Contributor

Default has changed, so expect small regressions.

@VinInn @fwyzard @silviodonato , should this PR be backported to 12_3_X? (since HLT will use that release at the start of Run 3)

@silviodonato
Copy link
Contributor

I definitely support the request of a backport, if it is possible. It would be very good if we can start the data taking including this PR.

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 2620d3b into cms-sw:master Mar 28, 2022
@missirol
Copy link
Contributor

I definitely support the request of a backport, if it is possible. It would be very good if we can start the data taking including this PR.

Okay, @silviodonato.

Unless anyone objects to this (or goes ahead with the backport), I will open the backport PR to 12_3_X in the next days as per TSG's request. (cc: @Martin-Grunewald)

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