cmssw icon indicating copy to clipboard operation
cmssw copied to clipboard

Published 20 hours ago • verified publisher icon cms-sw

Option to make track covariance Pos-def in packedcandidate

Open vlimant opened this issue 2 years ago • 30 comments

PR description:

as reported in BPH https://indico.cern.ch/event/1155820/contributions/4853223/ the packing scheme of the track covariance in MINIAOD introduces the possibility for the covariance matrix to not be positive-definite, with implications to vertex refitting. The PR adds a new method to PackedCanidate responsible for returning a reference to a pseudoTrack with positive defined covariance matrix.

PR validation:

a simple analyser reading packed candidate was run retrieving the pseudoPosDefTrack to check that the determinant is getting positive.

vlimant avatar Oct 04 '22 11:10 vlimant

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32394

  • This PR adds an extra 28KB to repository

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

  • code-format: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32394/code-format.patch e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32394/code-format.patch | patch -p1 You can also run scram build code-format to apply code format directly

cmsbuild avatar Oct 04 '22 11:10 cmsbuild

@vlimant first of all, thanks ! I'm afraid the code needs to be forced to follow the code-format

mtosi avatar Oct 04 '22 12:10 mtosi

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32395

  • This PR adds an extra 32KB to repository

cmsbuild avatar Oct 04 '22 12:10 cmsbuild

A new Pull Request was created by @vlimant (vlimant) for master.

It involves the following packages:

  • DataFormats/PatCandidates (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks. @gpetruc, @missirol, @gouskos, @rovere, @hatakeyamak 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

cmsbuild avatar Oct 04 '22 12:10 cmsbuild

please test

mandrenguyen avatar Oct 04 '22 13:10 mandrenguyen

Hello

a quick note that was also suggested in a similar PR (#34446, which added a method to obtain a low quality cov matrix in case none was stored and initially used a parameter in pseudoTrack for that): since the track and the matrix are embedded in the PackedCandidate, if we add a parameter to pseudoTrack whoever calls it first decides whether it is posdef or not. If the pseudotrack of a candidate was previously requested without the new parameter, calling pseudoTrack with the new parameter will not give a forced positive definite matrix.

This means that if you require a positive definite matrix you must always call resetPseudoTrack. Technically that isn't even enough, because it's a race condition. Even if you reset the track, another thread can call pseudoTrack without the parameter before you have the chance to call it with the parameter, which means you can still get a non posdef matrix.

What we ended up doing in the LUT PR was adding the modification as a method and not inside the unpacking (in this case it would mean adding a makeTheCovMatrixPosDef method that modifies the matrix stored in the PackedCandidate) and then adding an EDProducer that reads the PackedCandidate collection and creates a copy with the modification applied. This does not suffer from any of the race conditions. It also does not bother anyone that did not request the posdef matrix, since it will be in a new collection.

elusian avatar Oct 04 '22 13:10 elusian

good point on race condition @elusian ; the scope of interest for posdef covariance matrix is small for the time being (BPH only?) and they should have it under control in their analysis code. Nothing changes for all other users.

vlimant avatar Oct 04 '22 15:10 vlimant

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32399

  • This PR adds an extra 28KB to repository

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

  • code-format: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32399/code-format.patch e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32399/code-format.patch | patch -p1 You can also run scram build code-format to apply code format directly

cmsbuild avatar Oct 04 '22 15:10 cmsbuild

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32402

  • This PR adds an extra 32KB to repository

cmsbuild avatar Oct 04 '22 15:10 cmsbuild

Pull request #39599 was updated. @cmsbuild, @mandrenguyen, @clacaputo can you please check and sign again.

cmsbuild avatar Oct 04 '22 15:10 cmsbuild

@vlimant It's not a matter of limited scope of interest, in fact other plugins not using the new parameter is worse, as they just need to read the collection before or at the same time as the user of this code to create problems.

In general, if something is globally cached state (as the matrix and track members are) you cannot have more than one way to compute it, or you get the race when different readers try to fill the cache with different versions of the value.

My suggestion is to move the new code from an if branch inside unpackCovariance to a new (non-const) method that takes the already unpacked covariance matrix and modifies it, which has to be called explicitly on a copy of the PackedCandidate. Since it's a copy (with a new cache slot) there is no issue with having different cached values.

If the user then needs a posdef matrix, they can copy the PackedCandidate, call the new method on the copy, and then all calls to pseudoTrack will give a track with a correct matrix.

elusian avatar Oct 04 '22 16:10 elusian

@elusian : what I meant is that BPH will have an analyser/ntupler producing what they need, and since they know about the special need they will enable the option, and no-one gets fooled. If the "fix" gets in mainstream unpacking of PackedCandidate, then we'll make it the default for all.

vlimant avatar Oct 04 '22 16:10 vlimant

type tracking

slava77 avatar Oct 06 '22 22:10 slava77

Pull request #39599 was updated. @cmsbuild, @mandrenguyen, @clacaputo, @swertz, @vlimant can you please check and sign again.

cmsbuild avatar Oct 06 '22 22:10 cmsbuild

I think that the content of packedCandidates should be unique per release during reading. The current implementation is job/implementation-dependent.

the acceptable solutions could be

  • either on-the-fly return of a copy (of as little as just a pseudoTrack or its covariance) that can be used also used a new collection with pos-forced values
  • or actually force-pos for everyone during unpacking (without options)

slava77 avatar Oct 06 '22 22:10 slava77

I think this version will be acceptable

vlimant avatar Oct 07 '22 09:10 vlimant

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32475

  • This PR adds an extra 32KB to repository

cmsbuild avatar Oct 07 '22 09:10 cmsbuild

Pull request #39599 was updated. @cmsbuild, @mandrenguyen, @clacaputo, @swertz, @vlimant can you please check and sign again.

cmsbuild avatar Oct 07 '22 09:10 cmsbuild

please test

vlimant avatar Oct 07 '22 10:10 vlimant

-1

Failed Tests: Build ClangBuild Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4f311f/28105/summary.html COMMIT: 8de7474c463f7ac97382cbb87583f9dc3321a668 CMSSW: CMSSW_12_6_X_2022-10-06-2300/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39599/28105/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation warning when building: See details on the summary page.

Clang Build

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

cmsbuild avatar Oct 07 '22 11:10 cmsbuild

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32477

  • This PR adds an extra 32KB to repository

cmsbuild avatar Oct 07 '22 12:10 cmsbuild

Pull request #39599 was updated. @cmsbuild, @mandrenguyen, @clacaputo, @swertz, @vlimant can you please check and sign again.

cmsbuild avatar Oct 07 '22 12:10 cmsbuild

please test

vlimant avatar Oct 10 '22 09:10 vlimant

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4f311f/28151/summary.html COMMIT: 192287b980b74ad5fa91464a145a8b06ea372508 CMSSW: CMSSW_12_6_X_2022-10-09-2300/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39599/28151/install.sh to create a dev area with all the needed externals and cmssw changes.

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-4f311f/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3392309
  • DQMHistoTests: Total failures: 9
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3392278
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 49 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Oct 10 '22 13:10 cmsbuild

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39599/32569

  • This PR adds an extra 32KB to repository

cmsbuild avatar Oct 14 '22 08:10 cmsbuild

@clacaputo thanks indeed. all done.

vlimant avatar Oct 14 '22 08:10 vlimant

Pull request #39599 was updated. @cmsbuild, @mandrenguyen, @clacaputo, @swertz, @vlimant can you please check and sign again.

cmsbuild avatar Oct 14 '22 08:10 cmsbuild

please test

vlimant avatar Oct 14 '22 08:10 vlimant

+1

vlimant avatar Oct 14 '22 08:10 vlimant

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4f311f/28255/summary.html COMMIT: 2e6d7efe2286971ed02d608eaba0b7e09ee8cb0e CMSSW: CMSSW_12_6_X_2022-10-13-2300/el8_amd64_gcc10 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39599/28255/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: 5 differences found in the comparisons
  • Reco comparison had 6 failed jobs
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3392309
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3392281
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
  • Checked 204 log files, 49 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

cmsbuild avatar Oct 14 '22 12:10 cmsbuild