Update of PointSeededTrackingRegionsProducer to work in CMSSW 9 #18817
Conversation
|
A new Pull Request was created by @JanFSchulte (Jan-Frederik Schulte) for master. It involves the following packages: RecoTracker/TkTrackingRegions @perrotta, @cmsbuild, @slava77, @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. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
| int m_maxNVertices; | ||
|
|
||
| std::vector<edm::ParameterSet> points; |
There was a problem hiding this comment.
hi @JanFSchulte - please store a vector of eta and phi values instead of the ParameterSets. This will avoid many string comparisons (and save space)
|
Pull request #18817 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again. |
| @@ -68,7 +68,8 @@ class PointSeededTrackingRegionsProducer : public TrackingRegionProducer | |||
| edm::ParameterSet points = regPSet.getParameter<edm::ParameterSet>("points"); | |||
| etaPoints = points.getParameter<std::vector<double>>("eta"); | |||
| phiPoints = points.getParameter<std::vector<double>>("phi"); | |||
| if (!(etaPoints.size() == phiPoints.size())) edm::LogError ("PointSeededTrackingRegionsProducer")<<"same number of eta and phi coordinates must be specified"; | |||
| //if (!(etaPoints.size() == phiPoints.size())) edm::Exception ("PointSeededTrackingRegionsProducer")<<"same number of eta and phi coordinates must be specified"; | |||
There was a problem hiding this comment.
Remove commented out line
| vDefaults1.addParameter<double>("phi", 0.0); | ||
| vDefaults.push_back(vDefaults1); | ||
| desc.addVPSet("points", descPoints,vDefaults); | ||
| */ |
There was a problem hiding this comment.
Remove commented out lines
| @@ -227,6 +238,7 @@ class PointSeededTrackingRegionsProducer : public TrackingRegionProducer | |||
| ++n_regions; | |||
| } | |||
| } | |||
| //std::cout<<"n_seeded_regions = "<<n_regions<<std::endl; | |||
There was a problem hiding this comment.
Remove commented out line
|
Pull request #18817 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again. |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
| double x = std::cos(phiPoints[i]); | ||
| double y = std::sin(phiPoints[i]); | ||
| double theta = 2*std::atan(std::exp(-etaPoints[i])); | ||
| double z = 1./std::tan(theta); |
There was a problem hiding this comment.
how about
z = ( 1−exp(-η)² ) / (2 * exp(-η))
?
No idea whether it is more accurate and/or faster or neither...
|
Comparison is ready Comparison Summary:
|
|
+1 |
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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
| // basic inputsi | ||
| edm::ParameterSet points = regPSet.getParameter<edm::ParameterSet>("points"); | ||
| etaPoints = points.getParameter<std::vector<double>>("eta"); | ||
| phiPoints = points.getParameter<std::vector<double>>("phi"); |
There was a problem hiding this comment.
I liked the previous form of points more (vector of PSets with eta and phi attributes instead of two vectors of doubles). That doesn't prevent storing the eta and phi values as vector<double> members.
OTOH I don't want to delay this PR any further, and maybe after its integration it's not worth to change the configuration interface anymore (before next year at least).
There was a problem hiding this comment.
I see your point; I agree a vector of PSet makes more sense, but indeed it should be converted into a vector of doubles, or better into a vector of "coordinates", defined for example as
struct EtaPhi {
double eta;
double phi;
}
There was a problem hiding this comment.
In fact, the convertsion from eta/phi to x/y/z culd also be done at construction time
There was a problem hiding this comment.
I agree on both points
|
+1 |
The PointSeededTrackingRegionsProducer.h is updated with a correct fillDescription method to work in CMSSW 91X. Also, the possibility of using multiple eta-phi points to seed regions is added. Need for mitigation of the effect of non-functioning modules in the new pixel detector at HLT.