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] Tools supporting better layer-related computations -- technical changes to support further development #43145

Merged
merged 3 commits into from
Nov 3, 2023

Conversation

osschar
Copy link
Contributor

@osschar osschar commented Oct 29, 2023

PR description:

There are no significant physics changes.

Several changes only affect mkFit in standalone mode.

Main goals:

  • Make it easier to express vectorized code.

  • Be able to do simple propagations, parameters only, no errors.

  • Explore more precise hit preselection in each layer (see MkFinder.selectHitIndicesV2()).

  • Make it easier to export low-level algorithm / tracking state for further analysis directly in ROOT format.

  • Allow further development of Phase2 mkFit tracking, together with the new propagate-to-plane functionality (in another PR).

Detailed changes

Add half_length member to ModuleInfo, fill it in MkFitGeometryESProducer.cc

Extend HitInfo with half-strip-length, add qbar (the other of r/z variables), Store fast-access arrays as std::vector.

Move propToR/Z() can/maxReachRadius() from Track to TrackBase.

Implement LayerOfHits::reset() to make sure containers are empty before re-filling. Call this from both methods (beginHitRegistration() and suckInHits()).

Miscellaneous changes to better support dump-hit-window case.

  • RecoTracker/MkFitCore/interface/Track.h Add function mcHitIDofFirstHit() to simplify dump-hit-window code.

  • RecoTracker/MkFitCore/src/MkBuilder.cc Add missing MkFinder setup in dump-hit-window case.

  • RecoTracker/MkFitCore/standalone/Makefile.config Clarify usage of Ofast (by Steve).

  • RecoTracker/MkFitCMS/standalone/Makefile Use GNU make &: syntax (grouped targets) for dictionary targets.

  • RecoTracker/MkFitCore/src/MiniPropagators.h/cc: Implementations of propagators of track parameters only (no errors/covariances), with various levels of approximation and also supporting vectorization.

Use VDT for fast transcendentals in MiniPropagators.

Add scalar and element-wise arithemtic operators and transcendentals to Matriplex. This makes writing vectorized loops using Matriplex variables much clearer.

Prepare infrastructure for deep-dump comparisons in selectHitIndicesV2 (and elsewhere).

  • MkFitCore/interface/Config.h
    Fix typo.

  • MkFitCore/interface/MkBuilder.h MkFitCore/interface/Track.h MkFitCore/interface/TrackStructures.h MkFitCore/src/TrackStructures.cc MkFitCore/src/MkBuilder.cc MkFitCore/standalone/Event.cc MkFitCore/standalone/Event.h
    Pass seed-index in the current seed-vector along. In Event, provide a means of determining sim-track from hits of each seed track.

  • MkFitCore/src/MkBuilder.cc
    Add handling of WSR_Failed.

  • MkFitCore/src/Matrix.h
    Add short-int Matriplex typedefs

  • MkFitCore/src/MiniPropagators.cc MkFitCore/src/MiniPropagators.h
    Fail-flag consistency, rename State dphi to dalpha.

  • MkFitCore/src/MkBase.h
    Add radius() function.

  • MkFitCore/src/MkFinder.cc MkFitCore/src/MkFinder.h
    Modularization of new/old hit-selection. Reimplemented bin-search for edge positions and determined global scaling of bin-search windows to reproduce current behavior.

  • MkFitCore/standalone/Makefile MkFitCore/standalone/Makefile.config
    Add rules for building RntDumper stuff.

  • MkFitCore/standalone/RntDumper/
    New sub-directory: Implementation of classes and structures for dumping ROOT TTrees or RNTuples.

  • MkFitCMS/standalone/Makefile
    Add linking agains libMicRntDump.so

  • MkFitCMS/standalone/MkStandaloneSeqs.cc
    Make it easier to find out why standalone tracks are not matched to sim tracks.

  • MkFitCMS/standalone/Shell.cc MkFitCMS/standalone/buildtestMPlex.cc
    Setup Event current-seed-vector pointers as needed during event processing.

  • MkFitCMS/standalone/mkFit.cc
    Finalize all RntDumpers on exit.

Standalone consolidation plus cleanup of parameters, remove unused ones.

  • MkFitCMS/standalone/buildtestMPlex.cc
    When validation is on make sure seeds are restored after BH, STD, CE runs.

  • MkFitCMS/standalone/mkFit.cc
    Rename run-all to run default -- make it run std, and ce.

  • MkFitCore/src/MkBuilder.cc
    Missing MkFinder setup in backwardFitBH().

  • MkFitCore/standalone/Geoms/CylCowWLids.cc
    Array of eta extents too short.

  • MkFitCore/standalone/Makefile.config
    Consolidate CMSSW and other occurences of Ofast, msse2, std=c++17/1z. Cleanup unused -D defines / settings.

  • MkFitCore/standalone/ConfigStandalone.h
    Remove unused simulation multiple scattering parameters.

PR validation:

No significant physics changes: MTV plots

There are no significant physics changes.

Several changes only affect mkFit in standalone mode.

Main goals:

- Make it easier to express vectorized code.

- Be able to do simple propagations, parameters only, no errors.

- Explore more precise hit preselection in each layer (see
  MkFinder.selectHitIndicesV2()).

- Make it easier to export low-level algorithm / tracking state for
  further analysis directly in ROOT format.

- Allow further development of Phase2 mkFit tracking, together with
  the new propagate-to-plane functionality (in another PR).

Add half_length member to ModuleInfo, fill it in MkFitGeometryESProducer.cc

Extend HitInfo with half-strip-length, add qbar (the other of r/z variables), Store fast-access arrays as std::vector<HitInfo>.

Move propToR/Z() can/maxReachRadius() from Track to TrackBase.

Implement LayerOfHits::reset() to make sure containers are empty before re-filling.
Call this from both methods (beginHitRegistration() and suckInHits()).

Miscellaneous changes to better support dump-hit-window case.

* RecoTracker/MkFitCore/interface/Track.h
  Add function mcHitIDofFirstHit() to simplify dump-hit-window code.

* RecoTracker/MkFitCore/src/MkBuilder.cc
  Add missing MkFinder setup in dump-hit-window case.

* RecoTracker/MkFitCore/standalone/Makefile.config
  Clarify usage of Ofast (by Steve).

* RecoTracker/MkFitCMS/standalone/Makefile
  Use GNU make &: syntax (grouped targets) for dictionary targets.

* RecoTracker/MkFitCore/src/MiniPropagators.h/cc:
  Implementations of propagators of track parameters only (no
  errors/covariances), with various levels of approximation and also
  supporting vectorization.

Use VDT for fast transcendentals in MiniPropagators.

Add scalar and element-wise arithemtic operators and transcendentals
to Matriplex. This makes writing vectorized loops using Matriplex
variables much clearer.

Prepare infrastructure for deep-dump comparisons in selectHitIndicesV2 (and elsewhere).

* MkFitCore/interface/Config.h
  Fix typo.

* MkFitCore/interface/MkBuilder.h
* MkFitCore/interface/Track.h
* MkFitCore/interface/TrackStructures.h
* MkFitCore/src/TrackStructures.cc
* MkFitCore/src/MkBuilder.cc
* MkFitCore/standalone/Event.cc
* MkFitCore/standalone/Event.h
  Pass seed-index in the current seed-vector along.
    In Event, provide a means of determining sim-track from hits of each
      seed track.

* MkFitCore/src/MkBuilder.cc
  Add handling of WSR_Failed.

* MkFitCore/src/Matrix.h
  Add short-int Matriplex typedefs

* MkFitCore/src/MiniPropagators.cc
* MkFitCore/src/MiniPropagators.h
  Fail-flag consistency, rename State dphi to dalpha.

* MkFitCore/src/MkBase.h
  Add radius() function.

* MkFitCore/src/MkFinder.cc
* MkFitCore/src/MkFinder.h
  Modularization of new/old hit-selection.
    Reimplemented bin-search for edge positions and determined global scaling
      of bin-search windows to reproduce current behavior.

* MkFitCore/standalone/Makefile
* MkFitCore/standalone/Makefile.config
  Add rules for building RntDumper stuff.

* MkFitCore/standalone/RntDumper/
  New sub-directory: Implementation of classes and structures for dumping ROOT
    TTrees or RNTuples.

* MkFitCMS/standalone/Makefile
  Add linking agains libMicRntDump.so

* MkFitCMS/standalone/MkStandaloneSeqs.cc
  Make it easier to find out why standalone tracks are not matched to sim
    tracks.

* MkFitCMS/standalone/Shell.cc
* MkFitCMS/standalone/buildtestMPlex.cc
  Setup Event current-seed-vector pointers as needed during event processing.

* MkFitCMS/standalone/mkFit.cc
  Finalize all RntDumpers on exit.

Standalone consolidation plus cleanup of parameters, remove unused ones.

* MkFitCMS/standalone/buildtestMPlex.cc
  When validation is on make sure seeds are restored after BH, STD, CE runs.

* MkFitCMS/standalone/mkFit.cc
  Rename run-all to run default -- make it run std, and ce.

* MkFitCore/src/MkBuilder.cc
  Missing MkFinder setup in backwardFitBH().

* MkFitCore/standalone/Geoms/CylCowWLids.cc
  Array of eta extents too short.

* MkFitCore/standalone/Makefile.config
  Consolidate CMSSW and other occurences of Ofast, msse2, std=c++17/1z.
  Cleanup unused -D defines / settings.

* MkFitCore/standalone/ConfigStandalone.h
  Remove unused simulation multiple scattering parameters.
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43145/37426

@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)

@mandrenguyen, @cmsbuild, @jfernan2 can you please review it and eventually sign? Thanks.
@mmusich, @gpetruc, @missirol, @mtosi, @rovere, @VourMa, @GiacomoSguazzoni, @VinInn, @JanFSchulte, @dgulhan, @makortel, @felicepantaleo this is something you requested to watch as well.
@rappoccio, @antoniovilela, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Oct 29, 2023

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-981bbc/35491/summary.html
COMMIT: 4f6a60e
CMSSW: CMSSW_13_3_X_2023-10-29-0000/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43145/35491/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 29 lines from the logs
  • Reco comparison results: 4895 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3362691
  • DQMHistoTests: Total failures: 49947
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3312722
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 214 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@osschar
Copy link
Contributor Author

osschar commented Oct 29, 2023

There is a minimal conflic between this pr and PR # #43146.
It occurs around MkFitCore/src/MkFinder.cc:170 -- both changes should be accepted.

@mandrenguyen
Copy link
Contributor

I was having a look at the comparisons. They mostly look fine but I'm scratching my head about this pair:
141.044_RunJetMET2023D/all_mini_reRECO_step3_inMINIAOD/c_HBHERecHitsSorted_reducedEgamma_reducedHBHEHits_reRECO_obj_obj_eraw.

141.044_RunJetMET2023D/all_mini_reRECO_step3_inMINIAOD/c_HBHERecHitsSorted_reducedEgamma_reducedHBHEHits_reRECO_obj_obj_energy

Am i just reading too much into some fluctuations?
While we're at it, since most of these changes seem purely technical, can you please point out the ones that lead to all these minor (and hopefully negligible) differences? Thanks!

@osschar
Copy link
Contributor Author

osschar commented Oct 30, 2023

All the differences should come from this section:
https://github.com/cms-sw/cmssw/pull/43145/files#diff-98dcf509af7754b02d4fe0a9902ab6d8514b8401880de93d6fd957c09fa0a87cR454-R457

This blocks adding of hits for further processing (for a given candidate, at the current layer) when the desired radius has not been reached. This can happen close to helix apogee and for very low-pt tracks (I think it was < 0.2 GeV but more likely 0.1GeV). The propagation can also fail rather spectacularly resulting in nans and nonsencical values for hit-search windows (which I noticed in the new version of hit pre-processing function).

As there were no significant changes in the low-level MTV I decided to keep it in.

@mandrenguyen
Copy link
Contributor

Thanks @osschar
Do you see a connection to what you describe and the changes to the hbhe rechits?

@mandrenguyen
Copy link
Contributor

Also, would it make sense to factorize the purely technical changes from the one you refer to? That might help to be sure that there are no unintended consequences from the former.

@osschar
Copy link
Contributor Author

osschar commented Oct 30, 2023

How sensible this is -- I don't know :) Is the plot from particle-flow? The change could lead to some low-pt tracks missing the last hit (likely a bogus one) and then failing the track-quality cuts because of this. But if this is really number of hcal hits, there should be zero effect from a tracking change :)

Other than that, sure, I can take this out ... should I go ahead?

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43145/37507

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2023

Pull request #43145 was updated. @mandrenguyen, @jfernan2, @cmsbuild can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Nov 3, 2023

@cmsbuild please test

let's see if vdt_headers gives fewer differences

@smuzaffar
Copy link
Contributor

OK @slava77, I tried building vdt with some debug print outs and yes

  • LD_PRELOAD=path/libvdt.so test-my-vdt calls the vdt math functions
  • Linking vdt library to cmssw shared lib/plugin is not calling vdt math functions
  • Linking vdt library to the executable calls the vdt math

So one either needs to explicitly link vdt to the main executable (e.g cmsRun) or use LD_PRELOAD.

@makortel
Copy link
Contributor

makortel commented Nov 3, 2023

@smuzaffar This is very tangential to this PR, but should we drop the vdt dependence from L1Trigger/DTTriggerPhase2/BuildFile.xml? From your tests it seems the dependence there is mostly confusing.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2023

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-981bbc/35579/summary.html
COMMIT: 78d80fe
CMSSW: CMSSW_13_3_X_2023-11-03-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/43145/35579/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 312 lines to the logs
  • Reco comparison results: 4868 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3362691
  • DQMHistoTests: Total failures: 35781
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3326888
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 214 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@slava77
Copy link
Contributor

slava77 commented Nov 3, 2023

Reco comparison results: 4868 differences found in the comparisons

this looks essentially the same as in the previous iteration. So, the vdt vs vdt_headers had no impact.

So, having taken off a remote possibility of long range library use effects, I think what remains is just the small changes in tracks propagating to less numerically stable gsfTracks and further on.

@mandrenguyen
Copy link
Contributor

+1
Small changes compatible with expectation based on discussion above

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2023

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. @sextonkennedy, @rappoccio, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2)

@rappoccio
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.

7 participants