Update mkFit for CMSSW_12_2_0 #35974

mmasciov · 2021-11-03T18:04:56Z

PR description:

This PR follows #35652 and updates mkFit in view of CMSSW_12_2_0.

In detail, this PR:

updates the mkFit track seed partition function for the definition of parallel tasks;
updates the mkFit track seed converter to account for matched hits in track seeds;
(via Update mkFit release to V3.5.1-0 cmsdist#7427 and Update mkFit JSON configuration files for CMSSW_12_2_0 cms-data/RecoTracker-MkFit#7) updates mkFit release and configurations in order to improve the overall physics performance at track-building level for tracking iterations where mkFit is enabled (as in PR Enable mkFit by default in 4+2 tracking iterations (initialStepPreSpl… #35660).

It requires cms-sw/cmsdist#7427 and cms-data/RecoTracker-MkFit#7.
Note: these PRs were updated in response to BTV results in https://its.cern.ch/jira/browse/PDMVRELVALS-136, in order to improve the track impact parameter resolution in tracks reconstructed in pixel-triplet-seeded iterations. Effects of the update on track reconstructions efficiency and fakes are small.

PR validation:

Details of performance after this PR have been presented at TRK POG meeting on November 1st:
https://indico.cern.ch/event/1092090/#2-mkfit-status-report

Full MTV results, including the update of cms-sw/cmsdist#7427 and cms-data/RecoTracker-MkFit#7 on November 9th:

TTbar with PU: https://mmasciovecchio.web.cern.ch/MkFit_hitSelectionWindows_Nov21/MTV_TTbar_PU_PR35974_updateNov9_12X/
High-pT QCD without PU: https://mmasciovecchio.web.cern.ch/MkFit_hitSelectionWindows_Nov21/MTV_highPtQCD_noPU_PR35974_updateNov9_12X/

cmsbuild · 2021-11-03T18:12:21Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35974/26401

This PR adds an extra 16KB to repository

cmsbuild · 2021-11-03T18:12:45Z

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

It involves the following packages:

RecoTracker/MkFit (reconstruction)

@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @ebrondol, @gpetruc, @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 · 2021-11-05T13:39:03Z

test parameters:

pull_request = Update mkFit release to V3.5.1-0 cmsdist#7427,Update mkFit JSON configuration files for CMSSW_12_2_0 cms-data/RecoTracker-MkFit#7

slava77 · 2021-11-05T13:39:15Z

@cmsbuild please test

cmsbuild · 2021-11-05T18:04:48Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ca13d3/20278/summary.html
COMMIT: dbdcfb7
CMSSW: CMSSW_12_2_X_2021-11-05-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35974/20278/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: 20095 differences found in the comparisons
DQMHistoTests: Total files compared: 42
DQMHistoTests: Total histograms compared: 2901890
DQMHistoTests: Total failures: 30262
DQMHistoTests: Total nulls: 6
DQMHistoTests: Total successes: 2871600
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.071 KiB( 41 files compared)
DQMHistoSizes: changed ( 136.793 ): -0.012 KiB JetMET/SUSYDQM
DQMHistoSizes: changed ( 136.874 ): -0.059 KiB JetMET/SUSYDQM
Checked 177 log files, 37 edm output root files, 42 DQM output files
TriggerResults: no differences found

jpata · 2021-11-08T12:59:17Z

Small differences are seen in tracks, and thus in all downstream reco quantities.
Do these look in line with what is expected based on the report at the TRK POG?

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_2_X_2021-11-05-1100+ca13d3/46688/validateJR/all_OldVSNew_RunDoubleEG2017Cwf136p793/all_OldVSNew_RunDoubleEG2017Cwf136p793c_recoTracks_generalTracks__reRECO_obj_eta.png

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_2_X_2021-11-05-1100+ca13d3/46688/validateJR/all_OldVSNew_RunDoubleEG2017Cwf136p793/all_OldVSNew_RunDoubleEG2017Cwf136p793c_log10recoTracks_generalTracks__reRECO_obj_pt.png

jpata

minor comments inline

jpata · 2021-11-08T13:00:32Z

RecoTracker/MkFit/plugins/MkFitIterationConfigESProducer.cc

+    for (int i = 0; i < size; ++i) {
+      const Track &S = in_seeds[i];
+
+      HitOnTrack hot = S.getLastHitOnTrack();


Suggested change

HitOnTrack hot = S.getLastHitOnTrack();

const auto& hot = S.getLastHitOnTrack();

perhaps?

jpata · 2021-11-08T13:00:41Z

RecoTracker/MkFit/plugins/MkFitIterationConfigESProducer.cc

+      const Track &S = in_seeds[i];
+
+      HitOnTrack hot = S.getLastHitOnTrack();
+      float eta = eoh[hot.layer].GetHit(hot.index).eta();


Suggested change

float eta = eoh[hot.layer].GetHit(hot.index).eta();

const float eta = eoh[hot.layer].GetHit(hot.index).eta();

jpata · 2021-11-08T13:00:53Z

RecoTracker/MkFit/plugins/MkFitIterationConfigESProducer.cc

+      return inside;
+    };
+
+    for (int i = 0; i < size; ++i) {


Suggested change

for (int i = 0; i < size; ++i) {

for (size_t i = 0; i < size; ++i) {

jpata · 2021-11-08T13:01:22Z

RecoTracker/MkFit/plugins/MkFitIterationConfigESProducer.cc

+          bool in_tid = endcap_pos_check(S, maxR, tidp_rout, tidp_rin, tidp1.m_zmin - tid_z_extra);
+          bool in_tec = endcap_pos_check(S, maxR, tecp_rout, tecp_rin, tecp1.m_zmin - tec_z_extra);


Suggested change

bool in_tid = endcap_pos_check(S, maxR, tidp_rout, tidp_rin, tidp1.m_zmin - tid_z_extra);

bool in_tec = endcap_pos_check(S, maxR, tecp_rout, tecp_rin, tecp1.m_zmin - tec_z_extra);

const bool in_tid = endcap_pos_check(S, maxR, tidp_rout, tidp_rin, tidp1.m_zmin - tid_z_extra);

const bool in_tec = endcap_pos_check(S, maxR, tecp_rout, tecp_rin, tecp1.m_zmin - tec_z_extra);

also below

jpata · 2021-11-08T13:01:54Z

RecoTracker/MkFit/plugins/MkFitIterationConfigESProducer.cc

+      }
+
+      part.m_region[i] = reg;
+      part.m_sort_score[i] = 7.0f * (reg - 2) + eta;


this "magic formula" may need some comment or explanation.

mmasciov · 2021-11-09T02:42:03Z

Small differences are seen in tracks, and thus in all downstream reco quantities. Do these look in line with what is expected based on the report at the TRK POG?

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_2_X_2021-11-05-1100+ca13d3/46688/validateJR/all_OldVSNew_RunDoubleEG2017Cwf136p793/all_OldVSNew_RunDoubleEG2017Cwf136p793c_recoTracks_generalTracks__reRECO_obj_eta.png

https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_2_X_2021-11-05-1100+ca13d3/46688/validateJR/all_OldVSNew_RunDoubleEG2017Cwf136p793/all_OldVSNew_RunDoubleEG2017Cwf136p793c_log10recoTracks_generalTracks__reRECO_obj_pt.png

@jpata, yes: few/small differences are expected and look in line with expectations.

Let us note that we have updated this PR via the related PRs cms-sw/cmsdist#7427 and cms-data/RecoTracker-MkFit#7, in response to BTV validation results highlighted in https://its.cern.ch/jira/browse/PDMVRELVALS-136. The PR description is going to be updated accordingly. This update allows to improve impact parameter resolution, with minor changes to track reconstruction efficiency and fake rate. MTV results are going to be provided in the PR description.

slava77 · 2021-11-09T16:32:02Z

I think the resolution of this issue is still not in the 1100 IB

why do you think so?
the #36021 is merged in CMSSW_12_2_X_2021-11-09-1100

mmasciov · 2021-11-09T17:20:52Z

Is this supposed to be also backported to 12_1_X?

@tvami I will open a PR for backport to 12_1_X once this is signed.

cmsbuild · 2021-11-09T20:22:31Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ca13d3/20397/summary.html
COMMIT: 6cebeea
CMSSW: CMSSW_12_2_X_2021-11-08-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35974/20397/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
11634.911 step 1
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

No significant changes to the logs found
Reco comparison results: 17336 differences found in the comparisons
DQMHistoTests: Total files compared: 41
DQMHistoTests: Total histograms compared: 2694541
DQMHistoTests: Total failures: 29878
DQMHistoTests: Total nulls: 6
DQMHistoTests: Total successes: 2664635
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.067 KiB( 40 files compared)
DQMHistoSizes: changed ( 136.793 ): -0.020 KiB JetMET/SUSYDQM
DQMHistoSizes: changed ( 136.874 ): -0.047 KiB JetMET/SUSYDQM
Checked 173 log files, 37 edm output root files, 41 DQM output files
TriggerResults: no differences found

jpata · 2021-11-10T11:53:22Z

+reconstruction

updates mkFit with corresponding small reco changes in all downstream quantities (detailed slides are linked in the description
should be merged concurrently with Update mkFit release to V3.5.1-0 cmsdist#7427, Update mkFit JSON configuration files for CMSSW_12_2_0 cms-data/RecoTracker-MkFit#7

cmsbuild · 2021-11-10T11:53:45Z

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)

tvami · 2021-11-10T13:39:36Z

urgent

mmasciov · 2021-11-10T13:51:58Z

Is this supposed to be also backported to 12_1_X?

@tvami I will open a PR for backport to 12_1_X once this is signed.

This is done in PR #36075.

qliphy · 2021-11-11T05:03:30Z

+1

fwyzard · 2021-11-11T13:29:11Z

RecoTracker/MkFit/plugins/MkFitIterationConfigESProducer.cc

+    const LayerInfo &tib1 = trk_info.m_layers[4];
+    const LayerInfo &tob1 = trk_info.m_layers[10];
+
+    const LayerInfo &tidp1 = trk_info.m_layers[21];
+    const LayerInfo &tidn1 = trk_info.m_layers[48];
+
+    const LayerInfo &tecp1 = trk_info.m_layers[27];
+    const LayerInfo &tecn1 = trk_info.m_layers[54];
+
+    // Merge first two layers to account for mono/stereo coverage.
+    // TrackerInfo could hold joint limits for sub-detectors.
+    const auto &L = trk_info.m_layers;
+    const float tidp_rin = std::min(L[21].m_rin, L[22].m_rin);
+    const float tidp_rout = std::max(L[21].m_rout, L[22].m_rout);
+    const float tecp_rin = std::min(L[27].m_rin, L[28].m_rin);
+    const float tecp_rout = std::max(L[27].m_rout, L[28].m_rout);
+    const float tidn_rin = std::min(L[48].m_rin, L[49].m_rin);
+    const float tidn_rout = std::max(L[48].m_rout, L[49].m_rout);
+    const float tecn_rin = std::min(L[54].m_rin, L[55].m_rin);
+    const float tecn_rout = std::max(L[54].m_rout, L[55].m_rout);


During the review of the Patatrack PRs we received a lot of comments regarding the use of "magic" numbers in the pixel code - even though most of the time we had simply modified or copied existing code.

Could you fix throughout this (and similar files, if any) the use of 4, 10, 21, 48, 27, 54, ... ?

@fwyzard, thanks for your comment. This has been addressed in #36246.

[12.1.X] Update of mkFit as in 12_2_0 (backport of PR #35974)

Update mkFit seed converted and partition function

dbdcfb7

mmasciov mentioned this pull request Nov 3, 2021

Update mkFit JSON configuration files for CMSSW_12_2_0 cms-data/RecoTracker-MkFit#7

Merged

cmsbuild added this to the CMSSW_12_2_X milestone Nov 3, 2021

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Nov 3, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 3, 2021

mmasciov mentioned this pull request Nov 3, 2021

re-enable LogWarnings in MkFitOutputConverter #35803

Open

makortel mentioned this pull request Nov 3, 2021

Update mkFit release to V3.5.1-0 cms-sw/cmsdist#7427

Merged

cmsbuild added requires-external tests-started and removed tests-pending labels Nov 5, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 5, 2021

jpata reviewed Nov 8, 2021

View reviewed changes

Implement comments from PR review

6cebeea

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Nov 9, 2021

cmsbuild added the tests-started label Nov 9, 2021

cmsbuild added tests-approved and removed tests-started labels Nov 9, 2021

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Nov 10, 2021

smuzaffar mentioned this pull request Nov 10, 2021

Update tag for RecoTracker-MkFit to V00-07-00 cms-sw/cmsdist#7443

Merged

cmsbuild added the urgent label Nov 10, 2021

This was referenced Nov 10, 2021

[12.1.X] Update of mkFit release and configuration (backport of PRs #7427 + #7443) cms-sw/cmsdist#7444

Merged

[12.1.X] Update of mkFit as in 12_2_0 (backport of PR #35974) #36075

Merged

cmsbuild added orp-approved and removed orp-pending labels Nov 11, 2021

cmsbuild merged commit c7d19e4 into cms-sw:master Nov 11, 2021

cmsbuild mentioned this pull request Nov 11, 2021

Move to root 6.24 cms-sw/cmsdist#7445

Merged

fwyzard reviewed Nov 11, 2021

View reviewed changes

cmsbuild added a commit that referenced this pull request Nov 13, 2021

Merge pull request #36075 from trackreco/backportPR35974

e9f7bbc

[12.1.X] Update of mkFit as in 12_2_0 (backport of PR #35974)

mmasciov added a commit to trackreco/cmssw that referenced this pull request Nov 24, 2021

Backport from cms-sw#35974 + additional comments

76bb59b

This was referenced Nov 24, 2021

Update JSON files + backport of last CMSSW PR trackreco/cmssw#68

Merged

Update mkFit for CMSSW_12_2_0_pre3 #36246

Merged

makortel pushed a commit to trackreco/cmssw that referenced this pull request Nov 25, 2021

Backport from cms-sw#35974 + additional comments

f2ae129

missirol mentioned this pull request Feb 23, 2022

Update to L1 prescale code following move to fractional prescales #37046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update mkFit for CMSSW_12_2_0 #35974

Update mkFit for CMSSW_12_2_0 #35974

mmasciov commented Nov 3, 2021 •

edited

cmsbuild commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

slava77 commented Nov 5, 2021

slava77 commented Nov 5, 2021

cmsbuild commented Nov 5, 2021

jpata commented Nov 8, 2021

jpata left a comment

jpata Nov 8, 2021

mmasciov Nov 9, 2021

jpata Nov 8, 2021

mmasciov Nov 9, 2021

jpata Nov 8, 2021

mmasciov Nov 9, 2021

jpata Nov 8, 2021

mmasciov Nov 9, 2021

jpata Nov 8, 2021

mmasciov Nov 9, 2021

mmasciov commented Nov 9, 2021

slava77 commented Nov 9, 2021

mmasciov commented Nov 9, 2021

cmsbuild commented Nov 9, 2021

jpata commented Nov 10, 2021

cmsbuild commented Nov 10, 2021

tvami commented Nov 10, 2021

mmasciov commented Nov 10, 2021

qliphy commented Nov 11, 2021

fwyzard Nov 11, 2021

mmasciov Nov 25, 2021

	HitOnTrack hot = S.getLastHitOnTrack();
	const auto& hot = S.getLastHitOnTrack();

	float eta = eoh[hot.layer].GetHit(hot.index).eta();
	const float eta = eoh[hot.layer].GetHit(hot.index).eta();

	for (int i = 0; i < size; ++i) {
	for (size_t i = 0; i < size; ++i) {

		bool in_tid = endcap_pos_check(S, maxR, tidp_rout, tidp_rin, tidp1.m_zmin - tid_z_extra);
		bool in_tec = endcap_pos_check(S, maxR, tecp_rout, tecp_rin, tecp1.m_zmin - tec_z_extra);

Update mkFit for CMSSW_12_2_0 #35974

Update mkFit for CMSSW_12_2_0 #35974

Conversation

mmasciov commented Nov 3, 2021 • edited

PR description:

PR validation:

cmsbuild commented Nov 3, 2021

cmsbuild commented Nov 3, 2021

slava77 commented Nov 5, 2021

slava77 commented Nov 5, 2021

cmsbuild commented Nov 5, 2021

Comparison Summary

jpata commented Nov 8, 2021

jpata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmasciov commented Nov 9, 2021

slava77 commented Nov 9, 2021

mmasciov commented Nov 9, 2021

cmsbuild commented Nov 9, 2021

Comparison Summary

jpata commented Nov 10, 2021

cmsbuild commented Nov 10, 2021

tvami commented Nov 10, 2021

mmasciov commented Nov 10, 2021

qliphy commented Nov 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmasciov commented Nov 3, 2021 •

edited