Option to make track covariance Pos-def in packedcandidate #39599

vlimant · 2022-10-04T11:50:47Z

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.

cmsbuild · 2022-10-04T11:57:47Z

-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

mtosi · 2022-10-04T12:32:02Z

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

cmsbuild · 2022-10-04T12:56:13Z

+code-checks

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

This PR adds an extra 32KB to repository

cmsbuild · 2022-10-04T12:56:38Z

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

mandrenguyen · 2022-10-04T13:08:29Z

please test

elusian · 2022-10-04T13:27:41Z

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.

slava77 · 2022-10-04T13:35:18Z

DataFormats/PatCandidates/interface/PackedCandidate.h

@@ -1019,7 +1023,7 @@ namespace pat {
    void packVtx(bool unpackAfterwards = true);
    void unpackVtx() const;
    void packCovariance(const reco::TrackBase::CovarianceMatrix &m, bool unpackAfterwards = true);
-    void unpackCovariance() const;
+    void unpackCovariance(bool forcePosDef = true) const;


why do you need a default for protected interface? All calls should be under control.
Also, IIUC, for many users the old (not forced positive) is OK

slava77 · 2022-10-04T13:48:10Z

DataFormats/PatCandidates/src/PackedCandidate.cc

+      double det_(0);
+      bool computed_ = m->Det(det_);
+      //      std::cout<<"during unpacking, the determinant is: "<<det_<<std::endl;
+      if (computed_ && det_ < 0) {


in mkFit context we apply a full Sylvester's criterion; computing determinants for sub-matrices. The extra cost is not that much compared to computing the det for the full matrix

cmssw/RecoTracker/MkFit/plugins/MkFitOutputConverter.cc

Lines 321 to 338 in 17333a9

//Sylvester's criterion, start from the smaller submatrix size

double det = 0;

if ((!fts.curvilinearError().matrix().Sub<AlgebraicSymMatrix22>(0, 0).Det(det)) || det < 0) {

edm::LogInfo("MkFitOutputConverter")

<< "Fail pos-def check sub2.det for candidate " << candIndex << " with fts " << fts;

continue;

} else if ((!fts.curvilinearError().matrix().Sub<AlgebraicSymMatrix33>(0, 0).Det(det)) || det < 0) {

edm::LogInfo("MkFitOutputConverter")

<< "Fail pos-def check sub3.det for candidate " << candIndex << " with fts " << fts;

continue;

} else if ((!fts.curvilinearError().matrix().Sub<AlgebraicSymMatrix44>(0, 0).Det(det)) || det < 0) {

edm::LogInfo("MkFitOutputConverter")

<< "Fail pos-def check sub4.det for candidate " << candIndex << " with fts " << fts;

continue;

} else if ((!fts.curvilinearError().matrix().Det2(det)) || det < 0) {

edm::LogInfo("MkFitOutputConverter")

<< "Fail pos-def check det for candidate " << candIndex << " with fts " << fts;

continue;

thanks for the pointer indeed.

slava77 · 2022-10-04T13:49:26Z

DataFormats/PatCandidates/src/PackedCandidate.cc

+      //      std::cout<<"unpacking enforcing positive-definite cov matrix"<<std::endl;
+      //calculate the determinant and verify positivity
+      double det_(0);
+      bool computed_ = m->Det(det_);


please follow CMSSW style (lower case camelCaps, no trailing underscore in locals); the comment is for the full file

thanks for the reminder. "camelCaps" rule for local could be added to http://cms-sw.github.io/cms_coding_rules.html (unless I could not find it)

thanks for the reminder. "camelCaps" rule for local could be added to http://cms-sw.github.io/cms_coding_rules.html (unless I could not find it)

it's kind of there in rule 2.8, but the locals are in rule 2.9. So, the wording is not perfect.

vlimant · 2022-10-04T15:12:51Z

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.

cmsbuild · 2022-10-04T15:15:17Z

-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 · 2022-10-14T12:32:59Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4f311f/28255/summary.html
COMMIT: 2e6d7ef
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

vlimant · 2022-10-17T11:44:05Z

all good @clacaputo ?

clacaputo · 2022-10-18T08:42:36Z

+reconstruction

new method pseudoPosDefTrack add to PackedCanidate
no reco changes

cmsbuild · 2022-10-18T08:42:59Z

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)

rappoccio · 2022-10-18T15:58:33Z

+1

slava77 · 2023-01-15T16:44:46Z

It would be nice to backport this to 10_6_X.

@vlimant would you be available to do this?
Thanks.

vlimant · 2023-01-16T09:07:54Z

10.6 and everything in between ?

mmusich · 2023-01-16T09:39:42Z

10.6 and everything in between ?

10.6 would be needed because it's the main release for UL analysis. I think this feature is innocent enough to be added to all of that is in- between. What do @cms-sw/orp-l2 think?

slava77 · 2023-01-16T13:14:26Z

I guess 12_4 for Run3 2022 analyses. I'm not sure also about 12_6: was there a plan to run re-miniAOD here?

perrotta · 2023-01-16T13:14:38Z

10.6 and everything in between ?

10.6 would be needed because it's the main release for UL analysis. I think this feature is innocent enough to be added to all of that is in- between. What do @cms-sw/orp-l2 think?

Maybe it will not be really used in all them, but I'd add it to the release that can be still used in analyses: besides 10_6 for UL, 12_4 for 2022 pp, 12_5 for 2022 HI. 12_6 for re-nano and MC studies.

mmusich · 2023-01-18T09:54:59Z

Maybe it will not be really used in all them, but I'd add it to the release that can be still used in analyses: besides 10_6 for UL, 12_4 for 2022 pp, 12_5 for 2022 HI. 12_6 for re-nano and MC studies.

10_6 for UL -> covered in ~~track covariance Pos-def in packedcandidate #40526~~ [10.6.X] Option to make track covariance Pos-def in packedcandidate #40557
12_4 for 2022 pp -> done in [12.4.X] Option to make track covariance Pos-def in packedcandidate #40556
12_5 for 2022 HI -> done in [12.5.X] Option to make track covariance Pos-def in packedcandidate #40555
12_6 for re-nano and MC studies -> this is covered in this PR

cmsbuild added this to the CMSSW_12_6_X milestone Oct 4, 2022

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

cmsbuild added code-checks-rejected and removed code-checks-pending labels Oct 4, 2022

cmsbuild added code-checks-pending and removed code-checks-rejected labels Oct 4, 2022

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

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

slava77 reviewed Oct 4, 2022

View reviewed changes

cmsbuild added code-checks-pending tests-pending and removed tests-started code-checks-approved labels Oct 4, 2022

cmsbuild removed the code-checks-pending label Oct 4, 2022

cmsbuild added tests-started xpog-approved and removed tests-pending xpog-pending labels Oct 14, 2022

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

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

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

cmsbuild merged commit 5a33369 into cms-sw:master Oct 18, 2022

cmsbuild mentioned this pull request Oct 19, 2022

Add new HI Beamspot to the Run-3 HI MC + change vertex smearing to use the estimated HI version #39758

Merged

vlimant deleted the posDef_trackcov_unpacking branch November 22, 2022 10:18

vlimant mentioned this pull request Jan 16, 2023

track covariance Pos-def in packedcandidate #40526

Closed

This was referenced Jan 18, 2023

[12.5.X] Option to make track covariance Pos-def in packedcandidate #40555

Merged

[12.4.X] Option to make track covariance Pos-def in packedcandidate #40556

Merged

mmusich mentioned this pull request Jan 18, 2023

[10.6.X] Option to make track covariance Pos-def in packedcandidate #40557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to make track covariance Pos-def in packedcandidate #39599

Option to make track covariance Pos-def in packedcandidate #39599

vlimant commented Oct 4, 2022 •

edited

Loading

cmsbuild commented Oct 4, 2022

mtosi commented Oct 4, 2022

cmsbuild commented Oct 4, 2022

cmsbuild commented Oct 4, 2022

mandrenguyen commented Oct 4, 2022

elusian commented Oct 4, 2022

slava77 Oct 4, 2022

slava77 Oct 4, 2022

vlimant Oct 4, 2022

slava77 Oct 4, 2022

vlimant Oct 4, 2022

slava77 Oct 4, 2022

vlimant commented Oct 4, 2022

cmsbuild commented Oct 4, 2022

cmsbuild commented Oct 14, 2022

vlimant commented Oct 17, 2022 •

edited

Loading

clacaputo commented Oct 18, 2022

cmsbuild commented Oct 18, 2022

rappoccio commented Oct 18, 2022

slava77 commented Jan 15, 2023

vlimant commented Jan 16, 2023

mmusich commented Jan 16, 2023

slava77 commented Jan 16, 2023

perrotta commented Jan 16, 2023

mmusich commented Jan 18, 2023 •

edited

Loading

	//Sylvester's criterion, start from the smaller submatrix size
	double det = 0;
	if ((!fts.curvilinearError().matrix().Sub<AlgebraicSymMatrix22>(0, 0).Det(det)) \|\| det < 0) {
	edm::LogInfo("MkFitOutputConverter")
	<< "Fail pos-def check sub2.det for candidate " << candIndex << " with fts " << fts;
	continue;
	} else if ((!fts.curvilinearError().matrix().Sub<AlgebraicSymMatrix33>(0, 0).Det(det)) \|\| det < 0) {
	edm::LogInfo("MkFitOutputConverter")
	<< "Fail pos-def check sub3.det for candidate " << candIndex << " with fts " << fts;
	continue;
	} else if ((!fts.curvilinearError().matrix().Sub<AlgebraicSymMatrix44>(0, 0).Det(det)) \|\| det < 0) {
	edm::LogInfo("MkFitOutputConverter")
	<< "Fail pos-def check sub4.det for candidate " << candIndex << " with fts " << fts;
	continue;
	} else if ((!fts.curvilinearError().matrix().Det2(det)) \|\| det < 0) {
	edm::LogInfo("MkFitOutputConverter")
	<< "Fail pos-def check det for candidate " << candIndex << " with fts " << fts;
	continue;

Option to make track covariance Pos-def in packedcandidate #39599

Option to make track covariance Pos-def in packedcandidate #39599

Conversation

vlimant commented Oct 4, 2022 • edited Loading

PR description:

PR validation:

cmsbuild commented Oct 4, 2022

mtosi commented Oct 4, 2022

cmsbuild commented Oct 4, 2022

cmsbuild commented Oct 4, 2022

mandrenguyen commented Oct 4, 2022

elusian commented Oct 4, 2022

slava77 Oct 4, 2022

Choose a reason for hiding this comment

slava77 Oct 4, 2022

Choose a reason for hiding this comment

vlimant Oct 4, 2022

Choose a reason for hiding this comment

slava77 Oct 4, 2022

Choose a reason for hiding this comment

vlimant Oct 4, 2022

Choose a reason for hiding this comment

slava77 Oct 4, 2022

Choose a reason for hiding this comment

vlimant commented Oct 4, 2022

cmsbuild commented Oct 4, 2022

cmsbuild commented Oct 14, 2022

Comparison Summary

vlimant commented Oct 17, 2022 • edited Loading

clacaputo commented Oct 18, 2022

cmsbuild commented Oct 18, 2022

rappoccio commented Oct 18, 2022

slava77 commented Jan 15, 2023

vlimant commented Jan 16, 2023

mmusich commented Jan 16, 2023

slava77 commented Jan 16, 2023

perrotta commented Jan 16, 2023

mmusich commented Jan 18, 2023 • edited Loading

vlimant commented Oct 4, 2022 •

edited

Loading

vlimant commented Oct 17, 2022 •

edited

Loading

mmusich commented Jan 18, 2023 •

edited

Loading