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

[MkFit] Tracker geometry extraction from Reco geom modules #37418

Merged
merged 15 commits into from
Apr 6, 2022

Conversation

osschar
Copy link
Contributor

@osschar osschar commented Mar 30, 2022

This PR addresses the first long-term item in #36966 - removal of hardcoded phasI geometry from mkFit and implementation of proper geometry extraction from RECO geometry loaded in current cmsRun process.

  • Extract mkFit layer r-z bounding regions and TEC r-coverage gaps from individual modules.
  • Introduce ModuleInfo with module position, normal and principal direction. Each layer holds a std::vector and a mapping from detId to short in-layer module index that was constructed on the fly beforehand.
  • LayerInfo: remove/cleanup is_pixb/pixe/tib/tob/tid/tec data members and functions. They were not really used. Put in 'int subdet' that can be used to the same effect in scoring / cleaning functions.
  • Remove geometry creation files with hardcoded geometry information: MkFit/plugins/createPhase1TrackerGeometry.h/cc/acc

Corresponding changes for standalone extras are on https://github.com/trackreco/mkFit-external/tree/geom-extract

PR validation:

There are minimal changes in efficiency and fake rates, below 1% level. These are expected as the hardcoded mkFit geometry was not identical to one obtained by proper extraction. There are differences in extent of endcap r-gaps (O(1cm)) and z-extent of endcap mkFit layers (O(5cm)).

Timing is not expected to change significantly (if at all) as this PR only affects extents of mkFit layers.

Prototype dumper analyser to be used to export this info back to standalone mkfit.

Miraculously working python config for just constructing the MkFitGeometry and dumping it.
Auto-detect r coverage gap for TEC.

Q-bins come from a table. Apparently different in standalone and cmssw!
To be investigated with Mario.

Remove is_pixb/pixe/tib/tob/tid/tec data members and functions from
LayerInfo. They were not really used. Put in 'int subdet' that can
in principle be used to the same effect.

Remove MkFit/plugins/createPhase1TrackerGeometry* files.

To printout some stuff in dumper-module:
  cmsRun RecoTracker/MkFit/python/dumpMkFitGeometry.py
to be further improved for binary dump of geometry.
Vector of those is kept in LayerInfo.

For standalone usage (and for tracking-ntuple dump) we still need to
export TrackerInfo to a (binary) file and provide loader functions.
Cleanup standalone Makefiles for external Math/.
@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37418/29094

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37418/29095

@cmsbuild
Copy link
Contributor

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

slava77 commented Mar 30, 2022

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2022

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

@slava77
Copy link
Contributor

slava77 commented Apr 2, 2022

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 2, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-520b22/23613/summary.html
COMMIT: b5343ce
CMSSW: CMSSW_12_4_X_2022-04-01-2300/slc7_amd64_gcc10
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/37418/23613/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: 13988 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3593039
  • DQMHistoTests: Total failures: 26541
  • DQMHistoTests: Total nulls: 4
  • DQMHistoTests: Total successes: 3566472
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.012 KiB( 47 files compared)
  • DQMHistoSizes: changed ( 136.874 ): -0.004 KiB MessageLogger/Warnings
  • DQMHistoSizes: changed ( 138.4 ): -0.004 KiB JetMET/SUSYDQM
  • DQMHistoSizes: changed ( 138.5 ): -0.008 KiB JetMET/SUSYDQM
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 200 log files, 45 edm output root files, 48 DQM output files
  • TriggerResults: found differences in 5 / 47 workflows

@slava77
Copy link
Contributor

slava77 commented Apr 2, 2022

Additional Tests: PROFILING

CPU

Memory

  • mkFit geometry 575,420 -> 1,389,137 should correspond to more per-module data; although I didn't try to do the bean counting.

So, this looks somewhat as expected.

@osschar
Copy link
Contributor Author

osschar commented Apr 2, 2022

Yes, we have O(22k) modules with 40 bytes per module -> 22000*40 = 880000

@jpata
Copy link
Contributor

jpata commented Apr 5, 2022

+reconstruction

  • improves geometry handling in mkFit
  • many small changes to tracking and everything downstream, but according to the tracking validation in the PR description, no physics changes are expected
  • CPU and memory do not look to be significantly affected in the igprof test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 5, 2022

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)

@osschar
Copy link
Contributor Author

osschar commented Apr 5, 2022

Rats, I forgot include MkFit/test/BuildFile.xml for building of the standalone geometry dumper, MkFit/test/DumpMkFitGeometry.cc.

Shall I put it in here or I wait until the next PR so we don't need to go through test etc. hooplas again? The file is rather trivial:

matevz@phi3 RecoTracker> cat MkFit/test/BuildFile.xml
<library file="DumpMkFitGeometry.cc" name="RecoTrackerMkFitTest">
  <flags EDM_PLUGIN="1"/>
  <use name="FWCore/Framework"/>
  <use name="RecoTracker/MkFit"/>
  <use name="RecoTracker/MkFitCore"/>
  <use name="RecoTracker/Record"/>
</library>

@qliphy
Copy link
Contributor

qliphy commented Apr 6, 2022

@osschar You may add it within this PR, and can also take the chance to implement
https://github.com/cms-sw/cmssw/pull/37418/files#r839654493

@osschar
Copy link
Contributor Author

osschar commented Apr 6, 2022

Naaah, just go ahead and merge, I'll fix that print in another PR as well, it's almost ready -- the second long term item from our todo list, removal of Ice/ and consolidation of radix sort usage.

@qliphy
Copy link
Contributor

qliphy commented Apr 6, 2022

+1
to follow up on #37418 (comment)

@cmsbuild cmsbuild merged commit 10bbf79 into cms-sw:master Apr 6, 2022
@osschar
Copy link
Contributor Author

osschar commented Apr 6, 2022

Thank you! :)

@mmusich
Copy link
Contributor

mmusich commented Apr 15, 2022

For the record #37418 (comment) has been fixed at #37586

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.

6 participants