improving PixelVertexCollectionTrimmer #33091
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33091/21416
|
|
A new Pull Request was created by @missirol (Marino Missiroli) for master. It involves the following packages: RecoPixelVertexing/PixelVertexFinding @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
sumpt2 is not a particularly unique choice to sort vertices. So, it's less practical to save it as a data member (especially impractical if the computation is done after the vertex is reconstructed). I suggest to save an aligned vector (ValueMap) of sumpt2. This will follow roughly what is done for the offline PV reco. |
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-408cb9/13318/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
cmsbuild
added
code-checks-pending
tests-pending
and removed
code-checks-approved
tests-approved
labels
This change caused a large number of (small) differences in the HLT-related comparisons, so I'm reverting it. PS. Testing with the |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33091/21440
|
@missirol the isssues with wf 11634.911 are known, and as you say do not depend on this PR, see #32963 |
|
Thanks for the info, @perrotta. |
| else if (fractionSumPt2_ > 1) | ||
| edm::LogWarning("Configuration") << "value of \"fractionSumPt2\" is larger than 1."; | ||
|
|
||
| auto const PVcomparerPSet = iConfig.getParameter<edm::ParameterSet>("PVcomparer"); |
There was a problem hiding this comment.
| auto const PVcomparerPSet = iConfig.getParameter<edm::ParameterSet>("PVcomparer"); | |
| auto const& pvComparerPSet = iConfig.getParameterSet("PVcomparer"); |
- capitalization
- using a method returning by ref may be cheaper
| if (fractionSumPt2_ < 0) | ||
| edm::LogWarning("Configuration") << "value of \"fractionSumPt2\" is negative."; | ||
| else if (fractionSumPt2_ > 1) | ||
| edm::LogWarning("Configuration") << "value of \"fractionSumPt2\" is larger than 1."; |
There was a problem hiding this comment.
if this check is needed, perhaps it's more appropriate to throw?
While a value below 0 does nothing (does it even need a warning in this case?), the value above 1 will by construction make an empty vertex producer and seems more of a reason to think that this is a broken config.
IIUC, considering that this is a stream module this warning will appear nStreams times.
There was a problem hiding this comment.
Implemented as suggested (no strong opinion on the choice of warnings or exceptions here).
| edm::LogWarning("Configuration") << "PVcomparer.track_pt_min (" << track_pt_min << ") >= PVcomparer.track_pt_max (" | ||
| << track_pt_max << ") : PVClusterComparer will use pT=" << track_pt_max | ||
| << " for all selected tracks."; |
There was a problem hiding this comment.
wouldn't it make sense to throw here as well?
| for (size_t idx = 0; idx < vtxs.size(); ++idx) | ||
| foms.at(idx) = pvComparer_->pTSquaredSum(vtxs.at(idx)); |
There was a problem hiding this comment.
| for (size_t idx = 0; idx < vtxs.size(); ++idx) | |
| foms.at(idx) = pvComparer_->pTSquaredSum(vtxs.at(idx)); | |
| for (size_t idx = 0; idx < vtxs.size(); ++idx) | |
| foms[idx] = pvComparer_->pTSquaredSum(vtxs[idx]); |
the index range is correct by construction, no need to check with .at
| std::iota(sortIdxs.begin(), sortIdxs.end(), 0); | ||
| std::sort(sortIdxs.begin(), sortIdxs.end(), [&](size_t const i1, size_t const i2) { return foms[i1] > foms[i2]; }); | ||
|
|
||
| auto const minFOM_fromFrac = foms.at(sortIdxs.at(0)) * fractionSumPt2_; |
There was a problem hiding this comment.
| auto const minFOM_fromFrac = foms.at(sortIdxs.at(0)) * fractionSumPt2_; | |
| auto const minFOM_fromFrac = foms[sortIdxs.front()] * fractionSumPt2_; |
it looks like range checking is not needed here either
|
|
||
| auto const minFOM_fromFrac = foms.at(sortIdxs.at(0)) * fractionSumPt2_; | ||
|
|
||
| vtxs_trim->reserve(maxVtx_ < vtxs.size() ? maxVtx_ : vtxs.size()); |
There was a problem hiding this comment.
| vtxs_trim->reserve(maxVtx_ < vtxs.size() ? maxVtx_ : vtxs.size()); | |
| vtxs_trim->reserve(std::min(maxVtx_, vtxs.size())); |
There was a problem hiding this comment.
This was my first thought, but it leads to a compiler error
error: no matching function for call to 'min(const uint&, std::vector<reco::Vertex>::size_type)'
I've implemented the suggested solution, but including a type cast.
| if (foms.at(idx) >= minFOM_fromFrac and foms.at(idx) > minSumPt2_) | ||
| vtxs_trim->emplace_back(vtxs.at(idx)); |
There was a problem hiding this comment.
| if (foms.at(idx) >= minFOM_fromFrac and foms.at(idx) > minSumPt2_) | |
| vtxs_trim->emplace_back(vtxs.at(idx)); | |
| if (foms[idx] >= minFOM_fromFrac and foms[idx] > minSumPt2_) | |
| vtxs_trim->emplace_back(vtxs[idx]); |
seems safe in this context
|
|
||
| if (vtxs->empty()) { | ||
| if (vtxs.empty()) | ||
| edm::LogWarning("Input") << "Input collection of vertices is empty. Output collection will be empty."; |
There was a problem hiding this comment.
| edm::LogWarning("Input") << "Input collection of vertices is empty. Output collection will be empty."; | |
| edm::LogWarning("PixelVertexInput") << "Input collection of vertices is empty. Output collection will be empty."; |
categories are global and "Input" is a bit too generic if one were to make use of the category without also knowing the module name issuing this warning.
| } | ||
|
|
||
| if (vtxs_trim->empty()) | ||
| edm::LogInfo("Output") << "Output collection is empty."; |
There was a problem hiding this comment.
| edm::LogInfo("Output") << "Output collection is empty."; | |
| edm::LogInfo("PixelVertexOutput") << "Output collection is empty."; |
similar to PixelVertexInput above
cmsbuild
added
code-checks-pending
tests-pending
and removed
code-checks-approved
tests-approved
labels
|
Thanks for your comments, @slava77. I have tried to implement the suggested changes. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33091/21507
|
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-408cb9/13436/summary.html Comparison SummarySummary:
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
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) |
|
+1 |
PR description:
This PR is meant to improve the implementation of the plugin
PixelVertexCollectionTrimmer.This plugin is used at HLT to select a subset of pixel vertices, based on their Sum-Pt2 score.
In its current form, the plugin implicitly assumes that the input collection of vertices is already sorted according to the same sorting criterion used in
PixelVertexCollectionTrimmeritself (the 1st entry of the input collection is assumed to have the highest f.o.m.).The main update in this PR is to remove this assumption on the ordering of the inputs.
While said assumption was valid in the past (e.g. Run-2 HLT), it might not be valid in the future; for example, the pixel vertices in the Patatrack workflow use a sorting criterion which is slightly different from the "legacy" pixel vertices (the trimmer still uses the same sorting criterion as the legacy pixel vertices).
More details can be found in cms-patatrack#605.
As suggested by @AdrianoDee (TRK POG), I'm opening a PR here, so that experts can review it.
On a technical level, these updates would introduce two small changes in the producer:
in the PR, the output collection is sorted according to the sorter in
PixelVertexCollectionTrimmer(currently, the trimming is applied, but there is no re-sorting, and the ordering of the outputs is the same as the inputs);the current impl uses
sumpt2 > minSumPt2_; in the impl in the PR, this is effectively changed tosumpt2 >= minSumPt2_.Here are 3 (possibly alternative) ways to maybe improve things further, for experts to consider:
adding a data member to
reco::Vertexto save the Sum-Pt2 score, such that it would not have to be recomputed a posteriori in other modules (having to make sure different modules use the same sorting criterion);generalizing the implementation of
PVClusterComparer(the legacy sorter, used in the trimmer) such that it can be configured to give the legacy sorting, or the Patatrack sorting, depending on what is needed;making
PixelVertexCollectionTrimmercapable of using different sorting functions, rather than justPVClusterComparer.PR validation:
Private tests to verify that the changes work as intended.
Running the updated plugin on 100 events (Run-3 DY MC with PU) suggests that its timing remains very small (<0.1ms).
If this PR is a backport, please specify the original PR and why you need to backport that PR:
N/A (no backport planned)