fix(pid): DRICH IRT link migration - #2875
Conversation
Switch IrtCherenkovParticleID input wiring from RawHitsAssociations to RawHitsLinks to avoid dangling references after association collections were removed from default output selections. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
wdconinc
changed the title
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR isolates the DRICH IRT migration from using raw-hit associations to using raw-hit links, updating the IrtCherenkovParticleID algorithm, its factory, and the DRICH plugin wiring so the PID path consumes MCRecoTrackerHitLink collections.
Changes:
- Switched
IrtCherenkovParticleIDalgorithm input fromMCRecoTrackerHitAssociationCollectiontoMCRecoTrackerHitLinkCollection. - Updated
IrtCherenkovParticleID_factoryto readMCRecoTrackerHitLinkinputs and pass them to the algorithm. - Updated DRICH plugin wiring to provide
DRICHRawHitsLinksto the PID factory instead ofDRICHRawHitsAssociations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/factories/pid/IrtCherenkovParticleID_factory.h | Reads MCRecoTrackerHitLink inputs and passes them into the PID algorithm. |
| src/detectors/DRICH/DRICH.cc | Updates factory wiring to feed DRICHRawHitsLinks into DRICH IRT PID. |
| src/algorithms/pid/IrtCherenkovParticleID.h | Updates algorithm input type and input tag name from associations to links. |
| src/algorithms/pid/IrtCherenkovParticleID.cc | Uses in_hit_links in processing and output association/link wiring. |
Suppressed comments (1)
src/algorithms/pid/IrtCherenkovParticleID.cc:466
- This comment now says “hit associations” but the loop iterates
in_hit_links. Renaming the comment to “hit links” will reduce confusion (especially since the output method name still uses "Associations").
// keep the legacy association relation while consuming links for MC truth lookup
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Correction (formatting only): addressed review feedback from pullrequestreview-4965526672 in commit 040dbfe.
Associations output behavior is unchanged: |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/32183689072. Please merge this PR into the branch `wdconinc-irt-link-migration` to resolve failures in PR #2875. Auto-generated by [create-pull-request][1] [1]: https://github.com/peter-evans/create-pull-request Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/algorithms/pid/IrtCherenkovParticleID.cc:147
- This trace message is now using
in_hit_links->size(), which counts link objects (potentially multiple per raw hit) rather than raw hits. The current wording (“raw sensor hits with associated photons”) can be misleading when troubleshooting event content.
trace("number of raw sensor hits with associated photons: {}", in_hit_links->size());
Capybara summary for PR 2875
Last updated 2026-08-19T08:38-04:00 ba0757b |
There was a problem hiding this comment.
LGTM. Also I have downloaded the artifact and I can print out the number of photons fror Aerogel and Gas. BTW, I noticed that now we have RNTuple. This would require to change the benchmarking. I believe in the benchmark we should have to have
auto reader = podio::RNTupleReader();
and podio/RNTupleReader.h has to be added.
The default remains TTree, and only here in EICrecon CI do we explicitly switch to RNTuple. If you want to use RNTuple in the DRICH benchmarks, then you can switch. |
GitHub Actions
Inactive
There is a generic |
Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.
This PR isolates the DRICH IRT link migration from #2822 into a standalone change for focused review. It updates IrtCherenkovParticleID to consume
DRICHRawHitsLinks(usinggetFrom()/getTo()for link access) while preservingaddToRawHitAssociationsoutput behavior by still using association objects. DRICH wiring/factory inputs are updated to pass both links and associations.What is the urgency of this PR?
What kind of change does this PR introduce?
Please check if any of the following apply
Used Copilot CLI to split the change, apply review-requested fixes, and update wording/comments for correctness.