Use parabolic magnetic field in track final fit and modify min N(hits) condition in track duplicate merge #39578

mmasciov · 2022-10-03T12:38:21Z

PR description:

This PR:
(1) introduces the usage of parabolic magnetic field with corresponding propagator in track final fit, for main tracking iterations;
(2) modifies the condition on min N(hits) in KFFittingSmoother used in track duplicate merge.

(1) allows to reduce track fitting time by ~10%, with no visible change in tracking physics performance.
(2) allows to reduce duplicate rate at low pT and high pseudorapidity.

PR validation:

Please, refer to presentation at Tracking POG on September 12:
https://indico.cern.ch/event/1197539/#2-tracking-final-fit-opportuni
For (1): slides 5-8.
For (2): slides 26-27.

FYI: @mmusich @slava77

…icate merge

cmsbuild · 2022-10-03T12:47:44Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39578/32365

This PR adds an extra 20KB to repository

cmsbuild · 2022-10-03T12:48:09Z

A new Pull Request was created by @mmasciov (Mario Masciovecchio) for master.

It involves the following packages:

RecoTracker/TrackProducer (reconstruction)
TrackingTools/TrackFitters (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@VourMa, @felicepantaleo, @abbiendi, @CeliaFernandez, @mmusich, @cericeci, @andrea21z, @JanFSchulte, @jhgoh, @missirol, @HuguesBrun, @trocino, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @ebrondol, @mtosi, @dgulhan, @Fedespring, @lecriste, @gpetruc 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-03T13:05:37Z

please test

mmusich · 2022-10-03T13:07:08Z

would it be worth to enable the profiling for the tests?

slava77 · 2022-10-03T14:03:05Z

would it be worth to enable the profiling for the tests?

I agree

cmsbuild · 2022-10-03T19:58:45Z

+1

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

Summary:

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

slava77 · 2022-10-03T22:36:22Z

enable profiling

slava77 · 2022-10-03T22:36:35Z

@cmsbuild please test

slava77 · 2022-10-03T22:54:14Z

Looking at comparisons, I wasn't really expecting changes in refittedStandAloneMuons .
It's not obvious which way it gets pulled in

Somewhat related, I see in RecoMuon/Configuration/python/DisplacedMuonSeededStep_cff.py:muonSeededTracksOutInDisplaced = RecoTracker.TrackProducer.TrackProducer_cfi.TrackProducer.clone(
I wouldn't be sure that the parabolic field will work there.

@mmasciov
I was expecting that the changes are localized to (iterative) tracking.
Some more careful update is likely needed.

mandrenguyen · 2022-10-04T15:28:12Z

@mmasciov @slava77 Conversion step tracks are affected, which is not explicit in the code changes. Just checking, is that intentional / acceptable?

clacaputo · 2022-10-04T15:29:31Z

From igprof CPU usage comparison

11834.21
>> Total Cumulative(old->new): 331.84 -> 324.85
136.889
>> Total Cumulative(old->new): 959.74 -> 955.98
39634.21
>> Total Cumulative(old->new): 1164.03 -> 1177.86

12_6_0_pre2 for 11834.21

This PR for 11834.21

slava77 · 2022-10-04T15:44:49Z

@mmasciov @slava77 Conversion step tracks are affected, which is not explicit in the code changes. Just checking, is that intentional / acceptable?

conversionStepTracks are essentially a copy of convStepTracks (after extra quality sels), they come from convTrackCandidates built from photonConvTrajSeedFromSingleLeg , which uses generalTracks in the inputs

slava77 · 2022-10-04T16:01:31Z

39634.21
>> Total Cumulative(old->new): 1164.03 -> 1177.86

this is the only case going in the unexpected direction.
Looking at all_OldVSNew_TTbar14TeV2026D88PUwPRMXwf39634p999 (10 events, lower PU, just 50), there is nothing significant.

The TrackProducer time reduces as expected in this PR in this pase-2 wf.

rappoccio · 2022-10-04T16:07:44Z

@mandrenguyen let us know ASAP if you're okay with this going into pre3, as discussed in ORP today.

mandrenguyen · 2022-10-04T16:09:26Z

urgent
@rappoccio I'm just taking some more minutes to sort thru the comparisons, but I think I should be able to sign off soon

slava77 · 2022-10-04T16:11:19Z

39634.21
>> Total Cumulative(old->new): 1164.03 -> 1177.86

this is the only case going in the unexpected direction. Looking at all_OldVSNew_TTbar14TeV2026D88PUwPRMXwf39634p999 (10 events, lower PU, just 50), there is nothing significant.

The TrackProducer time reduces as expected in this PR in this pase-2 wf.

[2    -> 2   ] [5.5    -> 5.6   ]  [96.08     -> 99.22    ]                  PrimaryVertexProducer::produce                    
[9    -> 7   ] [2.1    -> 2.2   ]  [36.88     -> 38.43    ]                  PFDisplacedVertexProducer::produce                
[11   -> 11  ] [1.7    -> 1.7   ]  [28.87     -> 30.26    ]                  TemplatedVertexArbitrator<edm::View<reco::Candidate...
[12   -> 10  ] [1.5    -> 1.8   ]  [27.05     -> 32.17    ]                 TrackstersProducer::produce     
[13   -> 12  ] [1.4    -> 1.5   ]  [24.57     -> 26.79    ]                  PFClusterProducer::produce

PV is stochastic, perhaps a few tracks changing triggered a slowly converging case.
If there are per-event details ( timeMemoryInfo), it would be possible to check if it's a single event. I don't see timememoryinfo.log for step3/4 in https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e7e9f8/27963/profiling/39634.21/ (why?)

The TICL-related part seems to be similar in size of increase as the vertex; not obvious if PV feeds the TICL change though.

mandrenguyen · 2022-10-04T16:22:06Z

+reconstruction
Expected improvement to CPU timing of TrackProducer is observed in profiling.
Changes to track distribution and downstream objects like PF candidates are at noise level.
The slight increase of timing in the Phase 2 workflow is a bit concerning, but could by just related to fluctuations in the vertexing. This increase is anyway rather small compared to recent changes to the Phase 2 reco.

cmsbuild · 2022-10-04T16:22:29Z

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-04T16:23:54Z

+1

perrotta · 2022-10-04T16:41:51Z

RecoTracker/TrackProducer/python/TrackProducerIterativeDefault_cfi.py

+    useSimpleMF = cms.bool(True),
+    SimpleMagneticField = cms.string("ParabolicMf"),
+    Propagator = cms.string('PropagatorWithMaterialParabolicMf'),


Use safer syntax (for a possible update)

slava77 · 2022-10-04T16:43:41Z

TrackingTools/TrackFitters/plugins/KFFittingSmootherESProducer.cc

@@ -65,7 +65,7 @@ namespace {
      desc.add<int>("MaxNumberOfOutliers", 3);
      desc.add<int>("MinDof", 2);
      desc.add<bool>("NoOutliersBeginEnd", false);
-      desc.add<int>("MinNumberOfHits", 5);
+      desc.add<int>("MinNumberOfHits", 3);


it looks like this is cloned all over the place, like GsfElectronFittingSmoother or KFFittingSmootheForSTA, which all could trigger an increase in fakes (unless the number of hits 5->3 is shadowed by a tighter cut somewhere else).

It would be better to change this only in the iter tracking context.

So, a bugfix is quite likely needed now.

So, a bugfix is quite likely needed now.

Yes!
Please @slava77 @mmasciov provide that fix
Please @rappoccio wait for that fix before proceeding with building the pre-release!

@slava77, @perrotta, @rappoccio: PR #39611 now only applies this change to RKFittingSmoother, without affecting GsfElectronFittingSmoother or KFFittingSmootheForSTA.

mmusich · 2022-10-04T17:06:33Z

I have the impression the merge of this PR was unduly rushed.
Can the proponents clarify the hurry?

slava77 · 2022-10-04T17:19:42Z

I have the impression the merge of this PR was unduly rushed. Can the proponents clarify the hurry?

pre3 release.

How would a not rushed path look like?
(considering that I brought this up in the ORP, the signoff by POG may be outside of reco/ORP understanding)

mmusich · 2022-10-04T17:32:06Z

pre3 release

please clarify why it was needed for pre3.

How would a not rushed path look like?

e.g. understanding in detail the large deal of differences seen in tests. 1 day for a PR review looks very rushed by normal reconstruction integration trajectories.

slava77 · 2022-10-04T17:35:01Z

pre3 release

please clarify why it was needed for pre3.

relvals. fewer features per pre-release is better.

mmusich · 2022-10-04T17:36:18Z

relvals. fewer features per pre-release is better.

Good to know. Let's apply the same principle everywhere.

mmusich · 2022-10-04T17:40:50Z

type tracking, performance-improvements

mmasciov added 2 commits October 3, 2022 05:19

Use parabolic magnetic field and corrsponding propagator in track fit

e8a5ff5

Decrease cut on min number of hits in KFFittingSmoother used for dupl…

d975c87

…icate merge

cmsbuild added this to the CMSSW_12_6_X milestone Oct 3, 2022

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

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

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

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

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

Use parabolic magnetic field only for main tracking iterations

b20be59

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

cmsbuild added the urgent label Oct 4, 2022

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

rappoccio merged commit 5bacac1 into cms-sw:master Oct 4, 2022

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

perrotta reviewed Oct 4, 2022

View reviewed changes

slava77 reviewed Oct 4, 2022

View reviewed changes

mmasciov mentioned this pull request Oct 4, 2022

[Fix] Decrease cut on min number of hits only in RKFittingSmoother #39611

Merged

cmsbuild added performance-improvements tracking labels Oct 4, 2022

mmasciov mentioned this pull request May 7, 2023

Introduce procModifier for parabolic magnetic field in track building for default iterative tracking #41557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use parabolic magnetic field in track final fit and modify min N(hits) condition in track duplicate merge #39578

Use parabolic magnetic field in track final fit and modify min N(hits) condition in track duplicate merge #39578

mmasciov commented Oct 3, 2022 •

edited

Loading

cmsbuild commented Oct 3, 2022

cmsbuild commented Oct 3, 2022

mandrenguyen commented Oct 3, 2022

mmusich commented Oct 3, 2022

slava77 commented Oct 3, 2022

cmsbuild commented Oct 3, 2022

slava77 commented Oct 3, 2022

slava77 commented Oct 3, 2022

slava77 commented Oct 3, 2022

mandrenguyen commented Oct 4, 2022

clacaputo commented Oct 4, 2022 •

edited

Loading

slava77 commented Oct 4, 2022

slava77 commented Oct 4, 2022 •

edited

Loading

rappoccio commented Oct 4, 2022

mandrenguyen commented Oct 4, 2022

slava77 commented Oct 4, 2022

mandrenguyen commented Oct 4, 2022

cmsbuild commented Oct 4, 2022

rappoccio commented Oct 4, 2022

perrotta Oct 4, 2022

slava77 Oct 4, 2022

slava77 Oct 4, 2022

perrotta Oct 4, 2022

mmasciov Oct 4, 2022

mmusich commented Oct 4, 2022

slava77 commented Oct 4, 2022

mmusich commented Oct 4, 2022

slava77 commented Oct 4, 2022

mmusich commented Oct 4, 2022

mmusich commented Oct 4, 2022

Use parabolic magnetic field in track final fit and modify min N(hits) condition in track duplicate merge #39578

Use parabolic magnetic field in track final fit and modify min N(hits) condition in track duplicate merge #39578

Conversation

mmasciov commented Oct 3, 2022 • edited Loading

PR description:

PR validation:

cmsbuild commented Oct 3, 2022

cmsbuild commented Oct 3, 2022

mandrenguyen commented Oct 3, 2022

mmusich commented Oct 3, 2022

slava77 commented Oct 3, 2022

cmsbuild commented Oct 3, 2022

Comparison Summary

slava77 commented Oct 3, 2022

slava77 commented Oct 3, 2022

slava77 commented Oct 3, 2022

mandrenguyen commented Oct 4, 2022

clacaputo commented Oct 4, 2022 • edited Loading

slava77 commented Oct 4, 2022

slava77 commented Oct 4, 2022 • edited Loading

rappoccio commented Oct 4, 2022

mandrenguyen commented Oct 4, 2022

slava77 commented Oct 4, 2022

mandrenguyen commented Oct 4, 2022

cmsbuild commented Oct 4, 2022

rappoccio commented Oct 4, 2022

perrotta Oct 4, 2022

Choose a reason for hiding this comment

slava77 Oct 4, 2022

Choose a reason for hiding this comment

slava77 Oct 4, 2022

Choose a reason for hiding this comment

perrotta Oct 4, 2022

Choose a reason for hiding this comment

mmasciov Oct 4, 2022

Choose a reason for hiding this comment

mmusich commented Oct 4, 2022

slava77 commented Oct 4, 2022

mmusich commented Oct 4, 2022

slava77 commented Oct 4, 2022

mmusich commented Oct 4, 2022

mmusich commented Oct 4, 2022

mmasciov commented Oct 3, 2022 •

edited

Loading

clacaputo commented Oct 4, 2022 •

edited

Loading

slava77 commented Oct 4, 2022 •

edited

Loading