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

Properly handle arrays where index can be negative #21151

Merged
merged 1 commit into from
Nov 9, 2017
Merged

Properly handle arrays where index can be negative #21151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 29 additions & 5 deletions L1Trigger/L1TTwinMux/src/RPCHitCleaner.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,30 @@ RPCHitCleaner::RPCHitCleaner(RPCDigiCollection inrpcDigis){
m_inrpcDigis = inrpcDigis;
}

namespace {
constexpr int max_rpc_bx = 3;
constexpr int min_rpc_bx = -3;

//Need to shift the index so that index 0
// corresponds to min_rpc_bx
class BxToStrips {
public:
BxToStrips(): m_strips{} {} //zero initializes

static bool outOfRange(int iBX) {
return (iBX > max_rpc_bx or iBX < min_rpc_bx) ;
}

int& operator[](int iBX) {
return m_strips[iBX-min_rpc_bx];
}

size_t size() const { return m_strips.size(); }
private:
std::array<int,max_rpc_bx-min_rpc_bx+1> m_strips;
};
}

void RPCHitCleaner::run(const edm::EventSetup& c) {

std::map<detId_Ext, int> hits;
Expand Down Expand Up @@ -69,12 +93,12 @@ void RPCHitCleaner::run(const edm::EventSetup& c) {
for( auto chamber = m_inrpcDigis.begin(); chamber != m_inrpcDigis.end(); ++chamber ){
RPCDetId detid = (*chamber).first;
if(detid.region()!=0 ) continue; //Region = 0 Barrel
int strips[5] = {0};
BxToStrips strips;
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @Dr15Jones @rekovic - so I guess the real bug here is that either strips[5] should have been strips[7] (3+3+1=7) or that fabs(digi->bx())>3 should have been >=3 (so that 2+2+1=5). @rekovic - can you confirm which one it is sometime today?

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 first bug I found was this one
https://github.com/cms-sw/cmssw/pull/21151/files#diff-bb4facdb289f9efc85f12aad556325e7R124

It was a missing ‘+3’ in the array index calculation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore that, I shouldn’t reply to messages before having coffee in the morning.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems @rekovic won't comment so we'll go forward here. (and push in 94x)

int cluster_n1 = -10;
bx_hits[detid] = 10;
//Keep cluster with min bx in a roll
for( auto digi = (*chamber).second.first ; digi != (*chamber).second.second; ++digi ) {
if(fabs(digi->bx())>3 ) continue;
if(BxToStrips::outOfRange(digi->bx()) ) continue;
//int cluster_id = hits[(detid.ring()+2)][(detid.station()-1)][(detid.sector()-1)][(detid.layer()-1)][(digi->bx()+2)][detid.roll()-1][digi->strip()];
detId_Ext tmp{detid,digi->bx(),digi->strip()};
int cluster_id = hits[tmp];
Expand All @@ -93,11 +117,11 @@ void RPCHitCleaner::run(const edm::EventSetup& c) {
///keep only one bx per st/sec/wheel/layer
if(digi->bx()!=bx_hits[detid] ) continue;
///Count strips in a cluster
if(cluster_n1 != cluster_id) {strips[digi->bx()+3] = {0}; }
strips[digi->bx()+3] ++ ;
if(cluster_n1 != cluster_id) {strips[digi->bx()] = {0}; }
strips[digi->bx()] ++ ;
cluster_n1 = cluster_id;

if( vcluster_size[cluster_id] ==3 && strips[digi->bx()+3]!=2) continue;
if( vcluster_size[cluster_id] ==3 && strips[digi->bx()]!=2) continue;
///Keep clusters with size=2. Calculate and store the mean phi in RPCtoDTTranslator
RPCDigi digi_out(digi->strip(), digi->bx());
m_outrpcDigis.insertDigi(detid, digi_out);
Expand Down
45 changes: 33 additions & 12 deletions L1Trigger/L1TTwinMux/src/RPCtoDTTranslator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,44 @@ RPCtoDTTranslator::RPCtoDTTranslator(RPCDigiCollection inrpcDigis){

}

namespace {
constexpr int max_rpc_bx = 2;
constexpr int min_rpc_bx = -2;

//Need to shift the index so that index 0
// corresponds to min_rpc_bx
class BxToHit {
public:
BxToHit(): m_hits{} {} //zero initializes

static bool outOfRange(int iBX) {
return (iBX > max_rpc_bx or iBX < min_rpc_bx);
}

int& operator[](int iBX) {
return m_hits[iBX-min_rpc_bx];
}

size_t size() const { return m_hits.size(); }
private:
std::array<int,max_rpc_bx-min_rpc_bx+1> m_hits;
};
}


void RPCtoDTTranslator::run(const edm::EventSetup& c) {

std::vector<L1MuDTChambPhDigi> l1ttma_out;
std::vector<L1MuDTChambPhDigi> l1ttma_hits_out;

std::vector<rpc_hit> vrpc_hit_layer1, vrpc_hit_layer2, vrpc_hit_st3, vrpc_hit_st4;

int max_rpc_bx = 2; int min_rpc_bx = -2;

///Init structues
for( auto chamber = m_rpcDigis.begin(); chamber != m_rpcDigis.end(); ++chamber ) {
RPCDetId detid = (*chamber).first;
for( auto digi = (*chamber).second.first ; digi != (*chamber).second.second; ++digi ) {
if(detid.region()!=0 ) continue; //Region = 0 Barrel
if(digi->bx()>max_rpc_bx || digi->bx()<min_rpc_bx) continue;
if(BxToHit::outOfRange(digi->bx())) continue;
if(detid.layer()==1) vrpc_hit_layer1.push_back({digi->bx(), detid.station(), detid.sector(), detid.ring(), detid, digi->strip(), detid.roll(), detid.layer()});
if(detid.station()==3) vrpc_hit_st3.push_back({digi->bx(), detid.station(), detid.sector(), detid.ring(), detid, digi->strip(), detid.roll(), detid.layer()});
if(detid.layer()==2) vrpc_hit_layer2.push_back({digi->bx(), detid.station(), detid.sector(), detid.ring(), detid, digi->strip(), detid.roll(), detid.layer()});
Expand Down Expand Up @@ -167,9 +190,7 @@ void RPCtoDTTranslator::run(const edm::EventSetup& c) {

}
///Use ts2tag variable to store N rpchits for the same st/wheel/sec
int const bx_range = (max_rpc_bx - min_rpc_bx) + 1 ;
int hit[bx_range];
for (int n=0; n<bx_range; n++) hit[n] = 0;
BxToHit hit;
itr1=0;
for(unsigned int l1=0; l1<vrpc_hit_layer1.size(); l1++){
if(vrpc_hit_layer1[l1].station!=st || st>2 || vrpc_hit_layer1[l1].sector!=sec || vrpc_hit_layer1[l1].wheel!=wh) continue;
Expand All @@ -188,8 +209,8 @@ void RPCtoDTTranslator::run(const edm::EventSetup& c) {
phi2 /= 2;
}

l1ttma_hits_out.emplace_back(vrpc_hit_layer1[l1].bx, wh, sec-1, st, phi2, 0, 3, hit[vrpc_hit_layer1[l1].bx+2], 0, 2);
hit[vrpc_hit_layer1[l1].bx+2]++;
l1ttma_hits_out.emplace_back(vrpc_hit_layer1[l1].bx, wh, sec-1, st, phi2, 0, 3, hit[vrpc_hit_layer1[l1].bx], 0, 2);
hit[vrpc_hit_layer1[l1].bx]++;
}
itr1 = 0;
for(unsigned int l2=0; l2<vrpc_hit_layer2.size(); l2++){
Expand All @@ -208,8 +229,8 @@ void RPCtoDTTranslator::run(const edm::EventSetup& c) {
phi2 = phi2 + (radialAngle(vrpc_hit_layer2[l2-1].detid, c, vrpc_hit_layer2[l2-1].strip)<<2);
phi2 /= 2;
}
l1ttma_hits_out.emplace_back( vrpc_hit_layer2[l2].bx, wh, sec-1, st, phi2, 0, 3, hit[vrpc_hit_layer2[l2].bx+2] , 0, 2);
hit[vrpc_hit_layer2[l2].bx+2]++;
l1ttma_hits_out.emplace_back( vrpc_hit_layer2[l2].bx, wh, sec-1, st, phi2, 0, 3, hit[vrpc_hit_layer2[l2].bx] , 0, 2);
hit[vrpc_hit_layer2[l2].bx]++;

}
itr1 = 0;
Expand All @@ -230,8 +251,8 @@ void RPCtoDTTranslator::run(const edm::EventSetup& c) {
phi2 = phi2 + (radialAngle(vrpc_hit_st3[l1-1].detid, c, vrpc_hit_st3[l1-1].strip)<<2);
phi2 /= 2;
}
l1ttma_hits_out.emplace_back( vrpc_hit_st3[l1].bx, wh, sec-1, st, phi2, 0, 3, hit[vrpc_hit_st3[l1].bx+2], 0, 2);
hit[vrpc_hit_st3[l1].bx+2]++;
l1ttma_hits_out.emplace_back( vrpc_hit_st3[l1].bx, wh, sec-1, st, phi2, 0, 3, hit[vrpc_hit_st3[l1].bx], 0, 2);
hit[vrpc_hit_st3[l1].bx]++;

}
itr1 = 0;
Expand Down