Adopt onnx workflow over TMVA injecting low-q2 reconstructed particles later #2018
Conversation
github-actions
bot
added
the
topic: far-backward
|
Comparison benchmark plots for the currently trained TMVA vs. onnx networks available on the pythia6 test sample. (Note there is nothing inherently better about the onnx network for the reconstruction just how it was trained) TMVA being replaced: New onnx reconstruction: |
…s later (fix: iwyu) (#2019) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/17067207522. Please merge this PR into the branch `Swap-Low-Q2-reconstruction-from-TMVA-to-onnx` to resolve failures in PR #2018. 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>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…s later (fix: iwyu) (#2020) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/17071090254. Please merge this PR into the branch `Swap-Low-Q2-reconstruction-from-TMVA-to-onnx` to resolve failures in PR #2018. 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>
simonge
force-pushed
the
Swap-Low-Q2-reconstruction-from-TMVA-to-onnx
branch
from
4923428 to
f94d87f
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…s later (fix: iwyu) (#2019) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/17067207522. Please merge this PR into the branch `Swap-Low-Q2-reconstruction-from-TMVA-to-onnx` to resolve failures in PR #2018. 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>
simonge
force-pushed
the
Swap-Low-Q2-reconstruction-from-TMVA-to-onnx
branch
from
f94d87f to
363a72e
Compare
github-actions
bot
dismissed
their stale review
No Clang-Tidy warnings found so I assume my comments were addressed
src/global/reco/reco.cc
Outdated
| app->Add( | ||
| new JOmniFactoryGeneratorT<CollectionCollector_factory<edm4eic::ReconstructedParticle, true>>( | ||
| "CombinedReconstructedChargedParticles", | ||
| {"ReconstructedChargedParticles", "TaggerTrackerReconstructedParticles"}, |
There was a problem hiding this comment.
This changes definition of ReconstructedChargedParticles to not include FB particles. We probably don't want that.
There was a problem hiding this comment.
There are no clean ways of doing this. Assuming we don't want to replace ReconstructedChargedParticles with a subset collection we need to pass it though another algorithm first, this would mean merging it before the final PID factory, which may be the only reasonable approach but feels unnecessary.
There was a problem hiding this comment.
My alternative proposition is to add a factory which re-evaluates the PID based on the ParticleIDs from all of the PID detectors, as at the moment the PDG given to the particle is only based on the final pid_lut with a relevant cell in the table. This is out of scope of this PR and should probably be handled by someone from the PID groups.
There was a problem hiding this comment.
If I understand correctly, the issue is with needing association rewriting. Letting pid_lut algorithm do the rewriting for you is one way. In one of the PRs on calorimetry we've envisioned a generic association rewriting algorithm for such use cases, but wasn't implemented so far.
Co-authored-by: Dmitry Kalinkin <[email protected]>
…s later (fix: iwyu) (#2048) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/17192855642. Please merge this PR into the branch `Swap-Low-Q2-reconstruction-from-TMVA-to-onnx` to resolve failures in PR #2018. 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>
Briefly, what does this PR introduce?
Swaps out the particles reconstructed via the TMVA network for the onnx reconstruction. The particles are added to the ReconstructedParticles collection workflow later to remove unnecessary combinatorics in the tracking and pid stages.
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?
Several intermediate collections have been removed from the output. Anyone using ReconstructedChargedParticles will no longer have the Low-Q2 electrons included.
Does this PR change default behavior?
The resolution of the particles reconstructed from the low-q2 tagger should be slightly improved over the current method.