Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a small “cloner” meta-algorithm and factory, and wires it into the beam plugin to materialize standalone copies of selected MC beam subset collections, enabling two-stage digitization workflows that don’t keep the full MCParticles truth.
Changes:
- Introduces
Cloner<T>meta algorithm to duplicate elements from an input collection into a new output collection. - Adds
Cloner_factory<T>JANA omni-factory wrapper aroundCloner<T>. - Updates
src/global/beam/beam.ccto produceMCBeamElectronsClonedandMCBeamProtonsClonedcollections from the existing beam subset collections.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/global/beam/beam.cc | Registers two Cloner_factory<edm4hep::MCParticle> instances to clone MCBeamElectrons and MCBeamProtons into standalone collections for two-stage workflows. |
| src/factories/meta/Cloner_factory.h | Defines a generic JOmni-based factory that wires a single input PODIO collection into the Cloner<T> algorithm and exposes a single cloned output collection. |
| src/algorithms/meta/Cloner.h | Implements the generic Cloner<T> algorithm that iterates an input collection and pushes obj.clone() into the output collection to break subset dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/21552375295. Please merge this PR into the branch `cloner` to resolve failures in PR #2384. 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>
|
No diffs. |
|
Commenting a bit of a longer conceptual view, rather than directly this purge of MC information (Thinking about #2385 and #2384). Should we be looking at storing the beam particles as Reconstructed Particles? Even after the metadata is available. An algorithm would then take the vertex collection and the metadata on the beam settings to reconstruct a particle with an accurate covariance matrix. Which would be essential for the qError suggestion in eic/EDM4eic#93 |
I think ultimately there will need to be some way of combining the metadata with primary vertex to get t0 and such, so that seems like a reasonable way forward. But I don't know how close we are to even starting that. |
This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/21552375295. Please merge this PR into the branch `cloner` to resolve failures in PR #2384. 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>
This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/21552375295. Please merge this PR into the branch `cloner` to resolve failures in PR #2384. 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 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/21552375295. Please merge this PR into the branch `cloner` to resolve failures in PR #2384. 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>
| * This algorithm takes an input collection and creates an output collection | ||
| * containing cloned copies of those elements. This is primarily useful for | ||
| * subset collections (where elements point to objects in another collection) | ||
| * to create standalone collections that can be stored without the original. | ||
| * |
| // Clone MCBeamElectrons and MCBeamProtons for two-stage workflows | ||
| // This allows storing just the beam particles without the full MCParticles collection | ||
| app->Add(new JOmniFactoryGeneratorT<Cloner_factory<edm4hep::MCParticle>>( | ||
| "MCBeamElectronsCloned", {"MCBeamElectrons"}, {"MCBeamElectronsCloned"}, app)); | ||
| app->Add(new JOmniFactoryGeneratorT<Cloner_factory<edm4hep::MCParticle>>( |
| // Clone MCBeamElectrons and MCBeamProtons for two-stage workflows | ||
| // This allows storing just the beam particles without the full MCParticles collection | ||
| app->Add(new JOmniFactoryGeneratorT<Cloner_factory<edm4hep::MCParticle>>( | ||
| "MCBeamElectronsCloned", {"MCBeamElectrons"}, {"MCBeamElectronsCloned"}, app)); |
There was a problem hiding this comment.
Should we come up with naming scheme for subset collections instead? How about MCBeamElectronsSubset?
There was a problem hiding this comment.
Do we care about the breakage that could introduce?
There was a problem hiding this comment.
As far as I can imagine, no one knows how to work with subset collections without PODIO.
There was a problem hiding this comment.
Ok, I'll append subset later
This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/29870108483. Please merge this PR into the branch `cloner` to resolve failures in PR #2384. 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 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/algorithms/meta/Cloner.h:43
obj.clone()copies the object's relations as well. For relation-heavy types likeedm4hep::MCParticlethis means the cloned objects can still reference the originalMCParticlescollection (e.g. via parent/daughter relations), defeating the stated goal of being able to persist the cloned collection without persisting the original and likely triggering dangling-relation checks. The codebase explicitly avoidsclone()for MCParticles for this reason (seesrc/algorithms/reco/UndoAfterBurner.ccwhere relationships are not cloned). Consider either (a) implementing an MCParticle-specific cloning path that copies only the value fields and clears/omits relations, or (b) extending this algorithm with an option to drop/rewire relations so outputs are self-contained when the source collection is not written.
// Clone each element from input to output
for (const auto& obj : *in_coll) {
out_coll->push_back(obj.clone());
}
Briefly, what does this PR introduce?
This PR introduces a cloning meta algorithm and factory to turn subset collections into collections of objects. This is used here, in particular, to create a clone of the
MCBeamElectronsandMCBeamProtonscollections (subset collections toMCParticles), so those can be stored in a digitization step in #2380 without storing all truth ofMCParticles.What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
No.
Does this PR change default behavior?
No.