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
Changes to improve GE0 GEMSegments #31480
Changes from all commits
33a7e8d
fec279d
a906a51
d073f01
2aad7cd
75fddcf
aaa8cf0
821dff2
34e4875
20ae4b2
4c23125
0f69ba0
593bea0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,8 +40,8 @@ class GEMSegment final : public RecSegment { | |
const LocalVector& direction, | ||
const AlgebraicSymMatrix& errors, | ||
double chi2, | ||
double time, | ||
double timeErr); | ||
float bx, | ||
float deltaPhi); | ||
|
||
/// Destructor | ||
~GEMSegment() override; | ||
|
@@ -82,9 +82,10 @@ class GEMSegment final : public RecSegment { | |
|
||
GEMDetId gemDetId() const { return geographicalId(); } | ||
|
||
float time() const { return theTimeValue; } | ||
float timeErr() const { return theTimeUncrt; } | ||
float bunchX() const { return theBX; } | ||
|
||
float deltaPhi() const { return theDeltaPhi; } | ||
|
||
void print() const; | ||
|
||
private: | ||
|
@@ -93,9 +94,8 @@ class GEMSegment final : public RecSegment { | |
LocalVector theLocalDirection; // in chamber frame - the GeomDet local coordinate system | ||
AlgebraicSymMatrix theCovMatrix; // the covariance matrix | ||
double theChi2; // the Chi squared of the segment fit | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
double theTimeValue; // the best time estimate of the segment | ||
double theTimeUncrt; // the uncertainty on the time estimation | ||
float theBX; // the bunch crossing | ||
float theDeltaPhi; // Difference in segment phi position: outer layer - inner lay | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this kind of change may require an i/o rule to ensure backward compatibility. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kpedro88 Thanks for the comments. For here, based on comparing some other classes, I added a ioread for older versions of GEMSegment without theDeltaPhi. Do I need to also do something for the dropped timing information, or these will just be ignored? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropped information will just be ignored. |
||
}; | ||
|
||
std::ostream& operator<<(std::ostream& os, const GEMSegment& seg); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,3 +37,28 @@ const GEMChamber* GEMSuperChamber::chamber(int isl) const { | |
} | ||
return nullptr; | ||
} | ||
|
||
float GEMSuperChamber::computeDeltaPhi(const LocalPoint& position, const LocalVector& direction) const { | ||
auto extrap = [](const LocalPoint& point, const LocalVector& dir, double extZ) -> LocalPoint { | ||
if (dir.z() == 0) | ||
return LocalPoint(0.f, 0.f, extZ); | ||
double extX = point.x() + extZ * dir.x() / dir.z(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please prevent division by zero by checking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @watson-ij Please address this issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you look at the latest version of the code, there's a check for z==0 above this line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, must be going blind. ;-). Looks good. |
||
double extY = point.y() + extZ * dir.y() / dir.z(); | ||
return LocalPoint(extX, extY, extZ); | ||
}; | ||
if (nChambers() < 2) { | ||
return 0.f; | ||
} | ||
|
||
const float beginOfChamber = chamber(1)->position().z(); | ||
const float centerOfChamber = this->position().z(); | ||
const float endOfChamber = chamber(nChambers())->position().z(); | ||
|
||
LocalPoint projHigh = | ||
extrap(position, direction, (centerOfChamber < 0 ? -1.0 : 1.0) * (endOfChamber - centerOfChamber)); | ||
LocalPoint projLow = | ||
extrap(position, direction, (centerOfChamber < 0 ? -1.0 : 1.0) * (beginOfChamber - centerOfChamber)); | ||
auto globLow = toGlobal(projLow); | ||
auto globHigh = toGlobal(projHigh); | ||
return globHigh.phi() - globLow.phi(); //Geom::phi automatically normalizes to [-pi, pi] | ||
} |
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.
Now you pass everything as
float
: is it still necessary to pass chi2 as adouble
?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.
@perrotta I think this was originally to match the interface of CSCSegment, which also defines theChi2 as double. Is it prefered to just switch to float here?
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.
If using a double is any useful in the code it can stay as such. If not, better to reduce precision (and also make it uniform with the other similar parameters)
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.
In GEMCSCSegment, we combine the chi2 from CSC and GEM, so I would have thought it would be better to keep it the same precision in this case. @jshlee do you have an opinion?