New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GEM uniform angular strip topology #31640
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31640/18725
|
A new Pull Request was created by @hyunyong for master. It involves the following packages: Fireworks/Geometry @perrotta, @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @alja, @kpedro88, @slava77, @jpata can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@hyunyong Shouldn't the radial |
@dildick GEM strip topology was based on the trapezoidal strip topology and some functions are not comparable with the radial strip topology. |
+1 |
The tests are being triggered in jenkins.
|
@@ -0,0 +1,73 @@ | |||
#ifndef _GEM_STRIP_TOPOLOGY_H_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use correct include guard format: subsystem_package_filename
float theCentreToIntersection; // distance centre of detector face to intersection of edge strips (projected) | ||
float thePhiOfOneEdge; // local 'phi' of one edge of plane of strips (I choose it negative!) | ||
float theYAxisOrientation; // 1 means y axis going from smaller to larger side, -1 means opposite direction | ||
float yCentre; // Non-zero if offset in local y between midpoint of detector (strip plane) extent and local origin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
member variables need to have consistent naming scheme, i.e. theYCentre
(or even better, to correspond to base class, remove "the", start with lowercase, and end with underscore)
thePhiOfOneEdge = -(0.5 * theNumberOfStrips) * theAngularWidth * theYAxisOrientation; | ||
yCentre = 0; | ||
#ifdef VERBOSE | ||
cout << "Constructing GEMStripTopology with" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cout is not allowed, use MessageLogger (everywhere)
@@ -13,14 +13,17 @@ GEMEtaPartitionSpecs::GEMEtaPartitionSpecs(SubDetector rss, const std::string& n | |||
float r0 = h * (B + b) / (B - b); | |||
float striplength = h * 2; | |||
float strips = _p[3]; | |||
float pitch = (b + B) / strips; | |||
float dphi = _p[5] * TMath::Pi() / 180.; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use https://github.com/cms-sw/cmssw/blob/master/DataFormats/Math/interface/angle_units.h to convert radians to degrees, avoid TMath
int nstrip = static_cast<int>(strips); | ||
_top = new TrapezoidalStripTopology(nstrip, pitch, striplength, r0); | ||
_top = new GEMStripTopology(nstrip, phiPitch, striplength, r0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly outside the scope of this PR, but it would be better to use smart pointers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this PR as your comments, but I can't understand how to use smart pointers for this.
-1 Tested at: 597c95a CMSSW: CMSSW_11_2_X_2020-10-01-1100 I found follow errors while testing this PR Failed tests: RelVals
When I ran the RelVals I found an error in the following workflows: runTheMatrix-results/23434.999_TTbar_14TeV+2026D49PU_PMXS1S2PR+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU/step3_TTbar_14TeV+2026D49PU_PMXS1S2PR+TTbar_14TeV_TuneCP5_GenSimHLBeamSpot14+PREMIX_PremixHLBeamSpot14PU+DigiTriggerPU+RecoGlobalPU+HARVESTGlobalPU.log |
Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped) |
@cmsbuild please test |
The tests are being triggered in jenkins.
|
+1 |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
@@ -72,8 +72,7 @@ | |||
#include <DataFormats/CSCRecHit/interface/CSCSegmentCollection.h> | |||
#include <Geometry/CommonDetUnit/interface/GeomDet.h> | |||
#include <Geometry/Records/interface/MuonGeometryRecord.h> | |||
#include <Geometry/CommonTopologies/interface/RectangularStripTopology.h> | |||
#include <Geometry/CommonTopologies/interface/TrapezoidalStripTopology.h> | |||
#include <Geometry/CommonTopologies/interface/GEMStripTopology.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replacement is in line with the overall migration performed in this PR,
On the other hand, this include doesn't even seem to be needed at all in this test analyzer (as they were not need also the previous two which are being replaced).
Not really an issue, anyhow...
+1
|
+1 |
+upgrade |
merge |
PR description:
PR validation:
https://indico.cern.ch/event/959823/contributions/4034596/attachments/2114039/3556405/GEMStripTopo.pdf
@jshlee @watson-ij @bsunanda