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

Add EDProducers for mkFit #27895

Merged
merged 20 commits into from
Sep 18, 2019
Merged

Add EDProducers for mkFit #27895

merged 20 commits into from
Sep 18, 2019

Conversation

makortel
Copy link
Contributor

@makortel makortel commented Aug 28, 2019

PR description:

This PR adds the CMSSW modules to run mkFit. The external for mkFit has already been added (cms-sw/cmsdist#5150, cms-sw/cmsdist#5169).

The following customize functions are added

  • in RecoTracker/MkFit/python/customizeInitialStepOnly.py
    • customizeInitialStepOnly() limit the iterative tracking to initialStep only
    • customizeInitialStepOnlyNoMTV() in addition, remove MTV
  • in RecoTracker/MkFit/python/customizeInitialStepToMkFit.py
    • customizeInitialStepToMkFit() to replace initialStep pattern recognition with mkFit.

In addition this PR adds a runTheMatrix workflow 11634.7 (2021 TTbar noPU) running the reconstruction with customizeInitialStepToMkFit() customize function.

This PR requires cms-sw/cmsdist#5179 to link on arm and ppc.

PR validation:

Added workflow 11634.7 runs.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@makortel
Copy link
Contributor Author

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27895/11680

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

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27895/11681

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

Configuration/PyReleaseValidation
RecoTracker/MkFit
Validation/RecoTrack

The following packages do not have a category, yet:

RecoTracker/MkFit
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

@cmsbuild, @andrius-k, @chayanit, @zhenhu, @schneiml, @prebello, @kpedro88, @pgunnell, @kmaeshima, @jfernan2, @fioriNTU can you please review it and eventually sign? Thanks.
@felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @mschrode, @wmtford, @gpetruc, @ebrondol, @mtosi, @dgulhan this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

RecoTracker/MkFit
Please create a PR for https://github.com/cms-sw/cms-bot/blob/master/categories_map.py to assign category

Done in cms-sw/cms-bot#1195

@makortel
Copy link
Contributor Author

@cmsbuild, please test workflow 11634.7

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 28, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2239/console Started: 2019/08/28 20:47

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e6d66d/2517/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-e6d66d/11634.7_TTbar_14TeV+TTbar_14TeV_TuneCUETP8M1_2021_GenSimFull+DigiFull_2021+RecoFull_trackingMkFit_2021+HARVESTFull_2021

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2957336
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2956994
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 145 log files, 15 edm output root files, 34 DQM output files

@jfernan2
Copy link
Contributor

+1

@kpedro88
Copy link
Contributor

+upgrade

@chayanit
Copy link

+1

@slava77
Copy link
Contributor

slava77 commented Sep 17, 2019

+1

for #27895 d4140cf

  • code changes are in line with the PR description and the follow up review
  • jenkins tests pass and comparisons with the baseline show no differences (the mkFit workflow is not included in the comparisons); the mkFit test workflow runs OK in jenkins tests
  • local tests with 100 events using 11634.7 (ttbar no PU) and also with 200 events in wf 11834.99 with mkFit enabled (PU50 PU55-75 ttbar 2021 setup) shows somewhat expected behavior (with still a few issues to be resolved in follow up PRs):
    • with PU55-75 the initial step iteration track building part is 0.39 s/ev with mkFit compared to 1.45 s using the standard CKF (a factor of 3.7 faster); of the 0.39 s/ev the input and output data conversion steps take 0.01 and 0.03 s, respectively (12% of the "building" time).
    • Issues to resolve before the code can get to production quality tests:
      • valgrind reports many warnings related to using uninitialized variables in logic
      • I observe non-repeatable behavior in single-thread tests with very small but observable differences in the track parameters
      • duplicate cleaning is not enabled in the default mkFit configuration in CMSSW setup, which does not match the standard setup reported to the tracking POG

@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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

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