small update to CRV coincidence clustering process

[ehrlich-uva]

No description provided.

[FNALbuild]

Hi @ehrlich-uva,
You have proposed changes to files in these packages:

• CRVReco

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for 64acef9: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 64acef9.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 64acef9 at b4e91ff
build (prof)	✅	Log file. Build time: 08 min 22 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❓	Log file. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 1 files
clang-tidy	➡️	2 errors 72 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 64acef9 after being merged into the base branch at b4e91ff.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[oksuzian]

Let me review this pull request to understand the changes and identify any potential issues.Now let me look at the full context of the C++ file around the critical change area to better understand the mutation of a const_iterator element:Here is my review of PR #1776 — a summary of the changes followed by issues found.

---

Summary

This PR introduces a new per-sector FCL parameter finalClusterMaxDistance to the CRV coincidence clustering process. Previously, the _maxDistance on each hit was implicitly reset to 0 at the start of the final clustering pass (since no explicit assignment existed). Now, the parameter is configurable per sector, and all FCL files set it to 0 (preserving current behavior).

Files Changed (5)

File	Change
CRVReco/src/CrvCoincidenceFinder_module.cc	New FCL parameter declaration, struct field, config reading, and initialization of _maxDistance at the start of checkCoincidence
CRVReco/fcl/prolog_v11.fcl	Added finalClusterMaxDistance : 0 to all 25 sector configs
CRVReco/fcl/prolog_v12.fcl	Added finalClusterMaxDistance : 0 to all 23 sector configs
CRVReco/fcl/epilog_extracted_v03.fcl	Added finalClusterMaxDistance : 0 to 3 sector configs
CRVReco/fcl/epilog_run1a_v01.fcl	Added finalClusterMaxDistance : 0 to 2 sector configs

---

Issues Found

1. 🔴 Modifying const_iterator-dereferenced object via mutable — correctness concern on copied data (Line 671)

iterHit->_maxDistance=_sectorMap.at(iterHit->_crvSector).finalClusterMaxDistance;  //set new max distance for final cluster. can be 0.
                                                                                   //will be adjusted to larger values depending on the actual distances between coincidence hits.
int    layer=iterHit->_layer;
hitsLayers[layer].push_back(*iterHit);

The assignment to iterHit->_maxDistance modifies the hit in the input vector hits (which is const std::vector<CrvHit>&), leveraging the mutable keyword on _maxDistance. This works, but on line 674, *iterHit is then copied into hitsLayers[layer]. The copy captures the updated _maxDistance. However, the coincidenceHitSet iterators (lines 730–731, 768–770, 804–807) point into the hitsLayers local arrays, not the original hits vector. When hits are later pushed into coincidenceHits (line 813), they come from hitsLayers copies.

The logic flow is correct but fragile: the mutable keyword allows mutation through const_iterator and const reference, which is an unusual and error-prone pattern. This is a pre-existing design issue, not introduced by this PR, but worth flagging.

2. 🟡 prolog_v12.fcl is missing 2 sectors compared to prolog_v11.fcl

prolog_v11.fcl has 25 sector configurations (R1–R6, L1–L3, T1–T5, E1–E2, U, D1–D4, C1–C4) with the new parameter added.

prolog_v12.fcl has only 23 sector configurations — it appears to be missing sectors C3 and C4. The finalClusterMaxDistance was added to all sectors present in v12, but if v12 is supposed to have those extra sectors (like v11 does), they are missing entirely (not just the new parameter). This may be an existing discrepancy between v11 and v12, not introduced by this PR, but is worth verifying intentionality.

3. 🟡 checkCoincidence parameter is const but hits are mutated (pre-existing design smell)

void CrvCoincidenceFinder::checkCoincidence(const std::vector<CrvHit> &hits, std::list<CrvHit> &coincidenceHits)

The function signature declares hits as const, yet the new code on line 671 mutates iterHit->_maxDistance through the mutable keyword. While technically valid C++, this violates the spirit of the const contract and could surprise future maintainers. Consider changing the signature to take a non-const reference since the function now intentionally modifies the hits.

4. 🟢 Minor: All finalClusterMaxDistance values are 0 — effectively a no-op for now

Every FCL file sets finalClusterMaxDistance : 0. Since the checkCombination function adjusts _maxDistance upward based on actual inter-hit distances (and 0 is the minimum floor), this change is functionally a no-op for the current configuration. The infrastructure is being put in place for future tuning. This is fine, but it means this PR cannot be validated through physics output differences.

5. 🟢 Minor typo in pre-existing comment (line 143)

//_maxDistance is updated when the coindidences are checked

"coindidences" → "coincidences" (pre-existing typo, not introduced by this PR).

---

Overall Assessment

The PR is a clean, mechanical addition of a new configurable parameter (finalClusterMaxDistance) to explicitly control the minimum distance floor used in the final clustering step. The code compiles (build label is present), the change is backward-compatible (all values set to 0), and it is well-structured across all relevant FCL files. The main concern is the const + mutable mutation pattern which, while functional, makes the code harder to reason about and maintain.

[FNALbuild]

📝 The HEAD of main has changed to 723b9b1. Tests are now out of date.

[ehrlich-uva]

Responses

1. Removed a mutable and some consts. It is correct that the updated maxDistance gets applied to the local copy of the hits.
2. This is correct. v12 has fewer sectors than v11.
3. Removed a mutable and some consts.
4. Correct. We may change finalClusterMaxDistance to a value other than 0 in the future, if necessary.
5. Typo fixed.

[oksuzian]

@FNALbuild build

[FNALbuild]

⌛ The following tests have been triggered for ea0c0e6: build (Build queue - API unavailable)

[FNALbuild]

☀️ The build tests passed at ea0c0e6.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged ea0c0e6 at 9576add
build (prof)	✅	Log file. Build time: 04 min 05 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❓	Log file. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 1 files
clang-tidy	➡️	2 errors 73 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at ea0c0e6 after being merged into the base branch at 9576add.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.