[MkFit] Use binnor for seed and hit sorting, remove Ice/ #37586

osschar · 2022-04-15T17:43:16Z

Changes

Add radix-sort option in binnor.
While importing seeds use binnor for sorting or rely on previous sorting done during seed cleaning.
Split track-related classes out of HitStructures.h/cc into a new TrackStructures.h/cc file-set.
Use binnor with radix_sort in LayerOfHits.
Use single std::vector for storing dead-bin information.
Remove MkFitCore/src/Ice/ directory, remove -Wno-error=strict-aliasing from
RecoTracker/MkFitCore/BuildFile.xml.
Improve std-vs-radix sort heuristic, cleanup binnor_demo.cxx (use same axis parameters as in LayerOfHits; perform timing-loop multiple times).

Validation

MTV plots http://uaf-10.t2.ucsd.edu/~mmasciov/MIC/IceOut/MTV_mkFit_TTbarPU_from1240pre2_cmssw1240pre2_IceOut/

Efficiencies remain unchanged. There are minor fluctuations in fake/duplicate rates induced by more precise sorting of hits in phi and potentially also from additional sorting of incoming seeds in phi (they were only sorted in eta before). There are additional hit-on-track processing changes expected in treatment of dead-regions, large clusters, and through usage of the additional per-module information that is now available -- following all these selection windows will be retuned to achieve optimal performance.

Related issues

This PR addresses the second long term issue in mkFit To-Do list #36966.
Also included are minor log-printout changes requested by @qliphy in [MkFit] Tracker geometry extraction from Reco geom modules #37418 that were postponed in that PR (see commit 958ba5f).

…ing done during seed cleaning.

…kStructures.h/cc file-set.

Use single std::vector<bool> for storing dead-bin information. Remove MkFitCore/src/Ice/ directory.

…lized, fix std::sort codepath, only use radix if n-entries >= 128.

…ple times.

… entries in loops over bins.

…ndex-type bits.

cmsbuild · 2022-04-15T17:52:22Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37586/29339

This PR adds an extra 188KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoTracker/MkFit/plugins/MkFitIterationConfigESProducer.cc modified in PR(s): Add min pT filter in mkFit iterations during track building #37581
- File RecoTracker/MkFitCore/interface/IterationConfig.h modified in PR(s): Add min pT filter in mkFit iterations during track building #37581, MkFit TrackCandidate error rescale + relaxed selection for overlap hits #37580
- File RecoTracker/MkFitCore/src/MkBuilder.cc modified in PR(s): Add min pT filter in mkFit iterations during track building #37581
- File RecoTracker/MkFitCore/src/MkFinder.cc modified in PR(s): MkFit TrackCandidate error rescale + relaxed selection for overlap hits #37580

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-37586/29339/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37586/29339/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2022-04-15T18:01:57Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37586/29340

This PR adds an extra 216KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoTracker/MkFit/plugins/MkFitIterationConfigESProducer.cc modified in PR(s): Add min pT filter in mkFit iterations during track building #37581
- File RecoTracker/MkFitCore/interface/IterationConfig.h modified in PR(s): Add min pT filter in mkFit iterations during track building #37581, MkFit TrackCandidate error rescale + relaxed selection for overlap hits #37580
- File RecoTracker/MkFitCore/src/MkBuilder.cc modified in PR(s): Add min pT filter in mkFit iterations during track building #37581
- File RecoTracker/MkFitCore/src/MkFinder.cc modified in PR(s): MkFit TrackCandidate error rescale + relaxed selection for overlap hits #37580

cmsbuild · 2022-04-15T18:02:21Z

A new Pull Request was created by @osschar (Matevž Tadel) for master.

It involves the following packages:

RecoTracker/MkFit (reconstruction)
RecoTracker/MkFitCMS (reconstruction)
RecoTracker/MkFitCore (reconstruction)

@jpata, @cmsbuild, @clacaputo, @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 · 2022-04-25T16:40:53Z

@cms-sw/tracking-pog-l2 @mmusich @cms-sw/reconstruction-l2
please clarify on the plans for your review of this PR.
Please expedite if possible.
We have more PRs in mkFIt that would depend on this merged.
Thank you.

mmusich · 2022-04-26T06:57:38Z

+1

technical
addresses the second long term issue in mkFit To-Do list #36966.

jpata · 2022-04-26T08:23:50Z

+reconstruction

improved hit sorting and code reorganization
minor changes to tracks and everything downstream (per the usual reco language, not a technical PR)

cmsbuild · 2022-04-26T08:24:08Z

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)

perrotta · 2022-04-26T10:58:19Z

Is there any evaluation of the effect on timing? Modifying sorting I'd expect can have some...

slava77 · 2022-04-26T13:58:53Z

test parameters:

workflow_profiling = 11834.21
enable_test = profiling

slava77 · 2022-04-26T14:00:10Z

@cmsbuild please test

slava77 · 2022-04-26T14:02:00Z

@smuzaffar
is there a way to run only a profiling test (after the build), skip the relvals and unit tests?

smuzaffar · 2022-04-26T14:05:06Z

@slava77 , no sorry currently there is no way to do this.

osschar · 2022-04-26T15:46:24Z

Is there any evaluation of the effect on timing? Modifying sorting I'd expect can have some...

I actually ported radix sort of integers into binnor so I don't expect significant changes (if any, I guess they will be in the noise). But yes, makes sense to check :)

cmsbuild · 2022-04-26T17:53:41Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3a9357/24228/summary.html
COMMIT: 6976fc2
CMSSW: CMSSW_12_4_X_2022-04-26-1100/slc7_amd64_gcc10
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37586/24228/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-3a9357/39434.75_TTbar_14TeV+2026D88_HLT75e33+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+DigiTrigger+RecoGlobal+HLT75e33+HARVESTGlobal

Summary:

No significant changes to the logs found
Reco comparison results: 11290 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3695434
DQMHistoTests: Total failures: 9791
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3685620
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 48 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 205 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

slava77 · 2022-04-26T18:56:05Z

Additional Tests: PROFILING

MkFitProducer igprof from 24.34 to 24.74
- essentially all of it is in mkfit::run_OneIteration from 24.09 to 24.57 . Most of the difference seems accidental though, either from timing reproducibility or from a change in order of some objects.
  - find_tracks_load_seeds is the affected function call, its cost goes down 1.67 to 1.56
MkFitEventOfHitsProducer from 0.51 to 0.61
- mkfit::LayerOfHits::endRegistrationOfHits from 0.09 to 0.15 this is probably the clearest measure (not mixed with other calls)
overall, there seems to be up to 0.5 (0.1% of the job cost) increase in cost, but the more likely value is closer to 0.1 (0.02%) based on the analysis of related contributions.

perrotta · 2022-04-27T11:57:58Z

+1

Thank you @slava77, all this looks quite reasonable

osschar added 11 commits April 6, 2022 09:20

Add radix-sort option in binnor.

f2ce998

Use edm::Log for geometry producer, add MkFit/test/BuildFile.xml.

958ba5f

while importing seeds use binnor for sorting or rely on previous sort…

2258fdb

…ing done during seed cleaning.

Split track-related classes out of HitStructures.h/cc into a new Trac…

5ad71f7

…kStructures.h/cc file-set.

Use binnor with radix_sort in LayerOfHits.

33daf82

Use single std::vector<bool> for storing dead-bin information. Remove MkFitCore/src/Ice/ directory.

Fixes and improvements in usage of binnor.

b74a386

Bufix - properly account for case when ranks have not yet been initia…

9a1c59f

…lized, fix std::sort codepath, only use radix if n-entries >= 128.

Use same axis parameters as in LayerOfHits; perform timing-loop multi…

f368041

…ple times.

Improve std-vs-radix sort heuristic, cleanup innor_demo.cxx.

c4bab34

Fix clamping of real to safe-bin-index calculation, skip empty binned…

dd2f00a

… entries in loops over bins.

Make safe-bin-index calculation safe also when bin-axis uses up all i…

cb4b7ce

…ndex-type bits.

cmsbuild added this to the CMSSW_12_4_X milestone Apr 15, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 15, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Apr 15, 2022

code check, one extra space in front of an in-line comment :)

bd01102

cmsbuild added code-checks-pending and removed code-checks-rejected labels Apr 15, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 15, 2022

Remove redundant checks.

02d29cd

cmsbuild added code-checks-pending and removed code-checks-approved labels Apr 15, 2022

cmsbuild added tracking-pog-approved and removed tracking-pog-pending labels Apr 26, 2022

cmsbuild added fully-signed reconstruction-approved and removed reconstruction-pending pending-signatures labels Apr 26, 2022

cmsbuild added tests-started and removed tests-approved labels Apr 26, 2022

cmsbuild added tests-approved and removed tests-started labels Apr 26, 2022

cmsbuild added orp-approved and removed orp-pending labels Apr 27, 2022

cmsbuild merged commit ea4db72 into cms-sw:master Apr 27, 2022

cmsbuild mentioned this pull request Apr 28, 2022

Attempt to fix compilation warnings and optimize usage of VecGeom cms-sw/cmsdist#7821

Merged

clacaputo mentioned this pull request Apr 28, 2022

mkFit To-Do list #36966

Open

9 tasks

This was referenced May 31, 2022

Crash in MkFit affecting PromptReco #38127

Closed

[MkFitCore] Backport binnor changes to fix 38127. #38151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MkFit] Use binnor for seed and hit sorting, remove Ice/ #37586

[MkFit] Use binnor for seed and hit sorting, remove Ice/ #37586

osschar commented Apr 15, 2022 •

edited

Loading

cmsbuild commented Apr 15, 2022

cmsbuild commented Apr 15, 2022

cmsbuild commented Apr 15, 2022

slava77 commented Apr 25, 2022

mmusich commented Apr 26, 2022

jpata commented Apr 26, 2022

cmsbuild commented Apr 26, 2022

perrotta commented Apr 26, 2022

slava77 commented Apr 26, 2022

slava77 commented Apr 26, 2022

slava77 commented Apr 26, 2022

smuzaffar commented Apr 26, 2022

osschar commented Apr 26, 2022

cmsbuild commented Apr 26, 2022

slava77 commented Apr 26, 2022

perrotta commented Apr 27, 2022

[MkFit] Use binnor for seed and hit sorting, remove Ice/ #37586

[MkFit] Use binnor for seed and hit sorting, remove Ice/ #37586

Conversation

osschar commented Apr 15, 2022 • edited Loading

Changes

Validation

Related issues

cmsbuild commented Apr 15, 2022

cmsbuild commented Apr 15, 2022

cmsbuild commented Apr 15, 2022

slava77 commented Apr 25, 2022

mmusich commented Apr 26, 2022

jpata commented Apr 26, 2022

cmsbuild commented Apr 26, 2022

perrotta commented Apr 26, 2022

slava77 commented Apr 26, 2022

slava77 commented Apr 26, 2022

slava77 commented Apr 26, 2022

smuzaffar commented Apr 26, 2022

osschar commented Apr 26, 2022

cmsbuild commented Apr 26, 2022

Comparison Summary

slava77 commented Apr 26, 2022

perrotta commented Apr 27, 2022

osschar commented Apr 15, 2022 •

edited

Loading