Add minPhi+maxPhi to RecoTrackSelectorBase and TrackingParticleSelector #19029
Conversation
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: CommonTools/RecoAlgos @perrotta, @cmsbuild, @civanch, @monttj, @silviodonato, @fwyzard, @mdhildreth, @dmitrijus, @Martin-Grunewald, @slava77, @vanbesien, @davidlange6 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. |
| for producer in producers_by_type(process, "RecoTrackRefSelector"): | ||
| if not hasattr(producer, "minPhi"): | ||
| producer.minPhi = cms.double(-3.2) | ||
| producer.maxPhi = cms.double(3.2) |
There was a problem hiding this comment.
This (adding two top-level parameters to a plugin) is better done by a fillDescriptions().
There was a problem hiding this comment.
I know... But as far as I can tell, the object selector templates do not offer fillDescriptions() at the moment (and I don't have time now to try that).
https://github.com/cms-sw/cmssw/blob/master/CommonTools/RecoAlgos/plugins/RecoTrackRefSelector.cc#L19
https://github.com/cms-sw/cmssw/blob/master/CommonTools/UtilAlgos/interface/ObjectSelectorStreamProducer.h#L13
https://github.com/cms-sw/cmssw/blob/master/CommonTools/UtilAlgos/interface/ObjectSelectorProducer.h
https://github.com/cms-sw/cmssw/blob/master/CommonTools/UtilAlgos/interface/StoreManagerTrait.h#L92-L103
| @@ -19,6 +19,8 @@ class RecoTrackSelectorBase { | |||
| ptMin_(cfg.getParameter<double>("ptMin")), | |||
| minRapidity_(cfg.getParameter<double>("minRapidity")), | |||
| maxRapidity_(cfg.getParameter<double>("maxRapidity")), | |||
| meanPhi_((cfg.getParameter<double>("minPhi")+cfg.getParameter<double>("maxPhi"))/2.), | |||
| rangePhi_((cfg.getParameter<double>("maxPhi")-cfg.getParameter<double>("minPhi"))/2.), | |||
There was a problem hiding this comment.
this does not seem to work correctly for something like
minPhi = 4.712 # 3/2 π
maxPhi = 1.571 # 1/2 π
There was a problem hiding this comment.
@fwyzard I agree. The idea is that minPhi should be smaller than maxPhi (I can certainly add a check for that), and in case one needs to specify a range containing π (which I understood was your point), the maxPhi is set to a value larger than π (I could also add checks that minPhi must be smaller than π, and maxPhi must be larger than -π).
The range from 3π/2 to π/2 (going through π) can be specified by reversing your minPhi and maxPhi
minPhi = 1.571 # 1/2 π
maxPhi = 4.712 # 3/2 π
and then it should work (selecting tracks whose phi is in range 3.1415 +- 1.5705).
I can also add some documentation to the classes to clarify the intended usage (as it was a bit tricky and took me some time end up with this implementation).
There was a problem hiding this comment.
Mhm, no, my point was really to select tracks whose phi is around 0.
This
minPhi = 4.712 # 3/2 π
maxPhi = 1.571 # 1/2 π
should be equivalent to
minPhi = -1.571 # -1/2 π == 3/2 π
maxPhi = 1.571 # 1/2 π
There was a problem hiding this comment.
Ah, ok. Would the checks I mentioned above (minPhi < maxPhi, minPhi < π, maxPhi > -π) be enough (i.e. the first case would give an error), or the input to be normalized/sanitized first?
To me a configuration would be easier to understand (by me) if minPhi < maxPhi would be enforced despite of phi wrapping around, but if the opposite is desired, I'm fine with that as well.
There was a problem hiding this comment.
One possibility would be to configure directly with meanPhi and rangePhi (or maybe call it deltaPhi).
There was a problem hiding this comment.
Yes, I think these checks (minPhi < maxPhi, minPhi < π, maxPhi > -π) should be enough to cover all possibilities.
You still leave the possibility of specifying some ranges in multiple ways (e.g. 3/4π..9/4π vs -5/4π..1/4π) but that's not a problem if the code handles both pair of values consistently.
There was a problem hiding this comment.
@fwyzard Indeed those are still possible, but the code should deal with them properly (thanks to reco::deltaPhi() reducing the range of the difference wrt. meanPhi to -π..π).
|
Comparison job queued. |
|
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
|
+1 |
cmsbuild
added
comparison-pending
simulation-pending
tests-pending
and removed
comparison-available
simulation-approved
tests-approved
labels
|
Pull request #19029 was updated. @perrotta, @cmsbuild, @civanch, @monttj, @vazzolini, @silviodonato, @fwyzard, @mdhildreth, @dmitrijus, @Martin-Grunewald, @kmaeshima, @slava77, @vanbesien, @davidlange6 can you please check and sign again. |
|
@cmsbuild, please test Rebased on top of CMSSW_9_3_X_2017-06-26-2300 (thanks @Martin-Grunewald for the notification). |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
merge |
This PR adds ability to select tracks/TrackingParticles in phi slices to
RecoTrackSelectorBase/TrackingParticleSelector.Tested in 910pre3 and CMSSW_9_2_X_2017-05-30-1100 (rebased on top of
CMSSW_9_2_X_2017-06-13-2300CMSSW_9_2_X_2017-06-18-2300CMSSW_9_2_X_2017-06-25-2300CMSSW_9_3_X_2017-06-26-2300). No changes expected.@rovere @VinInn