properly treat averaging with self in GhostTrackVertexFinder::vtxMean #37088

slava77 · 2022-02-28T15:28:47Z

while debugging in some corner of phase space I noticed that a relatively benign case of averaging of two identical points in GhostTrackVertexFinder::vtxMean can lead to a NaN in the results.
The NaN eventually propagates downstream and leads to
an exception thrown in PerigeeConversions::ftsToPerigeeParameters (cms::Exception("PerigeeConversions", "Track with pt=0")), which is actually caught in

cmssw/TrackingTools/TrajectoryState/src/TrajectoryStateClosestToPoint.cc

Lines 17 to 22 in 14da925

    
           } catch (const cms::Exception& ex) { 
        
             if (ex.category() != "PerigeeConversions") 
        
               throw; 
        
             edm::LogInfo("TrajectoryStateClosestToPoint_PerigeeConversions") 
        
                 << "Caught exception " << ex.explainSelf() << ".\n"; 
        
             valid = false;

. Apparently, given no checks for finiteness, a perp computed on a 3D vector of NaN values looks like a zero.

I'm proposing a simple fix to return one of the identical points.

I'm adding a warning only because this condition was previously leading to a (caught) exception.

(~~Also, I'm not familiar enough with GhostTrackVertexFinder to conclude that an averaging with self is an indication of a logic error and that a fix upstream is needed.~~)
After some tests in relvals, the conditions seems to happen in O(1e-3) events.

cmsbuild · 2022-02-28T15:35:24Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37088/28574

This PR adds an extra 20KB 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-37088/28574/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37088/28574/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2022-02-28T15:48:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37088/28576

This PR adds an extra 20KB to repository

cmsbuild · 2022-02-28T15:48:39Z

A new Pull Request was created by @slava77 (Slava Krutelyov) for master.

It involves the following packages:

RecoVertex/GhostTrackFitter (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@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

slava77 · 2022-02-28T15:49:56Z

@cmsbuild please test

cmsbuild · 2022-02-28T20:30:35Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-85247b/22718/summary.html
COMMIT: fd748b1
CMSSW: CMSSW_12_3_X_2022-02-28-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37088/22718/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: 17 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 4001207
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 4001178
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: found differences in 1 / 48 workflows

slava77 · 2022-02-28T20:46:38Z

RecoVertex/GhostTrackFitter/src/GhostTrackVertexFinder.cc

@@ -123,6 +125,11 @@ static double vtxErrorLong(const GlobalError &error, const GlobalVector &dir) {
 static GlobalPoint vtxMean(const GlobalPoint &p1, const GlobalError &e1, const GlobalPoint &p2, const GlobalError &e2) {
  GlobalVector diff = p2 - p1;

+  if UNLIKELY (diff.perp2() == 0) {


uhm, I actually meant to use mag2 here. I will update.

cmsbuild · 2022-02-28T21:11:44Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37088/28585

This PR adds an extra 20KB to repository

cmsbuild · 2022-02-28T21:12:06Z

Pull request #37088 was updated. @jpata, @cmsbuild, @clacaputo, @slava77 can you please check and sign again.

slava77 · 2022-02-28T21:12:23Z

@cmsbuild please test

cmsbuild · 2022-03-01T01:22:16Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-85247b/22725/summary.html
COMMIT: fc78082
CMSSW: CMSSW_12_3_X_2022-02-28-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37088/22725/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: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 4001207
DQMHistoTests: Total failures: 4
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 4001181
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

slava77 · 2022-03-01T02:09:58Z

Reco comparison results: 7 differences found in the comparisons

136.874 and 35034.0 have one logWarning each related to this PR.

The comparisons also show differences in 39496.0 39500.0 in simPVZ plot
but considering that the plot looks empty, I suspect that it's an unrelated change from some uninitialized value, perhaps triggered by recompilation of a related code.

slava77 · 2022-03-01T02:12:52Z

136.874 and 35034.0 have one logWarning each related to this PR.

So, this apparently means that not all cases of GhostTrackVertexFinder::vtxMean lead to a crash, since these workflows run OK in the baseline.
IMHO, the update is straightforward and the LogWarning is not really required.
@cms-sw/reconstruction-l2 please let me know if this needs to be kept or removed.

jpata · 2022-03-01T11:26:54Z

so there are no reco differences (in e.g. 136.874), despite the bugfix?

slava77 · 2022-03-01T16:42:54Z

so there are no reco differences (in e.g. 136.874), despite the bugfix?

After debugging this a bit I have updated the description of the PR: the condition was leading to an exception, but the exception was caught, which explains why there is no crash for the events with a warning.

I can confirm that without the fix (in Run: 319450 Event: 105539294; event index 43 in the workflow) this part of the code is producing an invalid point with a -nan and this result is ignored (a default invalid vertex is returned), while with the check in this PR a valid (no nan is returned).

At least in this workflow this PR affects only pfGhostTrackVertexTagInfos, which contributes to pfGhostTrackBJetTags, which is saved in RECO, but it has no monitoring either in the standard DQM or in the reco validate.C

jpata · 2022-03-02T09:29:59Z

@emilbols are pfGhostTrackVertexTagInfos actually used for physics anywhere, if they are not monitored?
cc @cms-sw/btv-pog-l2

jpata · 2022-03-04T10:20:21Z

+reconstruction

bugfix for ghost track vertices, with minimal physics effect, but prevents a rare crash
ghost track btags are not monitored, and per an email exchange, could potentially be considered as deprecated, I opened an issue here: pfGhostTrackVertexTagInfos deprecated? #37140

cmsbuild · 2022-03-04T10:20:42Z

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)

qliphy · 2022-03-04T13:01:17Z

+1

cmsbuild added this to the CMSSW_12_3_X milestone Feb 28, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Feb 28, 2022

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Feb 28, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 28, 2022

cmsbuild added tests-started and removed tests-pending labels Feb 28, 2022

cmsbuild added tests-approved and removed tests-started labels Feb 28, 2022

slava77 commented Feb 28, 2022

View reviewed changes

properly treat averaging with self

fc78082

slava77 force-pushed the from-CMSSW_12_3_0_pre1-GhostTrackVertex branch from fd748b1 to fc78082 Compare February 28, 2022 21:04

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Feb 28, 2022

cmsbuild removed the code-checks-pending label Feb 28, 2022

cmsbuild added the code-checks-approved label Feb 28, 2022

cmsbuild added tests-started and removed tests-pending labels Feb 28, 2022

cmsbuild added tests-approved and removed tests-started labels Mar 1, 2022

jpata mentioned this pull request Mar 4, 2022

pfGhostTrackVertexTagInfos deprecated? #37140

Open

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Mar 4, 2022

cmsbuild added orp-approved and removed orp-pending labels Mar 4, 2022

cmsbuild merged commit 35036ab into cms-sw:master Mar 4, 2022

cmsbuild mentioned this pull request Mar 4, 2022

Update Millepede to V04-11-01 and switch to git cms-sw/cmsdist#7660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

properly treat averaging with self in GhostTrackVertexFinder::vtxMean #37088

properly treat averaging with self in GhostTrackVertexFinder::vtxMean #37088

slava77 commented Feb 28, 2022 •

edited

Loading

cmsbuild commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

slava77 commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

slava77 Feb 28, 2022

cmsbuild commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

slava77 commented Feb 28, 2022

cmsbuild commented Mar 1, 2022

slava77 commented Mar 1, 2022

slava77 commented Mar 1, 2022

jpata commented Mar 1, 2022

slava77 commented Mar 1, 2022

jpata commented Mar 2, 2022 •

edited

Loading

jpata commented Mar 4, 2022

cmsbuild commented Mar 4, 2022

qliphy commented Mar 4, 2022

	} catch (const cms::Exception& ex) {
	if (ex.category() != "PerigeeConversions")
	throw;
	edm::LogInfo("TrajectoryStateClosestToPoint_PerigeeConversions")
	<< "Caught exception " << ex.explainSelf() << ".\n";
	valid = false;

properly treat averaging with self in GhostTrackVertexFinder::vtxMean #37088

properly treat averaging with self in GhostTrackVertexFinder::vtxMean #37088

Conversation

slava77 commented Feb 28, 2022 • edited Loading

cmsbuild commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

slava77 commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

Comparison Summary

slava77 Feb 28, 2022

Choose a reason for hiding this comment

cmsbuild commented Feb 28, 2022

cmsbuild commented Feb 28, 2022

slava77 commented Feb 28, 2022

cmsbuild commented Mar 1, 2022

Comparison Summary

slava77 commented Mar 1, 2022

slava77 commented Mar 1, 2022

jpata commented Mar 1, 2022

slava77 commented Mar 1, 2022

jpata commented Mar 2, 2022 • edited Loading

jpata commented Mar 4, 2022

cmsbuild commented Mar 4, 2022

qliphy commented Mar 4, 2022

slava77 commented Feb 28, 2022 •

edited

Loading

jpata commented Mar 2, 2022 •

edited

Loading