Selector by region #31055
Selector by region #31055
Conversation
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31055/17564
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
| public: | ||
| explicit TrackSelectorByRegion(const edm::ParameterSet& conf) | ||
| : tracksToken_(consumes<reco::TrackCollection>(conf.getParameter<edm::InputTag>("tracks"))) { | ||
| inputTrkRegionToken_ = consumes<edm::OwnVector<TrackingRegion>>(conf.getParameter<edm::InputTag>("inputROI")); |
There was a problem hiding this comment.
I would suggest to rename "inputROI" to "regions"
| explicit TrackSelectorByRegion(const edm::ParameterSet& conf) | ||
| : tracksToken_(consumes<reco::TrackCollection>(conf.getParameter<edm::InputTag>("tracks"))) { | ||
| inputTrkRegionToken_ = consumes<edm::OwnVector<TrackingRegion>>(conf.getParameter<edm::InputTag>("inputROI")); | ||
| produce_collection = conf.getParameter<bool>("produceTrackCollection"); |
There was a problem hiding this comment.
as briefly discussed today, it might be useful to have an other boolean parameter for handling the mask collection
|
|
||
| auto tracksHandle = iEvent.getHandle(tracksToken_); | ||
|
|
||
| if (tracksHandle.isValid()) { |
There was a problem hiding this comment.
I would remove the isValid() check; otherwise this module sill silently ignore any errors in the configuration.
If the handle is not valid it's OK to get a run time error.
RecoTracker/FinalTrackSelectors/plugins/TrackSelectorByRegion.cc
Outdated
Show resolved
Hide resolved
| const auto& regions = *regionsHandle; | ||
|
|
||
| for (const auto& tmp : regions) | ||
| if (const auto* roi = dynamic_cast<const TrackingRegion*>(&tmp)) { |
There was a problem hiding this comment.
the dynamic_cast should not be needed any longer, because you have introduced the function in the TrackingRegion itself, now
(each region has its own implementation of the function, and it will be automatically called)
| if (tracksHandle.isValid()) { | ||
| const auto tracks = *tracksHandle; | ||
| mask->assign(tracks.size(), false); | ||
| if (regionsHandle.isValid()) { |
There was a problem hiding this comment.
I would remove this check - see above
| if (regionsHandle.isValid()) { | ||
| const auto& regions = *regionsHandle; | ||
|
|
||
| for (const auto& tmp : regions) |
There was a problem hiding this comment.
I would rename tmp to region
RecoTracker/FinalTrackSelectors/plugins/TrackSelectorByRegion.cc
Outdated
Show resolved
Hide resolved
RecoTracker/FinalTrackSelectors/plugins/TrackSelectorByRegion.cc
Outdated
Show resolved
Hide resolved
|
|
||
| for (const auto& tmp : regions) | ||
| if (const auto* roi = dynamic_cast<const TrackingRegion*>(&tmp)) { | ||
| auto amask = roi->checkTracks(tracks); |
There was a problem hiding this comment.
can be auto const& ... to avoid a copy (even though the compiler should optimise it away here)
RecoTracker/TkTrackingRegions/interface/RectangularEtaPhiTrackingRegion.h
Outdated
Show resolved
Hide resolved
| for (const auto& tmp : regions) | ||
| if (const auto* roi = dynamic_cast<const TrackingRegion*>(&tmp)) { | ||
| auto amask = roi->checkTracks(tracks); | ||
| for (size_t it = 0; it < amask.size(); it++) { |
There was a problem hiding this comment.
I would suggest to use i for the counter instead of it; it looks like an iterator :-)
| for (size_t it = 0; it < mask->size(); it++) | ||
| if (mask->at(it)) | ||
| output_tracks->push_back(tracks[it]); |
There was a problem hiding this comment.
these should be run only if(produce_collection) { ... }
| #include <vector> | ||
| #include <memory> | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
Why do you put everything in an anonymous namespace ?
It's probably correct, but I don't think I have seen it used frequently (or at all) ?
| if (!etaRange().inside(track.eta())) { | ||
| continue; | ||
| } | ||
| if (std::abs(reco::deltaPhi(track.phi(), phiDirection())) > phiMargin().right()) { |
There was a problem hiding this comment.
also, since phiMargin() could potentially be asymmetric, should this make explicit checks for direction - margin and direction + margin ?
|
The code-checks are being triggered in jenkins. |
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31055/17712
|
|
@cmsbuild please test Thanks for the update! I'll just run a quick build and then it should be fine from my side. |
|
The tests are being triggered in jenkins.
|
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1
|
cmsbuild
added
fully-signed
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:
Module that can take in a track collection and a tracking region , then produce the subset track collection with tracks that are compliant to the constraints in tracking region
PR validation:
Results for CMSSW_11_2_0_pre2 , RelValZMM_14
A brief overview :
RvR := Regionally reconstructed tracks passing the selector
GvR := Global pixel tracks passing the selector
SUMMARY for TrackingRegion : hltIterL3MuonPixelTracksTrackingRegions, TrackCollection : hltIterL3MuonPixelTracks
SUMMARY for TrackingRegion : hltPixelTracksTrackingRegionsL3MuonNoVtx, TrackCollection : hltPixelTracksL3MuonNoVtx
Plots for the same are attached alongside
