add TrackSelectorBy Region and CandidateRegion #29359
Conversation
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29359/14451
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
@mtosi tests can not start without this check passing |
|
The code-checks are being triggered in jenkins. |
|
thanks @slava77 ! I overlooked it :( |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29359/14490
|
|
A new Pull Request was created by @mtosi (mia tosi) for master. It involves the following packages: RecoTracker/FinalTrackSelectors @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| explicit TrackSelectorByCandidateRegion(const edm::ParameterSet& conf) | ||
| : tracksToken_(consumes<reco::TrackCollection>(conf.getParameter<edm::InputTag>("tracks"))), | ||
| beamspotToken_(consumes<reco::BeamSpot>(conf.getParameter<edm::InputTag>("beamspot"))) { | ||
| edm::ParameterSet trackPSet = conf.getParameter<edm::ParameterSet>("TrackPSet"); |
There was a problem hiding this comment.
auto const& trackPSet = getParameterSet("TrackPSet"); should be a bit more effective (no copy)
| throw cms::Exception("Configuration") | ||
| << "TrackSelectorByCandidateRegion::chi2vsPt: pt2 needs to be > 0; is " << pt2_; | ||
|
|
||
| // for (auto const & ir : conf.getParameter<edm::VParameterSet>("regions")) { |
There was a problem hiding this comment.
is this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant
| << "TrackSelectorByCandidateRegion::chi2vsPt: pt2 needs to be > 0; is " << pt2_; | ||
|
|
||
| // for (auto const & ir : conf.getParameter<edm::VParameterSet>("regions")) { | ||
| edm::ParameterSet regionPSet = conf.getParameter<edm::ParameterSet>("RegionPSet"); |
There was a problem hiding this comment.
auto const& regionPSet = conf.getParameterSet("RegionPSet");
There was a problem hiding this comment.
It looks like this part here and the implementation should be simply calling CandidateSeededTrackingRegionsProducer (via ES or directly).
This will eliminate the reimplementation of already existing code.
| mode_ = VERTICES_FIXED; | ||
| else if (mode == "VerticesSigma") | ||
| mode_ = VERTICES_SIGMA; | ||
| // else edm::LogError ("TrackSelectorByCandidateRegion") << "Unknown mode string: " << mode; |
There was a problem hiding this comment.
is this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant
| * either BeamSpotSigma or BeamSpotFixed mode, depending on the positiveness of nSigmaZBeamSpot. | ||
| **/ | ||
|
|
||
| inputCandidateToken_ = mayConsume<reco::CandidateView>(regionPSet.getParameter<edm::InputTag>("input")); |
There was a problem hiding this comment.
it looks like there are no conditions in the ::produce methods limiting access to this product ==> change to consumes
| iEvent.getByToken(tracksToken_, tracksHandle); | ||
|
|
||
| // std::cout << "[TrackSelectorByRegion::produce] tracksHandle.isValid ? " << (tracksHandle.isValid() ? "YEAP" : "NOPE") << std::endl; | ||
| if (tracksHandle.isValid()) { |
There was a problem hiding this comment.
as in TrackSelectorByCandidateRegion: is there some intrinsic limitation in the HLT to miss writing a track collection in the event? If not, to avoid configuration problems it would be more appropriate to not check for validity and let the job fail if it's incorrectly configured
| // std::cout << "tracks: " << tracks.size() << std::endl; | ||
| for (auto trk : tracks) { | ||
| const auto pt = trk.pt(); | ||
| if (pt < minPt_) { | ||
| // std::cout << " KO !!! for pt" << std::endl; |
There was a problem hiding this comment.
is this commented out code needed?
Please remove or add comments inline in the code why the commented out block is relevant
|
|
||
| size_t it = 0; | ||
| // std::cout << "tracks: " << tracks.size() << std::endl; | ||
| for (auto trk : tracks) { |
| // if ( std::abs(trk.vz() - origin[k].z()) > zBound[k] ) { | ||
| // std::cout << "std::abs(trk.vz() - origin[k].z()): " << std::abs(trk.vz() - origin[k].z()) << " zBound: " << zBound[k] << std::endl; | ||
| // std::cout << "std::abs(trk.dz(origin[k])): " << std::abs(trk.dz(origin[k])) << " zBound: " << zBound[k] << std::endl; | ||
| if (std::abs(trk.dz(origin[k])) > zBound[k]) { | ||
| // std::cout << " KO !! for z" << std::endl; | ||
| continue; | ||
| } | ||
| // std::cout << "eta : " << eta << " etaMin[k]: " << etaMin[k] << " etaMax[k]: " << etaMax[k] << std::endl; | ||
| if (eta < etaMin[k]) { | ||
| // std::cout << " KO !!! for eta" << std::endl; | ||
| continue; | ||
| } | ||
| if (eta > etaMax[k]) { | ||
| // std::cout << " KO !!! for eta" << std::endl; | ||
| continue; | ||
| } | ||
| // std::cout << "phi: " << phi << " phi0[k]: " << phi0[k] << " std::abs(reco::deltaPhi(phi,phi0[k])): " << std::abs(reco::deltaPhi(phi,phi0[k])) << " phi0margin: " << phi0margin[k] << std::endl; | ||
| if (std::abs(reco::deltaPhi(phi, phi0[k])) > phi0margin[k] * 1.1) { | ||
| // std::cout << " KO !!! for phi" << std::endl; |
There was a problem hiding this comment.
is this commented out code needed?
If debugging is still expected, please add it with LogTrace or LogDebug or similar so that necessary parts can be checked without rewriting the code
| continue; | ||
| } | ||
| // std::cout << "phi: " << phi << " phi0[k]: " << phi0[k] << " std::abs(reco::deltaPhi(phi,phi0[k])): " << std::abs(reco::deltaPhi(phi,phi0[k])) << " phi0margin: " << phi0margin[k] << std::endl; | ||
| if (std::abs(reco::deltaPhi(phi, phi0[k])) > phi0margin[k] * 1.1) { |
There was a problem hiding this comment.
why 1.1? Is this an external additional scaling? If so, perhaps it should be made configurable.
|
@mtosi |
|
ciao, thanks for this reminder, I slowly progress :( |
|
Kind reminder @mtosi |
|
Kind reminder @mtosi . We will close CMSSW_11_1_0_pre7 tomorrow. |
|
thanks @silviodonato, I'm afraid I need some extra time :( |
|
there is a merge conflict here now |
|
thanks @slava77 |
|
|
-1 just taking this off our list for now, since updates are needed anyways |
|
Please let me know as soon as you want to open back this PR. |
PR description:
instead of reconstruct tracks in different regions, while having already tracks reconstructed globally,
one can select the subset of tracks based on the ROI a-posteriori
these 2 plugins are meant to be used mainly at HLT,
where for instance pixel tracks are reconstructed globally
and then they can be selected around muons, electrons, taus, jets
for the different needs, like seeding tracks for the first iteration in the isolation and b-tagging or reconstructing PV, etc