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 2 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
31 changes: 16 additions & 15 deletions DataFormats/SiStripDetId/interface/SiStripDetId.h
View file
Open in desktop
Expand Up @@ -112,18 +112,18 @@ 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;
if ( det() == DetId::Tracker) {
if ( subdetId() == static_cast<int>(SiStripDetId::TEC) ) {
return SiStripDetId::TEC;
} else if ( subdetId() == static_cast<int>(SiStripDetId::TID) ) {
return SiStripDetId::TID;
} else if ( subdetId() == static_cast<int>(SiStripDetId::TOB) ) {
return SiStripDetId::TOB;
} else if ( subdetId() == static_cast<int>(SiStripDetId::TIB) ) {
return SiStripDetId::TIB;
} else {
return SiStripDetId::UNKNOWN;
}
} else {
return SiStripDetId::UNKNOWN;
}
Expand Down Expand Up @@ -161,14 +161,15 @@ uint32_t SiStripDetId::glued() const {

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

uint32_t SiStripDetId::partnerDetId() const {
if ( ((id_>>sterStartBit_) & sterMask_ ) == 1 ) {
uint32_t testId = (id_>>sterStartBit_) & sterMask_;
if ( testId == 1 ) {
return ( id_ + 1 );
} else if ( ((id_>>sterStartBit_) & sterMask_ ) == 2 ) {
} else if ( testId == 2 ) {
return ( id_ - 1 );
} else { return 0; }
}
Expand Down
28 changes: 16 additions & 12 deletions DataFormats/TrackerCommon/interface/TrackerTopology.h
View file
Open in desktop
Expand Up @@ -266,56 +266,60 @@ 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_ );
return 1;
} else { return 0; }
}

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

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

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

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

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

uint32_t tobGlued(const DetId &id) const {
if ( ((id.rawId()>>tobVals_.sterStartBit_) & tobVals_.sterMask_ ) == 1 ) {
uint32_t testId = (id.rawId()>>tobVals_.sterStartBit_) & tobVals_.sterMask_;
if ( testId == 1 ) {
return ( id.rawId() - 1 );
} else if ( ((id.rawId()>>tobVals_.sterStartBit_) & tobVals_.sterMask_ ) == 2 ) {
} else if ( testId == 2 ) {
return ( id.rawId() - 2 );
} else { return 0; }
}
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 ) {
uint32_t testId = (id.rawId()>>tidVals_.sterStartBit_) & tidVals_.sterMask_;
if ( testId == 1 ) {
return ( id.rawId() - 1 );
} else if ( ((id.rawId()>>tidVals_.sterStartBit_) & tidVals_.sterMask_ ) == 2 ) {
} else if ( testId == 2 ) {
return ( id.rawId() - 2 );
} else { return 0; }
}
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