Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update of mkFit for 12_1_0_pre3 #35199

Merged
merged 2 commits into from Sep 10, 2021

Conversation

mmasciov
Copy link
Contributor

@mmasciov mmasciov commented Sep 8, 2021

PR description:

This PR follows #34921 and updates mkFit in view of next pre-release.

In detail, this PR includes:

  • usage of SiPixel quality DB information for mkFit;
  • update of mkFit backward search for pixelLessStep and tobTecStep (currently NOT enabled in trackingMkFit procModifier).

It requires cms-sw/cmsdist#7287 and cms-data/RecoTracker-MkFit#4

Developments have been presented in detail at Tracking POG (https://indico.cern.ch/event/1071226/#3-mkfit-status-report).

PR validation:

MTV results for tracking-only validation in TTbar RelVal (with mkFit enabled for 4 iterations as specified above, compared to full CKF [default]):
https://mmasciov.web.cern.ch/mmasciov/MkFitCMSSW/MTV_PRfor1210pre3/
More details and comparisons are included in report at Tracking POG (https://indico.cern.ch/event/1071226/#3-mkfit-status-report).

FYI @mtosi @vmariani @mmusich
(@slava77 @makortel)

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 8, 2021

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35199/25138

ERROR: Build errors found during clang-tidy run.

    spv[TrackerInfo::Reg_Transition_Pos].set_iterator_limits(9, 7, 34);
                                         ^
RecoTracker/MkFit/plugins/createPhase1TrackerGeometry.cc:102:38: error: no member named 'set_iterator_limits' in 'mkfit::SteeringParams' [clang-diagnostic-error]
    spv[TrackerInfo::Reg_Endcap_Pos].set_iterator_limits(8, 6, 19);
                                     ^
RecoTracker/MkFit/plugins/createPhase1TrackerGeometry.cc:106:8: error: no member named 'm_backward_search' in 'mkfit::IterationConfig' [clang-diagnostic-error]
    ic.m_backward_search = true;
       ^
RecoTracker/MkFit/plugins/createPhase1TrackerGeometry.cc:107:8: error: no member named 'm_backward_params' in 'mkfit::IterationConfig' [clang-diagnostic-error]
    ic.m_backward_params = ic.m_params;
       ^
RecoTracker/MkFit/plugins/createPhase1TrackerGeometry.cc:108:8: error: no member named 'm_backward_params' in 'mkfit::IterationConfig' [clang-diagnostic-error]
    ic.m_backward_params.maxHolesPerCand = 2;
       ^
RecoTracker/MkFit/plugins/createPhase1TrackerGeometry.cc:109:8: error: no member named 'm_backward_params' in 'mkfit::IterationConfig' [clang-diagnostic-error]
    ic.m_backward_params.maxConsecHoles = 2;
       ^
RecoTracker/MkFit/plugins/createPhase1TrackerGeometry.cc:112:38: error: no member named 'set_iterator_limits' in 'mkfit::SteeringParams' [clang-diagnostic-error]
    spv[TrackerInfo::Reg_Endcap_Neg].set_iterator_limits(12, 12, 24);
                                     ^
Suppressed 480 warnings (479 in non-user code, 1 with check filters).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

@slava77
Copy link
Contributor

slava77 commented Sep 8, 2021

test parameters:

@slava77
Copy link
Contributor

slava77 commented Sep 9, 2021

code-checks with cms.week1.PR_0bbdd73c/52.0-58ffe511def190eac1e384529f0e3f95

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35199/25154

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2021

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
Copy link
Contributor

slava77 commented Sep 9, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 9, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bfffb2/18447/summary.html
COMMIT: 8426207
CMSSW: CMSSW_12_1_X_2021-09-08-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/35199/18447/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bfffb2/18447/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bfffb2/18447/git-merge-result

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 3 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3001001
  • DQMHistoTests: Total failures: 0
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3000979
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Sep 10, 2021

+reconstruction

@cmsbuild
Copy link
Contributor

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)

@@ -57,10 +62,15 @@ MkFitEventOfHitsProducer::MkFitEventOfHitsProducer(edm::ParameterSet const& iCon
stripClusterIndexToHitToken_{consumes(iConfig.getParameter<edm::InputTag>("stripHits"))},
mkFitGeomToken_{esConsumes()},
putToken_{produces<MkFitEventOfHits>()},
usePixelQualityDB_{iConfig.getParameter<bool>("usePixelQualityDB")},
useStripStripQualityDB_{iConfig.getParameter<bool>("useStripStripQualityDB")} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity (and not related specifically to this PR): why "StripStrip" instead of just "Strip" in the name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For good or bad, to be consistent with existing naming (in functionally similar class) in

if (pset.getParameter<bool>("UseStripStripQualityDB")) {
stripQualityFlags += MeasurementTracker::BadStrips;
if (pset.getUntrackedParameter<bool>("DebugStripStripQualityDB", false)) {
stripQualityDebugFlags += MeasurementTracker::BadStrips;
}
}

@perrotta
Copy link
Contributor

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants