[NGT] Introduce general DQM Validation for Secondary Vertexing - #51577
Conversation
|
type ngt |
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51577/50346
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
94c251f to
aa7ce12
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51577/50347
|
|
A new Pull Request was created by @JanGerritSchulz for master. It involves the following packages:
@Moanwar, @civanch, @cmsbuild, @ctarricone, @gabrielmscampos, @jfernan2, @kpedro88, @mandrenguyen, @mdhildreth, @rseidita, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
test parameters:
|
|
@cmsbuild, please test |
|
-1 Failed Tests: HeaderConsistency RelVals AddOn Failed RelValsExpand to see more relval errors ...Failed AddOn Tests |
|
Pull request #51577 was updated. @Moanwar, @civanch, @cmsbuild, @ctarricone, @gabrielmscampos, @jfernan2, @kpedro88, @mandrenguyen, @mdhildreth, @rseidita, @srimanob can you please check and sign again. |
|
@cmsbuild, please test |
- introduce flags ignoreMissingAssociations and ignoreMissingCollections - if they are true LogWarnings are demoted to LogInfo - if they are false and there are missing collections, the job will crash - default setting is false - preferred setting for HLT (where collections are expected to be missing) is true
45d10c6 to
9f1262c
Compare
|
Sorry, I just had to fix a typo in the validator cloning... |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51577/50508
|
|
Pull request #51577 was updated. @Moanwar, @civanch, @cmsbuild, @ctarricone, @gabrielmscampos, @jfernan2, @kpedro88, @mandrenguyen, @mdhildreth, @rseidita, @srimanob can you please check and sign again. |
|
please test |
|
-1 Failed Tests: UnitTests Failed Unit TestsI found 1 errors in the following unit tests: ---> test test_dqm-plot had ERRORS Comparison SummarySummary:
Max Memory Comparisons exceeding threshold@cms-sw/core-l2 , I found 1 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
|
|
@cmsbuild, please test
|
|
+1 Size: This PR adds an extra 16KB to repository Comparison SummarySummary:
Max Memory Comparisons exceeding threshold@cms-sw/core-l2 , I found 1 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
|
|
+dqm |
|
+1 |
1 similar comment
|
+1 |
|
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. @ftenchini, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
|
@JanGerritSchulz @mandrenguyen After the merging of this PR, the following errors in the dictionaries are appearing during our builds (they were already visible in the "Duplicate Dictionaries" section of the test results of this PR): Can they get fixed? |
|
Does the output mean that the dictionary definitions need to go under For the first two, I am a bit puzzled though (but probably I am just not familiar enough with the setup in CMSSW). The |
Good point. I think @cms-sw/core-l2 can provide the correct answer and suggestions for it |
|
Yeah, the (not very smart) checker currently expects all class template instantiations with On a quick look the current placement of VertexToTrackingVertexAssociator is correct, and the checker needs to be updated. I'll take care of it.
|
|
Okay, great. Thanks for the explanation 👍 |
|
Done in #51700 |
| if (const auto *pkd = dynamic_cast<const pat::PackedCandidate *>(&cand)) { | ||
| (void)pkd; // suppress unused-variable warning pending the TODO above | ||
| return reco::TrackRef(); | ||
| } | ||
|
|
||
| return reco::TrackRef(); |
There was a problem hiding this comment.
Just to note that this effectively-no-op use of PackedCandidate resulted in #51709 . Is the TODO going to be addressed in the foreseeable future?
There was a problem hiding this comment.
Thanks for the comment. Reconsidering the point of the functions here, which is comparing vertices with TrackingVertices, and the fact that MiniAODs almost never even contain TrackingVertices (to my knowledge at least), I think the TODO is pointless anyway.
I would remove the TODO comment and the no-op if condition. Do you agree?
PR description:
This PR introduces a new DQM validation for Secondary Vertexing (SV). It follows a similar approach as primary vertex validation, and recycles+extends some of the existing infrastructure:
TrackingVertexreco::Vertexalready exists (same as for PV)TrackingVertexreco::VertexCompositePtrCandidateadded by templating the classfilterSimVerticesForPVs(enabled for PV, disabled for SV)SecondaryVertexAnalyzerBase<VertexCollection>specifically for SV validation:SecondaryVertexAnalyzer = SecondaryVertexAnalyzerBase<std::vector<reco::Vertex>>SecondaryVertexAnalyzerCPC = SecondaryVertexAnalyzerBase<std::vector<reco::VertexCompositePtrCandidate>>More details in the slides linked at the top.
The new validation is also directly included in the central DQM validation workflow for HLT using the two specific sequences
HLTSecondaryVertexValidationandHLTSecondaryVertexPostProcessorSequence. In the DQM output file, the added histograms are put in a new subfolder"DQMData/Run 1/HLT/Run summary/SecondaryVertices/Validation"by default. Example plots can be found in the slides.PR validation:
I tested on larger scale using 9000 TTbar events with 200 PU to produce the plots in the presentation. To run a simple example on small scale, you can use this recipe:
I ran this with and without the PR changes applied. All existing validation histograms seem unchanged, specifically the primary vertexing ones. I think the usual bot tests should be sufficient to test a bit more extensively.
I also did a quick and dirty timing check using RelVal samples, and I couldn't see any significant differences in the timing pie. The SV analyzer itself takes in the order of 10-20 ms.
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
This is not a backport. However, a backport could follow if there's interest to use it for Run-3 performance evaluation as well.
FYI @SWuchterl @philippgadow @pavlo-kashko @giovannicelotto