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

Code performance improvements in follow-up to #31722 #33864

Conversation

ericcano
Copy link
Contributor

PR description:

CMSSW #31722 Patatrack integration - Pixel track reconstruction (10/N) was merged with extra comments to be addressed.

This PR addresses the performance related comments.

PR validation:

The code was run in workflows (step3 and step4):

  • 11634.501 (CPU)
  • 11634.502 (GPU)
  • 11634.505 (CPU)
  • 11634.506 (GPU)

The workflow results where then compared using the plots generated by makeTrackValidationPlots.py. The CPU workflows gave results identical to the baseline integration build. The GPU workflows displayed slight variations, but the variations are also present between multiple runs of the same version (baseline or this branch).

Unit tests were fine except one problem not related the this branch and followed up in #33797

@cmsbuild cmsbuild added this to the CMSSW_12_0_X milestone May 27, 2021
@ericcano ericcano changed the title Cms patatrack cmssw 614 perf (follow-up to #31722) Code performance improvements in follow-up to #31722 May 27, 2021
@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33864/22894

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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33864/22897

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ericcano (Eric Cano) for master.

It involves the following packages:

RecoPixelVertexing/PixelTrackFitting

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @mtosi, @dgulhan this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@VinInn
Copy link
Contributor

VinInn commented May 29, 2021

most of the changes affect Rienman fit that is not default.

  1. My understanding is that Rienmann fit has NOT been tested against original patatrack version after integration.
    1a) It has to be done prior to any regression test of this PR
  2. Does a wf exist to test it?
  3. How do we know that the proposed changes are indeed a performance improvements?

@slava77
Copy link
Contributor

slava77 commented May 31, 2021

1. My understanding is that Rienmann fit has NOT been tested against original patatrack version after integration.

is it possible to add a test then?

@slava77
Copy link
Contributor

slava77 commented Jun 3, 2021

This branch has conflicts that must be resolved

Conflicting files
RecoPixelVertexing/PixelTrackFitting/interface/RiemannFit.h

please update, if this is till considered to be an active PR

@slava77
Copy link
Contributor

slava77 commented Jun 14, 2021

@ericcano
please clarify on the status of this PR.
Thank you.

@ericcano
Copy link
Contributor Author

ericcano commented Jun 15, 2021

@slava77 as the PR is pending testing RiemannFit, I opened an issue in patatrack (cms-patatrack#620) for creating a test, and will close it. We will re-introduce the changes when tests are available.

@ericcano ericcano closed this Jun 15, 2021
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.

4 participants