Skip to content

Fix bug in CPR #1813

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

Merged
oksuzian merged 1 commit into from
Apr 29, 2026
+23 −10
Merged

Fix bug in CPR #1813

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CalPatRec/inc/CalHelixFinderData.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,10 @@ namespace mu2e {
float eDepAvg;
};

const TimeCluster* _timeCluster; // hides vector of its time cluster straw hit indices
const TimeCluster* _timeCluster = nullptr; // hides vector of its time cluster straw hit indices
art::Ptr<TimeCluster> _timeClusterPtr;

HelixTraj* _helix;
HelixTraj* _helix = nullptr;
Helicity _helicity;

std::vector<int> _goodhits;
Expand Down
24 changes: 16 additions & 8 deletions CalPatRec/src/CalHelixFinderAlg.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,14 @@ namespace mu2e {
// the helix fit introduces a radial bias due to an asymmetry in the detector (more phase space for
// noise hits outside the circle than inside. correct for it.
float radius = Helix._radius; // + _rbias;
if (radius == 0. || Helix._dfdz == 0.) return;
if (radius == 0. || Helix._dfdz == 0.) {
std::cout << "[CalHelixFinderAlg::" << __func__ << "] "
<< "Bad Helix parameters! Radius = " << radius
<< " dphi/dz = " << Helix._dfdz
<< std::endl;
Helix._helix = nullptr;
return;
}
// omega is the inverse transverse radius of the particle's circular motion.
// It is signed by the particle angular momentum about the cirle center.
// This CANNOT be deduced geometrically, so must be supplied as an ad-hoc assumption
Expand Down Expand Up @@ -337,7 +344,7 @@ namespace mu2e {
Helix._fit = TrkErrCode(TrkErrCode::fail,3); // xy reconstruction failure
}
else if ((Helix._nZPhiSh < _minNHits) || (Helix._szphi.chi2DofLine() > _chi2zphiMax) ||
(fabs(Helix._szphi.dfdz()) < _minDfDz) || (fabs(Helix._szphi.dfdz()) > _maxDfDz)) {
(fabs(Helix._dfdz) < _minDfDz) || (fabs(Helix._dfdz) > _maxDfDz)) {
Helix._fit = TrkErrCode(TrkErrCode::fail,4); // phi-z reconstruction failure
}
else {
Expand All @@ -363,7 +370,7 @@ namespace mu2e {
}

defineHelixParams(Helix);
retval = true;
retval = Helix.helix() != nullptr;
}

return retval;
Expand Down Expand Up @@ -1049,8 +1056,8 @@ namespace mu2e {
success = true;
}
//----------------------------------------------------------------------//
if ( (Helix._szphi.qn() < minNFitHits) ||
((Helix._szphi.qn() >= minNFitHits) && (Helix._szphi.dfdz()*_dfdzsign < 0.)) ) {
if ( (Helix._szphi.qn() < minNFitHits) || // too few hits
(Helix._szphi.dfdz()*_dfdzsign < 0.) ) { // wrong slope
Copy link
Copy Markdown
Collaborator

@brownd1978 brownd1978 Apr 28, 2026

Choose a reason for hiding this comment

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

Above you changed Helix._szphi.dfdz() to Helix._dfdz. What is the difference between these 2 ways of accessing dfdz, and how is a user to know the right one?

Copy link
Copy Markdown
Contributor Author

@michaelmackenzie michaelmackenzie Apr 28, 2026

Choose a reason for hiding this comment

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

In the case the fit is not successful in the phi vs. z fit stage, it doesn't update the Helix_.dfdz parameters but the line fit object can still have non-zero results. The Helix._dfdz is what's actually used in the helix downstream. In cases where the fit is successful, I believe they only have numerical effect differences (this is what I saw through printouts at least). This is why this failure wasn't caught here though, this line fitter converged to a reasonable value but with the wrong slope, and that failure wasn't being properly checked. I'm not an expert on this algorithm unfortunately, so it's possible I misunderstood something in the code...

success = false;
}
else if (success) { // update helix results
Expand Down Expand Up @@ -1189,7 +1196,7 @@ namespace mu2e {
ordChCol.reserve(size);

if (_debug >0 ){
printf("-----------------------------------------------------------------------------------/n");
printf("-----------------------------------------------------------------------------------\n");
printf("[CalHelixFinderAlg::fillFaceOrderedHits] Plane Panel Layer Straw x y z \n");
}

Expand Down Expand Up @@ -1731,8 +1738,9 @@ namespace mu2e {
Helix._diag.nHitsRatio = nHitsRatio;
}

if (nHitsRatio > _maxNHitsRatio) goto PATTERN_RECOGNITION_END;
if (Helix._szphi.qn() == 0.) goto PATTERN_RECOGNITION_END;
if (nHitsRatio > _maxNHitsRatio) goto PATTERN_RECOGNITION_END; // bad hit ratio
if (Helix._szphi.qn() == 0.) goto PATTERN_RECOGNITION_END; // no hits found on line
if (!rc) goto PATTERN_RECOGNITION_END; // failed linear fit

rescueHitsBeforeSeed(Helix);
//-----------------------------------------------------------------------------
Expand Down
5 changes: 5 additions & 0 deletions CalPatRec/src/CalHelixFinder_module.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,11 @@ namespace mu2e {
int rc = _hfinder.findHelix(tmpResult);

if (!rc) continue;
if(!tmpResult.helix()) {
std::cout << "[CalHelixFinder::" << __func__ << "] " << event.id()
<< " Helix found but nullptr returned!!\n";
continue;
}
HelixSeed tmp_helix_seed;

initHelixSeed(tmp_helix_seed, tmpResult);
Expand Down