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.

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

Try to remove a few LogicErrors and DeadStores in DT/CSC LocalMuon reco #27835

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 3 additions & 4 deletions RecoLocalMuon/CSCRecHitD/src/CSCMake2DRecHit.cc
Expand Up @@ -42,7 +42,7 @@ CSCRecHit2D CSCMake2DRecHit::hitFromStripAndWire(const CSCDetId& id,
specs_ = layer->chamber()->specs();
id_ = id;

const float sqrt_12 = 3.4641;
static constexpr float inv_sqrt_12 = 0.2886751;

float tpeak = -99.f;

Expand Down Expand Up @@ -133,8 +133,7 @@ CSCRecHit2D CSCMake2DRecHit::hitFromStripAndWire(const CSCDetId& id,
}
tpeak = peakTimeFinder_->peakTime(tmax, adcArray, tpeak);
// Just for completeness, the start time of the pulse is 133 ns earlier, according to Stan :)
float t_zero = tpeak - 133.f;
LogTrace("CSCRecHit") << "[CSCMake2DRecHit] " << id << " strip=" << centerStrip << ", t_zero=" << t_zero
LogTrace("CSCRecHit") << "[CSCMake2DRecHit] " << id << " strip=" << centerStrip << ", t_zero=" << tpeak - 133.f
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that this is a good strategy in general to just expand the variable inline.
I somehow noticed in the recent few PRs the same feature of variables used in Log* claimed to be unused by the static analyzer.
(perhaps an induced memory) I seem to recall that we were previously running the static analyzer with EDM_ML_DEBUG enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

@slava77 after the integration of cms-sw/cms-bot#1197 this flag is enabled, as far as I can see in the latest test I've run https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ecd997/2318/llvm-analysis/ there is no new claim of unused variable. So are you ok with this update, or would you suggest to revert it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine for this simple case.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, this PR will be integrated in next IB

<< ", tpeak=" << tpeak;

float positionWithinTheStrip = -99.;
Expand All @@ -148,7 +147,7 @@ CSCRecHit2D CSCMake2DRecHit::hitFromStripAndWire(const CSCDetId& id,
lp0 = layergeom_->stripWireIntersection(centerStrip, centerWire);
positionWithinTheStrip = 0.f;
stripWidth = layergeom_->stripPitch(lp0);
sigmaWithinTheStrip = stripWidth / sqrt_12;
sigmaWithinTheStrip = stripWidth * inv_sqrt_12;
quality = 2;
} else {
// If not at the edge, used cluster of size ClusterSize:
Expand Down
15 changes: 6 additions & 9 deletions RecoLocalMuon/DTSegment/src/DTSegmentCand.cc
Expand Up @@ -111,23 +111,20 @@ bool DTSegmentCand::good() const {
}

bool DTSegmentCand::hitsShareLayer() const {
int layerN[20];
int i = 0;

const unsigned int hitsSize = theHits.size();
// we don't expect so many 1D hits, if such a segment arrives just drop it
if (theHits.size() > 20)
if (hitsSize > 20)
return false;

int layerN[hitsSize];
unsigned int i = 0;
for (DTSegmentCand::AssPointCont::iterator assHit = theHits.begin(); assHit != theHits.end(); ++assHit) {
layerN[i] = (*assHit).first->id().layerId().layer() + 10 * (*assHit).first->id().superlayerId().superlayer();
i++;
}

for (int i = 0; i < (int)theHits.size(); i++) {
for (int j = 0; j < i; j++) {
for (unsigned int j = 0; j < i; j++) {
if (layerN[i] == layerN[j])
return true;
}
i++;
}

return false;
Expand Down
6 changes: 3 additions & 3 deletions RecoLocalMuon/DTSegment/src/DTSegmentExtendedCand.cc
Expand Up @@ -29,10 +29,10 @@ using namespace std;
bool DTSegmentExtendedCand::isCompatible(const DTSegmentExtendedCand::DTSLRecClusterForFit& clus) {
LocalPoint posAtSL = position() + direction() * (clus.pos.z() - position().z()) / cos(direction().theta());
// cout << "pos :" << clus.pos << " posAtSL " << posAtSL << endl;
static float errScaleFact = 10.;
static float minError = 25.; // (cm)
static constexpr float errScaleFact = 10.;
static constexpr float minError = 25.; // (cm)
// cout << "clus.err.xx() " << clus.err << endl;
return fabs((posAtSL - clus.pos).x()) < max(errScaleFact * sqrt(clus.err.xx()), minError);
return std::abs((posAtSL - clus.pos).x()) < max(errScaleFact * sqrt(clus.err.xx()), minError);
}

unsigned int DTSegmentExtendedCand::nHits() const { return DTSegmentCand::nHits() + theClus.size(); }
Expand Down
38 changes: 13 additions & 25 deletions RecoLocalMuon/DTSegment/src/DTSegmentUpdator.cc
Expand Up @@ -495,6 +495,10 @@ void DTSegmentUpdator::rejectBadHits(DTChamberRecSegment2D* phiSeg) const {
cout << " Inside the segment updator, now loop on hits: ( x == z_loc , y == x_loc) " << endl;

vector<DTRecHit1D> hits = phiSeg->specificRecHits();
const size_t N = hits.size();
if (N < 3)
return;

for (vector<DTRecHit1D>::const_iterator hit = hits.begin(); hit != hits.end(); ++hit) {
// I have to get the hits position (the hit is in the layer rf) in SL frame...
GlobalPoint glbPos = (theGeom->layer(hit->wireId().layerId()))->toGlobal(hit->localPosition());
Expand All @@ -519,11 +523,6 @@ void DTSegmentUpdator::rejectBadHits(DTChamberRecSegment2D* phiSeg) const {
float Sy2 = 0.;
float Sxy = 0.;

size_t N = x.size();

if (N == 0)
return;

for (size_t i = 0; i < N; ++i) {
Sx += x.at(i);
Sy += y.at(i);
Expand All @@ -541,12 +540,10 @@ void DTSegmentUpdator::rejectBadHits(DTChamberRecSegment2D* phiSeg) const {

// Calc residuals:
float residuals[N];

for (size_t i = 0; i < N; ++i)
residuals[i] = 0;

float mean_residual = 0.; //mean of the absolute values of residuals
for (size_t i = 0; i < N; ++i) {
residuals[i] = y.at(i) - par[1] * x.at(i) - par[0];
mean_residual += std::abs(residuals[i]);
if (debug) {
cout << " i: " << i << " y_i " << y.at(i) << " x_i " << x.at(i) << " res_i " << residuals[i];
if (i == N - 1)
Expand All @@ -557,36 +554,27 @@ void DTSegmentUpdator::rejectBadHits(DTChamberRecSegment2D* phiSeg) const {
if (debug)
cout << " Residuals computed! " << endl;

// Perform bad hit rejecting -- update hits
vector<DTRecHit1D> updatedRecHits;

float mean_residual = 0.; //mean of the absolute values of residuals

for (size_t i = 0; i < N; ++i)
mean_residual += fabs(residuals[i]);

mean_residual = mean_residual / (N - 2);

if (debug)
cout << " mean_residual: " << mean_residual << endl;

int i = 0;

// Perform bad hit rejecting -- update hits
vector<DTRecHit1D> updatedRecHits;
for (vector<DTRecHit1D>::const_iterator hit = hits.begin(); hit != hits.end(); ++hit) {
DTRecHit1D newHit1D = (*hit);

float normResidual = mean_residual > 0 ? fabs(residuals[i]) / mean_residual : 0;
float normResidual = mean_residual > 0 ? std::abs(residuals[i]) / mean_residual : 0;
++i;
if (normResidual < 1.5) {
DTRecHit1D newHit1D = (*hit);
updatedRecHits.push_back(newHit1D);
if (debug)
cout << " accepted " << i + 1 << "th hit"
cout << " accepted " << i << "th hit"
<< " Irej: " << normResidual << endl;
++i;
} else {
if (debug)
cout << " rejected " << i + 1 << "th hit"
cout << " rejected " << i << "th hit"
<< " Irej: " << normResidual << endl;
++i;
continue;
}
}
Expand Down