[GSF tracking] Add full Sylvester criterion for positive definite check #39656

swagata87 · 2022-10-06T15:17:44Z

PR description:

This PR proposes to add a more strict check of positive definiteness in order to avoid GSF crashes observed recently at HLT as discussed in this issue #39570.

PR validation:

runTheMatrix.py -l 12434.0 ran fine.

In order to check if this PR would affect HLT electron efficiency, I reran HLT on EGamma RAW data, before and after this PR, and checked trigger decisions for 1000 events. HLT menu /online/collisions/2022/2e34/v1.4/HLT/V2 was run on /eos/cms/tier0/store/data/Run2022E/EGamma/RAW/v1/000/359/871/00000/fcf81596-54ed-45d3-9dee-32dec392f246.root. Trigger result was same, before and after this PR. For example:

HLT name | nEventPassed before PR | nEventPassed after PR
HLT_Ele32_WPTight_Gsf_v17 | 376 | 376
HLT_Ele23_Ele12_CaloIdL_TrackIdL_IsoVL_DZ_v21 | 30 | 30

The PR test can be done preferably by enabling profiling to see if there is any extra CPU cost for the computation of determinants of sub-matrices.

If this fix is accepted, then a backport to data-taking release cycle would be nice.

cmsbuild · 2022-10-06T15:24:38Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39656/32457

This PR adds an extra 12KB to repository

cmsbuild · 2022-10-06T15:25:04Z

A new Pull Request was created by @swagata87 (Swagata Mukherjee) for master.

It involves the following packages:

TrackingTools/GsfTracking (reconstruction)

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

slava77 · 2022-10-06T22:49:44Z

type tracking,egamma

fwyzard · 2022-10-07T11:28:39Z

TrackingTools/GsfTracking/src/GsfMultiStateUpdator.cc

    double det = 0;
+    double det22 = 0;
+    double det33 = 0;
+    double det44 = 0;
    if (updatedTSOS.isValid() && updatedTSOS.localError().valid() && updatedTSOS.localError().posDef() &&
+        (updatedTSOS.curvilinearError().matrix().Sub<AlgebraicSymMatrix22>(0, 0).Det(det22) && det22 > 0) &&
+        (updatedTSOS.curvilinearError().matrix().Sub<AlgebraicSymMatrix33>(0, 0).Det(det33) && det33 > 0) &&
+        (updatedTSOS.curvilinearError().matrix().Sub<AlgebraicSymMatrix44>(0, 0).Det(det44) && det44 > 0) &&
        (updatedTSOS.curvilinearError().matrix().Det2(det)) && det > 0) {


You can simplify this to

Suggested change

double det = 0;

double det22 = 0;

double det33 = 0;

double det44 = 0;

if (updatedTSOS.isValid() && updatedTSOS.localError().valid() && updatedTSOS.localError().posDef() &&

(updatedTSOS.curvilinearError().matrix().Sub<AlgebraicSymMatrix22>(0, 0).Det(det22) && det22 > 0) &&

(updatedTSOS.curvilinearError().matrix().Sub<AlgebraicSymMatrix33>(0, 0).Det(det33) && det33 > 0) &&

(updatedTSOS.curvilinearError().matrix().Sub<AlgebraicSymMatrix44>(0, 0).Det(det44) && det44 > 0) &&

(updatedTSOS.curvilinearError().matrix().Det2(det)) && det > 0) {

if (double det;

updatedTSOS.isValid() && updatedTSOS.localError().valid() && updatedTSOS.localError().posDef() &&

(det = 0., updatedTSOS.curvilinearError().matrix().Sub<AlgebraicSymMatrix22>(0, 0).Det(det) && det > 0) &&

(det = 0., updatedTSOS.curvilinearError().matrix().Sub<AlgebraicSymMatrix33>(0, 0).Det(det) && det > 0) &&

(det = 0., updatedTSOS.curvilinearError().matrix().Sub<AlgebraicSymMatrix44>(0, 0).Det(det) && det > 0) &&

(det = 0., updatedTSOS.curvilinearError().matrix().Det2(det) && det > 0))

{

thanks Andrea, now implemented

cmsbuild · 2022-10-07T14:41:12Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39656/32479

This PR adds an extra 12KB to repository

cmsbuild · 2022-10-07T14:41:37Z

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

mandrenguyen · 2022-10-08T07:32:28Z

please test

swagata87 · 2022-10-08T08:24:49Z

A timing measurement for HLT was done in cms-hlt-gpu.cern.ch in CMSSW_12_4_9 comparing Base vs Base+this PR.
(#jobs, #threads, #streams)=(8, 8, 0), and nEvents=10000.
The results are given below.

Base

DST_Run3_PFScoutingPixelTracking_v : 167.6 ms

Base+this PR

DST_Run3_PFScoutingPixelTracking_v : 170.4 ms

FYI: @cms-sw/hlt-l2

cmsbuild · 2022-10-08T11:31:50Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-66bf4a/28123/summary.html
COMMIT: fae4959
CMSSW: CMSSW_12_6_X_2022-10-07-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39656/28123/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-66bf4a/41834.0_TTbar_14TeV+2026D94+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: 3391103
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3391081
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

mandrenguyen · 2022-10-08T11:54:45Z

enable profiling
(apologies, missed this request in the PR description)

mandrenguyen · 2022-10-08T11:55:16Z

please test

cmsbuild · 2022-10-08T18:36:31Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-66bf4a/28125/summary.html
COMMIT: fae4959
CMSSW: CMSSW_12_6_X_2022-10-07-2300/el8_amd64_gcc10
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39656/28125/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-66bf4a/41834.0_TTbar_14TeV+2026D94+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3391103
DQMHistoTests: Total failures: 9
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3391072
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

mandrenguyen · 2022-10-08T19:17:45Z

+1
it seems there is a small increase in time to
CkfTrackCandidateMakerBase::produceBase
but that the total change of CPU is within the margin of error for the profiling.

cmsbuild · 2022-10-08T19:18:10Z

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 · 2022-10-08T20:11:43Z

+1

add full Sylvester criterion for pos-def

5d712a2

cmsbuild added this to the CMSSW_12_6_X milestone Oct 6, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Oct 6, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 6, 2022

cmsbuild added egamma tracking labels Oct 6, 2022

fwyzard reviewed Oct 7, 2022

View reviewed changes

implemented review comments

fae4959

cmsbuild added code-checks-pending and removed code-checks-approved labels Oct 7, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Oct 7, 2022

cmsbuild added tests-started and removed tests-pending labels Oct 8, 2022

cmsbuild added tests-approved and removed tests-started labels Oct 8, 2022

cmsbuild added tests-started and removed tests-approved labels Oct 8, 2022

cmsbuild added tests-approved and removed tests-started labels Oct 8, 2022

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Oct 8, 2022

cmsbuild added orp-approved and removed orp-pending labels Oct 8, 2022

cmsbuild merged commit 0ae2a78 into cms-sw:master Oct 8, 2022

This was referenced Oct 8, 2022

[124X][GSF tracking] Add full Sylvester criterion for positive definite check #39681

Merged

[125X][GSF tracking] Add full Sylvester criterion for positive definite check #39682

Merged

mmusich mentioned this pull request Oct 25, 2022

GsfTracking crash at HLT in run 359686 #39570

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GSF tracking] Add full Sylvester criterion for positive definite check #39656

[GSF tracking] Add full Sylvester criterion for positive definite check #39656

swagata87 commented Oct 6, 2022

cmsbuild commented Oct 6, 2022

cmsbuild commented Oct 6, 2022

slava77 commented Oct 6, 2022

fwyzard Oct 7, 2022

swagata87 Oct 7, 2022

cmsbuild commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

mandrenguyen commented Oct 8, 2022

swagata87 commented Oct 8, 2022

cmsbuild commented Oct 8, 2022

mandrenguyen commented Oct 8, 2022

mandrenguyen commented Oct 8, 2022

cmsbuild commented Oct 8, 2022

mandrenguyen commented Oct 8, 2022

cmsbuild commented Oct 8, 2022

perrotta commented Oct 8, 2022

[GSF tracking] Add full Sylvester criterion for positive definite check #39656

[GSF tracking] Add full Sylvester criterion for positive definite check #39656

Conversation

swagata87 commented Oct 6, 2022

PR description:

PR validation:

cmsbuild commented Oct 6, 2022

cmsbuild commented Oct 6, 2022

slava77 commented Oct 6, 2022

fwyzard Oct 7, 2022

Choose a reason for hiding this comment

swagata87 Oct 7, 2022

Choose a reason for hiding this comment

cmsbuild commented Oct 7, 2022

cmsbuild commented Oct 7, 2022

mandrenguyen commented Oct 8, 2022

swagata87 commented Oct 8, 2022

cmsbuild commented Oct 8, 2022

Comparison Summary

mandrenguyen commented Oct 8, 2022

mandrenguyen commented Oct 8, 2022

cmsbuild commented Oct 8, 2022

Comparison Summary

mandrenguyen commented Oct 8, 2022

cmsbuild commented Oct 8, 2022

perrotta commented Oct 8, 2022