Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Changes to improve GE0 GEMSegments #31480

Merged
merged 13 commits into from Sep 25, 2020
Merged

Changes to improve GE0 GEMSegments #31480

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
33a7e8d
Copy (and get working) ME0 RU segment algo as GE0
watson-ij Sep 3, 2020
fec279d
Reverse the changes to the GEMSegmentBuilder
watson-ij Sep 16, 2020
a906a51
Add deltaPhi to GEMSegment, for GE0 merging (i.e. merge ME0Segment info)
watson-ij Sep 16, 2020
d073f01
Code format
watson-ij Sep 16, 2020
2aad7cd
Add ioread for missing theDeltaPhi for data produced in older versions
watson-ij Sep 17, 2020
75fddcf
Prevent zero division in GEMSuperChamber::computeDeltaPhi
watson-ij Sep 21, 2020
aaa8cf0
Apply cleanup from kpedro and perrotta
watson-ij Sep 21, 2020
821dff2
Code format
watson-ij Sep 21, 2020
34e4875
Update include guard per kpedro's request
watson-ij Sep 22, 2020
20ae4b2
Change for loops to use ranges
watson-ij Sep 22, 2020
4c23125
Missed one -> to . change
watson-ij Sep 22, 2020
0f69ba0
Change type of GEMSegment::theChi2 from double to float
watson-ij Sep 22, 2020
593bea0
Revert "Change type of GEMSegment::theChi2 from double to float"
watson-ij Sep 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions DataFormats/GEMRecHit/interface/GEMSegment.h
View file
Open in desktop
Expand Up @@ -40,8 +40,8 @@ class GEMSegment final : public RecSegment {
const LocalVector& direction,
const AlgebraicSymMatrix& errors,
double chi2,
Copy link
Contributor

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 a double?

Copy link
Contributor Author

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?

Copy link
Contributor

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)

Copy link
Contributor Author

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?

double time,
double timeErr);
float bx,
float deltaPhi);

/// Destructor
~GEMSegment() override;
Expand Down Expand Up @@ -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:
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Expand Down
24 changes: 10 additions & 14 deletions DataFormats/GEMRecHit/src/GEMSegment.cc
View file
Open in desktop
Expand Up @@ -36,9 +36,8 @@ GEMSegment::GEMSegment(const std::vector<const GEMRecHit*>& proto_segment,
theLocalDirection(direction),
theCovMatrix(errors),
theChi2(chi2) {
theTimeValue = 0.0;
theTimeUncrt = 0.0;
theBX = -10.0;
theDeltaPhi = -10.0;
for (unsigned int i = 0; i < proto_segment.size(); ++i)
theGEMRecHits.push_back(*proto_segment[i]);
}
Expand All @@ -53,10 +52,9 @@ GEMSegment::GEMSegment(const std::vector<const GEMRecHit*>& proto_segment,
theOrigin(origin),
theLocalDirection(direction),
theCovMatrix(errors),
theChi2(chi2) {
theTimeValue = 0.0;
theTimeUncrt = 0.0;
theBX = bx;
theChi2(chi2),
theBX(bx) {
theDeltaPhi = -10.0;
for (unsigned int i = 0; i < proto_segment.size(); ++i)
theGEMRecHits.push_back(*proto_segment[i]);
}
Expand All @@ -66,16 +64,15 @@ GEMSegment::GEMSegment(const std::vector<const GEMRecHit*>& proto_segment,
const LocalVector& direction,
const AlgebraicSymMatrix& errors,
double chi2,
double time,
double timeErr)
float bx,
float deltaPhi)
: RecSegment(buildDetId(proto_segment.front()->gemId())),
theOrigin(origin),
theLocalDirection(direction),
theCovMatrix(errors),
theChi2(chi2) {
theTimeValue = time;
theTimeUncrt = timeErr;
theBX = -10.0;
theChi2(chi2),
theBX(bx),
theDeltaPhi(deltaPhi) {
for (unsigned int i = 0; i < proto_segment.size(); ++i)
theGEMRecHits.push_back(*proto_segment[i]);
}
Expand Down Expand Up @@ -136,8 +133,7 @@ std::ostream& operator<<(std::ostream& os, const GEMSegment& seg) {
<< " dir = " << seg.localDirection() << " dirErr = (" << sqrt(seg.localDirectionError().xx()) << ","
<< sqrt(seg.localDirectionError().yy()) << "0,)\n"
<< " chi2/ndf = " << ((seg.degreesOfFreedom() != 0.) ? seg.chi2() / double(seg.degreesOfFreedom()) : 0)
<< " #rechits = " << seg.specificRecHits().size() << " bx = " << seg.bunchX() << " time = " << seg.time()
<< " +/- " << seg.timeErr() << " ns";
<< " #rechits = " << seg.specificRecHits().size() << " bx = " << seg.bunchX() << " deltaPhi = " << seg.deltaPhi();

return os;
}
6 changes: 5 additions & 1 deletion DataFormats/GEMRecHit/src/classes_def.xml
View file
Open in desktop
Expand Up @@ -36,8 +36,12 @@
<class name="edm::Wrapper<edm::RangeMap<CSCDetId,edm::OwnVector<GEMCSCSegment,edm::ClonePolicy<GEMCSCSegment> >,edm::ClonePolicy<GEMCSCSegment> > >" splitLevel="0"/>
<class name="GEMCSCSegmentRef" splitLevel="0"/>

<class name="GEMSegment" ClassVersion="3">
<class name="GEMSegment" ClassVersion="4">
<version ClassVersion="4" checksum="1506093327"/>
<version ClassVersion="3" checksum="1513163288"/>
<ioread sourceClass="GEMSegment" version="[-3]" targetClass="GEMSegment" source="" target="theDeltaPhi">
<![CDATA[ theDeltaPhi = -10.; ]]>
</ioread>
</class>
<class name="std::vector<GEMSegment*>" splitLevel="0"/>
<class name="edm::OwnVector<GEMSegment,edm::ClonePolicy<GEMSegment> >" splitLevel="0"/>
Expand Down
2 changes: 2 additions & 0 deletions Geometry/GEMGeometry/interface/GEMSuperChamber.h
View file
Open in desktop
Expand Up @@ -56,6 +56,8 @@ class GEMSuperChamber : public GeomDet {
/// Return numbers of chambers
int nChambers() const;

float computeDeltaPhi(const LocalPoint& position, const LocalVector& direction) const;

private:
GEMDetId detId_;

Expand Down
25 changes: 25 additions & 0 deletions Geometry/GEMGeometry/src/GEMSuperChamber.cc
View file
Open in desktop
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please prevent division by zero by checking dir.z().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@watson-ij Please address this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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]
}