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
ngrenz: testing of stereo optimization #10963
Changes from 4 commits
95334b3
01f74b5
7cec228
b092a27
e41f093
6d24341
5dda346
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 |
---|---|---|
|
@@ -112,65 +112,66 @@ class SiStripDetId : public DetId { | |
// ---------- inline methods ---------- | ||
|
||
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; | ||
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; | ||
case TOB: geometry = int((id_>>layerStartBit_) & layerMask_)<5? OB2 : OB1; | ||
case TID: switch ((id_>>ringStartBitTID_) & ringMaskTID_) { | ||
case 1: return W1A; | ||
case 2: return W2A; | ||
case 3: return W3A; | ||
case 1: geometry = W1A; | ||
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. you are missing "break;" after each assignment (otherwise everything below a selected "case" is executed 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. thanks |
||
case 2: geometry = W2A; | ||
case 3: geometry = W3A; | ||
} | ||
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; | ||
case 2: geometry = W2B; | ||
case 3: geometry = W3B; | ||
case 4: geometry = W4; | ||
case 5: geometry = W5; | ||
case 6: geometry = W6; | ||
case 7: geometry = W7; | ||
} | ||
case UNKNOWN: default: return UNKNOWNGEOMETRY; | ||
case UNKNOWN: default: geometry = UNKNOWNGEOMETRY; | ||
} | ||
return geometry; | ||
} | ||
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. The compiler is complaining that if
|
||
|
||
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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
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. wha about 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. The idea is good, but I still need to test if testId is 1 or 2. 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. 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);} | ||
|
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.
@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 toinline uint32_t SiStripDetId::stereo() const { return ( ((id_>>sterStartBit_ ) & sterMask_ ) == 1 ) ? 1:0; }
but I am not sure whether the readability would suffer
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.
@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.
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.
Thanks, I will rewrite the function.
Do I need the inline expression?
Or rather I do not understand why there is the comment
but no inline expressions, in the original code
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 implementation of a method is inside header file it is assumed to be inlined even without a declaration.
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 find
return 1 == (id_>>sterStartBit_ ) & sterMask_ ) ? 1:0;
very clear and obvious