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

L1T phase-2: Add the Phase2 L1T VertexFinder (corrected version) #33752

Merged
merged 31 commits into from May 19, 2021

Conversation

cecilecaillol
Copy link
Contributor

PR description:

Same as #33609 but new PR because had to update to newer CMSSW and pulled more recent unrelated PRs.

This pull request includes:

An EDProducer that implements the primary-vertex-finding algorithms investigated for the L1T Interim TDR.
The EDAnalyser that was used to study the performance of these algorithms.
The Ntupler for analysis of vertexing algorithms.

The code for all of these is fully contained within L1Trigger/VertexFinder, and the l1tVertexFinder namespace; however of course in principle the name of this directory & namespace could be changed to conform to standard conventions.

A few more details on the EDProducer ...

Source code: interface/VertexProducer.h & plugins/VertexProducer.cc
It creates a l1t::VertexCollection, which is based on the L1T vertex data format class
l1vertices - vector of all vertices found by the algo defined in the Python config (default config uses "DBSCAN" algo)
Python config fragment (to be used by default for downstream algos): python/VertexProducer_cff.py
Parameter values are set to run DBSCAN vertex-finding algo
Other vertexing-finding algorithms can be run by changing the value of the Algorithm parameter in the VertexReconstruction parameter set.

Apart from that EDProducer, the other files in this pull request are:

Header + .cc files for AlgoSettings, L1Track, RecoVertex and `VertexFinder classes that are used by the vertex producer itself
interface/selection.h & src/selection.cc : Contains implementation of getPrimaryVertex function, that currently identifies highest-sum-pT vertex in vector of vertex objects.
plugins/VertexAnalyzer.cc : Implements the EDAnalyzer that we use to study the performance of vertex-finding algorithms
Header + .cc files for classes used by the EDAnalyzer: L1TrackTruthMatched, RecoVertexWithTP, Stub & TP

PR validation:

Same as #33609

if this PR is a backport please specify the original PR and why you need to backport that PR:

Same as #33609

Alexx Perloff and others added 30 commits May 3, 2021 15:15
… should have the latest changes from the developers, including the changes originally made in the phase2-l1t-integration-CMSSW_10_1_5 branch.

(cherry picked from commit f380d65)
…er. Make the FastHisto algorithm more configurable. A little bit of cleanup.

(cherry picked from commit 757e021)
… single place. Some header reorganization.

(cherry picked from commit c3e6667)
(cherry picked from commit 3cfca22)
… the VertexFinder. This commit also changes the std::cout statements to edm::MessageLogger statements among other cleanup actions.

(cherry picked from commit 7ea7d8c)
(cherry picked from commit a01b044)
…epends on. While the VertexProducer was plent fast, codes like InputData, TP, Stub, and L1TrackTruthMatched, often wrappers around existing CMSSW data formats, needed some help.

(cherry picked from commit 2abc38b)
… are also some performance improvements.

(cherry picked from commit 743aa67)
@cmsbuild
Copy link
Contributor

Pull request #33752 was updated. @cmsbuild, @rekovic, @civanch, @cecilecaillol, @mdhildreth can you please check and sign again.

@cecilecaillol
Copy link
Contributor Author

please test

@makortel
Copy link
Contributor

So I don't see why it is complaining about duplicate dicationaries. @smuzaffar any ideas?

I think this specific report comes from this script https://github.com/cms-sw/cmssw/blob/master/Utilities/ReleaseScripts/scripts/duplicateReflexLibrarySearch.py, and suspect that the equivDict (or something) would need an update. (I did not spend enough time to fully understand the behavior of the script though)

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3ad3d2/15168/summary.html
COMMIT: 9aa644b
CMSSW: CMSSW_12_0_X_2021-05-18-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33752/15168/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: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2648242
  • DQMHistoTests: Total failures: 12
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2648207
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: no differences found

@cecilecaillol
Copy link
Contributor Author

The same warning about duplicated dictionairies is still there after the inclusion of class versioning. Do we really need to fix it before merging the PR? If so, can you please suggest other solutions?

@silviodonato
Copy link
Contributor

@cms-sw/simulation-l2 do you have further comments?

@cecilecaillol
Copy link
Contributor Author

+l1

@civanch
Copy link
Contributor

civanch commented May 19, 2021

+1

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

@silviodonato
Copy link
Contributor

@makortel
Copy link
Contributor

@cms-sw/core-l2 can we modify https://github.com/cms-sw/cmssw/blob/master/Utilities/ReleaseScripts/scripts/duplicateReflexLibrarySearch.py later?

I'd be ~ok to proceed given that @smuzaffar is away.

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

6 participants