Skip to content

Refactor far-forward neutrals reconstruction - #2736

Open
baptistefraisse wants to merge 3 commits into
mainfrom
reco/ff-lambda-multicalo
Open

Refactor far-forward neutrals reconstruction #2736
baptistefraisse wants to merge 3 commits into
mainfrom
reco/ff-lambda-multicalo

Conversation

@baptistefraisse

@baptistefraisse baptistefraisse commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR refactors neutral and Lambda reconstruction.

1. Factorize FarForwardNeutralsReconstruction (issue #2689)

Following the discussions in PR #2622 followed by issue #2689, the neutral reconstruction has been refactored so that the same algorithm is used for each calorimeter independently instead of processing everything in a single instance.

## 2. Use index-based cluster loops

The neutral reconstruction now iterates over clusters using index-based loops instead of object-based iteration. This was motivated by the investigation of the Capybara single-threaded / multi-threaded mismatch, as it removes one possible source of non-deterministic behavior.

3. Improve Lambda candidate selection

The Lambda selection has been slightly modified to prioritize the candidate with the lowest chi-2. The selection also continues scanning the remaining candidates when appropriate instead of stopping after the first valid one.

@ruse-traveler @veprbl @DraTeots @wdconinc

@wdconinc

wdconinc commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Range-based for loops are deterministic, so the motivation doesn't seem to make sense. Indices are prone to abuse (and incorrect typing as int when they should likely be size_t throughout, removing the needs for static casts).

@wdconinc

wdconinc commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Please fix the merge conflicts first. They will remove the need for us to comment on things that pre-commit will fix.

@baptistefraisse
baptistefraisse force-pushed the reco/ff-lambda-multicalo branch from 47e8383 to 1127c0c Compare July 9, 2026 20:48
@baptistefraisse

baptistefraisse commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Sorry, my branch was not up to date with main. I restarted the branch from the current main and reapplied the changes cleanly. @wdconinc yes, the range-based loops themselves are deterministic. The motivation behind index-based loops was to ensure that cluster objects / their paths were not involved in the ST/MT discrepancy. But okay, if there's no benefit to keeping it this way, I stick to '&cluster' loops.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

best_k = k;
}
}
std::sort(cands.begin(), cands.end(), better);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ modernize-use-ranges ⚠️
use a ranges version of this algorithm

Suggested change
std::sort(cands.begin(), cands.end(), better);
std::ranges::sort(cands,, better);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants