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

ngrenz: testing of stereo optimization #10963

Merged
merged 7 commits into from Sep 2, 2015
Merged

ngrenz: testing of stereo optimization #10963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
95334b3
reduce calculation in DataFormats + avoid opject creation in SimTracker
ngrenz Aug 18, 2015
01f74b5
small change
ngrenz Aug 18, 2015
7cec228
inline
ngrenz Aug 27, 2015
b092a27
test of r-phi instead of stereo and glued
ngrenz Aug 27, 2015
e41f093
breaks
ngrenz Aug 28, 2015
6d24341
set geometry at the beginning
ngrenz Aug 28, 2015
5dda346
space
ngrenz Aug 28, 2015
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
92 changes: 53 additions & 39 deletions DataFormats/SiStripDetId/interface/SiStripDetId.h
View file
Open in desktop
Expand Up @@ -112,65 +112,79 @@ class SiStripDetId : public DetId {
// ---------- inline methods ----------
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@civanch sorry, do you refer to this comment

The functions have the same number of returns as in the previous program version
and the inline expression is never used.

I had thought to change the SiStripDetId::stereo() function to

inline uint32_t SiStripDetId::stereo() const { return ( ((id_>>sterStartBit_ ) & sterMask_ ) == 1 ) ? 1:0; }

but I am not sure whether the readability would suffer

Copy link
Contributor

Choose a reason for hiding this comment

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

@ngrenz , yes, it is the most effective c++ but I agree the readability is not the best. It is possible to have something like:

inline uit32_t SiStripDetId::stereo const
{
uit32_t k = 0;
if(1 == ((id_>>sterStartBit_ ) & sterMask_ )) { k = 1; }
return k;
}
similar solutions may be applied in other inlined methods from this header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will rewrite the function.

Do I need the inline expression?
Or rather I do not understand why there is the comment

 // ---------- inline methods ----------

but no inline expressions, in the original code

Copy link
Contributor

Choose a reason for hiding this comment

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

If implementation of a method is inside header file it is assumed to be inlined even without a declaration.

Copy link
Contributor

Choose a reason for hiding this comment

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

i find
return 1 == (id_>>sterStartBit_ ) & sterMask_ ) ? 1:0;

very clear and obvious


SiStripDetId::SubDetector SiStripDetId::subDetector() const {
if( det() == DetId::Tracker &&
subdetId() == static_cast<int>(SiStripDetId::TIB) ) {
return SiStripDetId::TIB;
} else if ( det() == DetId::Tracker &&
subdetId() == static_cast<int>(SiStripDetId::TID) ) {
return SiStripDetId::TID;
} else if ( det() == DetId::Tracker &&
subdetId() == static_cast<int>(SiStripDetId::TOB) ) {
return SiStripDetId::TOB;
} else if ( det() == DetId::Tracker &&
subdetId() == static_cast<int>(SiStripDetId::TEC) ) {
return SiStripDetId::TEC;
SiStripDetId::SubDetector result;
if ( det() == DetId::Tracker) {
if ( subdetId() == static_cast<int>(SiStripDetId::TEC) ) {
result = SiStripDetId::TEC;
} else if ( subdetId() == static_cast<int>(SiStripDetId::TID) ) {
result = SiStripDetId::TID;
} else if ( subdetId() == static_cast<int>(SiStripDetId::TOB) ) {
result = SiStripDetId::TOB;
} else if ( subdetId() == static_cast<int>(SiStripDetId::TIB) ) {
result = SiStripDetId::TIB;
} else {
result = SiStripDetId::UNKNOWN;
}
} else {
return SiStripDetId::UNKNOWN;
result = SiStripDetId::UNKNOWN;
}
return result;
}

SiStripDetId::ModuleGeometry SiStripDetId::moduleGeometry() const {
SiStripDetId::ModuleGeometry geometry = UNKNOWNGEOMETRY;
switch(subDetector()) {
case TIB: return int((id_>>layerStartBit_) & layerMask_)<3? IB1 : IB2;
case TOB: return int((id_>>layerStartBit_) & layerMask_)<5? OB2 : OB1;
case TIB: geometry = int((id_>>layerStartBit_) & layerMask_)<3? IB1 : IB2;
break;
case TOB: geometry = int((id_>>layerStartBit_) & layerMask_)<5? OB2 : OB1;
break;
case TID: switch ((id_>>ringStartBitTID_) & ringMaskTID_) {
case 1: return W1A;
case 2: return W2A;
case 3: return W3A;
case 1: geometry = W1A;
Copy link
Contributor

Choose a reason for hiding this comment

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

you are missing "break;" after each assignment (otherwise everything below a selected "case" is executed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

break;
case 2: geometry = W2A;
break;
case 3: geometry = W3A;
break;
}
break;
case TEC: switch ((id_>>ringStartBitTEC_) & ringMaskTEC_) {
case 1: return W1B;
case 2: return W2B;
case 3: return W3B;
case 4: return W4;
case 5: return W5;
case 6: return W6;
case 7: return W7;
case 1: geometry = W1B;
break;
case 2: geometry = W2B;
break;
case 3: geometry = W3B;
break;
case 4: geometry = W4;
break;
case 5: geometry = W5;
break;
case 6: geometry = W6;
break;
case 7: geometry = W7;
break;
}
case UNKNOWN: default: return UNKNOWNGEOMETRY;
case UNKNOWN: default:;
}
return geometry;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The compiler is complaining that if subDetector() is either TID or TEC and the next level switch statement doesn't match any of the next level case statements then geometry is not set. The easiest solution would be to just set SiStripDetId::ModuleGeometry geometry=UNKNOWNGEOMETRY; at the very beginning.


uint32_t SiStripDetId::glued() const {
if ( ((id_>>sterStartBit_) & sterMask_ ) == 1 ) {
return ( id_ - 1 );
} else if ( ((id_>>sterStartBit_) & sterMask_ ) == 2 ) {
return ( id_ - 2 );
} else { return 0; }
uint32_t testId = (id_>>sterStartBit_) & sterMask_;
return ( testId == 0 ) ? 0 : (id_ - testId);
}

uint32_t SiStripDetId::stereo() const {
if ( ((id_>>sterStartBit_ ) & sterMask_ ) == 1 ) {
return ( (id_>>sterStartBit_) & sterMask_ );
} else { return 0; }
return ( ((id_>>sterStartBit_) & sterMask_) == 1 ) ? 1 : 0;
}

uint32_t SiStripDetId::partnerDetId() const {
if ( ((id_>>sterStartBit_) & sterMask_ ) == 1 ) {
return ( id_ + 1 );
} else if ( ((id_>>sterStartBit_) & sterMask_ ) == 2 ) {
return ( id_ - 1 );
} else { return 0; }
uint32_t testId = (id_>>sterStartBit_) & sterMask_;
if ( testId == 1 ) {
testId = id_ + 1;
} else if ( testId == 2 ) {
testId = id_ - 1;
} else { testId = 0; }
return testId;
}

double SiStripDetId::stripLength() const {
Expand Down
44 changes: 12 additions & 32 deletions DataFormats/TrackerCommon/interface/TrackerTopology.h
View file
Open in desktop
Expand Up @@ -265,59 +265,39 @@ class TrackerTopology {

//these are clones of the old SiStripDetId
uint32_t tobStereo(const DetId &id) const {
if ( ((id.rawId() >>tobVals_.sterStartBit_ ) & tobVals_.sterMask_ ) == 1 ) {
return ( (id.rawId()>>tobVals_.sterStartBit_) & tobVals_.sterMask_ );
} else { return 0; }
return ( ((id.rawId() >>tobVals_.sterStartBit_ ) & tobVals_.sterMask_ ) == 1 ) ? 1 : 0;
}

uint32_t tibStereo(const DetId &id) const {
if ( ((id.rawId() >>tibVals_.sterStartBit_ ) & tibVals_.sterMask_ ) == 1 ) {
return ( (id.rawId()>>tibVals_.sterStartBit_) & tibVals_.sterMask_ );
} else { return 0; }
return ( ((id.rawId() >>tibVals_.sterStartBit_ ) & tibVals_.sterMask_ ) == 1 ) ? 1 : 0;
}

uint32_t tidStereo(const DetId &id) const {
if ( ((id.rawId() >>tidVals_.sterStartBit_ ) & tidVals_.sterMask_ ) == 1 ) {
return ( (id.rawId()>>tidVals_.sterStartBit_) & tidVals_.sterMask_ );
} else { return 0; }
return ( ((id.rawId() >>tidVals_.sterStartBit_ ) & tidVals_.sterMask_ ) == 1 ) ? 1 : 0;
}

uint32_t tecStereo(const DetId &id) const {
if ( ((id.rawId() >>tecVals_.sterStartBit_ ) & tecVals_.sterMask_ ) == 1 ) {
return ( (id.rawId()>>tecVals_.sterStartBit_) & tecVals_.sterMask_ );
} else { return 0; }
return ( ((id.rawId() >>tecVals_.sterStartBit_ ) & tecVals_.sterMask_ ) == 1 ) ? 1 : 0;
}

uint32_t tibGlued(const DetId &id) const {
if ( ((id.rawId()>>tibVals_.sterStartBit_) & tibVals_.sterMask_ ) == 1 ) {
return ( id.rawId() - 1 );
} else if ( ((id.rawId()>>tibVals_.sterStartBit_) & tibVals_.sterMask_ ) == 2 ) {
return ( id.rawId() - 2 );
} else { return 0; }
uint32_t testId = (id.rawId()>>tibVals_.sterStartBit_) & tibVals_.sterMask_;
return ( testId == 0 ) ? 0 : (id.rawId() - testId);
}

uint32_t tecGlued(const DetId &id) const {
if ( ((id.rawId()>>tecVals_.sterStartBit_) & tecVals_.sterMask_ ) == 1 ) {
return ( id.rawId() - 1 );
} else if ( ((id.rawId()>>tecVals_.sterStartBit_) & tecVals_.sterMask_ ) == 2 ) {
return ( id.rawId() - 2 );
} else { return 0; }
uint32_t testId = (id.rawId()>>tecVals_.sterStartBit_) & tecVals_.sterMask_;
return ( testId == 0 ) ? 0 : (id.rawId() - testId);
}

uint32_t tobGlued(const DetId &id) const {
if ( ((id.rawId()>>tobVals_.sterStartBit_) & tobVals_.sterMask_ ) == 1 ) {
return ( id.rawId() - 1 );
} else if ( ((id.rawId()>>tobVals_.sterStartBit_) & tobVals_.sterMask_ ) == 2 ) {
return ( id.rawId() - 2 );
} else { return 0; }
uint32_t testId = (id.rawId()>>tobVals_.sterStartBit_) & tobVals_.sterMask_;
return ( testId == 0 ) ? 0 : (id.rawId() - testId);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

wha about
return id.rawId() - testId;
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is good, but I still need to test if testId is 1 or 2.
From the perspective of performanc it is not faster, though it is shorter.

Copy link
Contributor

Choose a reason for hiding this comment

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

it can only be 0,1 or 2 ...


uint32_t tidGlued(const DetId &id) const {
if ( ((id.rawId()>>tidVals_.sterStartBit_) & tidVals_.sterMask_ ) == 1 ) {
return ( id.rawId() - 1 );
} else if ( ((id.rawId()>>tidVals_.sterStartBit_) & tidVals_.sterMask_ ) == 2 ) {
return ( id.rawId() - 2 );
} else { return 0; }
uint32_t testId = (id.rawId()>>tidVals_.sterStartBit_) & tidVals_.sterMask_;
return ( testId == 0 ) ? 0 : (id.rawId() - testId);
}

bool tobIsRPhi(const DetId &id) const { return SiStripDetId(id).stereo()==0 && !tobIsDoubleSide(id);}
Expand Down
14 changes: 7 additions & 7 deletions SimTracker/TrackerHitAssociation/src/TrackerHitAssociator.cc
View file
Open in desktop
Expand Up @@ -210,7 +210,7 @@ std::vector<PSimHit> TrackerHitAssociator::associateHit(const TrackingRecHit & t

//check if the recHit is a SiStripMatchedRecHit2D
if(dynamic_cast<const SiStripMatchedRecHit2D *>(&thit)) {
for(auto theSimHitAddr : simhitCFPos) {
for(auto const& theSimHitAddr : simhitCFPos) {
simHitCollectionID theSimHitCollID = theSimHitAddr.first;
simhit_collectionMap::const_iterator it = SimHitCollMap.find(theSimHitCollID);
if (it!= SimHitCollMap.end()) {
Expand All @@ -220,7 +220,7 @@ std::vector<PSimHit> TrackerHitAssociator::associateHit(const TrackingRecHit & t
// Try to remove ghosts by requiring a match to the simTrack also
unsigned int simHitid = theSimHit.trackId();
EncodedEventId simHiteid = theSimHit.eventId();
for(auto id : simtrackid) {
for(auto const& id : simtrackid) {
if(simHitid == id.first && simHiteid == id.second) {
result.push_back(theSimHit);
}
Expand All @@ -233,7 +233,7 @@ std::vector<PSimHit> TrackerHitAssociator::associateHit(const TrackingRecHit & t
}
}
} else {
for(auto theSimHitAddr : simhitCFPos) {
for(auto const& theSimHitAddr : simhitCFPos) {
simHitCollectionID theSimHitCollID = theSimHitAddr.first;
simhit_collectionMap::const_iterator it = SimHitCollMap.find(theSimHitCollID);
if (it!= SimHitCollMap.end()) {
Expand Down Expand Up @@ -290,7 +290,7 @@ std::vector<PSimHit> TrackerHitAssociator::associateHit(const TrackingRecHit & t
const PSimHit& ihit = *simHitIter;
unsigned int simHitid = ihit.trackId();
EncodedEventId simHiteid = ihit.eventId();
for(auto id : simtrackid) {
for(auto const& id : simtrackid) {
if(simHitid == id.first && simHiteid == id.second) {
// if(simHitid == simtrackid[i].first && simHiteid.bunchCrossing() == simtrackid[i].second.bunchCrossing()) {
// cout << "GluedDet Associator ---> ID" << ihit.trackId() << " Simhit x= " << ihit.localPosition().x()
Expand Down Expand Up @@ -394,7 +394,7 @@ void TrackerHitAssociator::associateCluster(const SiStripCluster* clust,
std::vector<simhitAddr> simhitCFPos;
associateSimpleRecHitCluster(clust, detid, simtrackid, &simhitCFPos);

for(auto theSimHitAddr : simhitCFPos ) {
for(auto const& theSimHitAddr : simhitCFPos ) {
simHitCollectionID theSimHitCollID = theSimHitAddr.first;
simhit_collectionMap::const_iterator it = SimHitCollMap.find(theSimHitCollID);
if (it!= SimHitCollMap.end()) {
Expand Down Expand Up @@ -506,7 +506,7 @@ std::vector<SimHitIdpr> TrackerHitAssociator::associateMatchedRecHit(const SiSt
std::vector<SimHitIdpr> simtrackid;
if(!(matched_mono.empty() || matched_st.empty())){
std::vector<SimHitIdpr> idcachev;
for(auto mhit: matched_mono){
for(auto const& mhit: matched_mono){
//save only once the ID
if(find(idcachev.begin(), idcachev.end(), mhit) == idcachev.end()) {
idcachev.push_back(mhit);
Expand Down Expand Up @@ -633,7 +633,7 @@ std::vector<SimHitIdpr> TrackerHitAssociator::associateGSRecHit(const GSSiTrack
{
vector<SimHitIdpr> simtrackid;
simtrackid.clear();
for(size_t index =0;index<gsrechit->nSimTrackIds();++index){
for(size_t index =0, indexEnd = gsrechit->nSimTrackIds(); index<indexEnd; ++index){
SimHitIdpr currentId(gsrechit->simTrackId(index), EncodedEventId(gsrechit->eeId()));
simtrackid.push_back(currentId);
}
Expand Down