-
Notifications
You must be signed in to change notification settings - Fork 10
Some GAr-related additions #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
chenel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part this looks fine to me.
The only thing I'm hesitant about is the SRGArParticle class. One of the design principles for the DUNE StandardRecord that we've tried to observe is that anything that's detector-agnostic should be in the "common" reco block rather than the detector-specific one. It seems to me that SRGArParticle mixes detector-specific and detector-agnostic together. Stuff like ToF times, ECal energies, etc. clearly is detector-specific, but maybe doesn't belong in a class called Particle since it's not ... particle-y, so much, but lower level. On the other hand, scores for "proton-ness" (proton_dEdX_score etc.) and "muon-ness" (muon_score) are clearly particle-y, but it's not clear they have to belong in a GAr-specific class.
Can we discuss whether it's possible to separate these into something lower level (without the name Particle) that contains the detector-specific stuff, and move the remainder into the SRParticle class in the common block?
|
Hi Jeremy! Thanks for reviewing this. I made some changes, trying to separate the detector-y and particle-y things as much as I could. Does it look better now? |
chenel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Francisco, this is much closer! Bravo. The only remaining concern I have is with the new SRPIDScoreBranch object. It's at odds with the way that one deduces what "type" of PID score the other particle types represent. For the others, it's done via which collection they live in, inside the SRRecoParticlesBranch (have a look at the Doxygen for SRRecoParticlesBranch and you'll see what I mean.)
We could adapt to this format easily enough by just creating 3 new vectors there: gar_mu, gar_proton_dedx, gar_proton_tof. I wonder what you think of this possibility?
|
Is this PR one that Francisco mentioned at the collaboration meeting (Sep. 10)? It has been languishing in review for a long time. Can we get this to move? |
|
Hi @chenel! Sorry it took me so long, I've been thesis writing all summer. Do you mean creating 3 vectors in I can try to make the |
Yeah, that's what I had in mind. It's fairly clunky but is the way different particle hypotheses are constructed elsewhere. Particles are not specific to |
|
Is this PR still alive? Shall we try to get it in? I believe Jeremy is tagged as the reviewer. |
|
I reviewed it and there's an outstanding request for a change that hasn't yet been acted on. |
Added GArSoft reconstruction to SRInteractionBranch and SRRecoParticlesBranch.
Small additions to SRGAr and other GAr-related classes. Also, created SRGArPartcile class (inherits from SRRecoParticle) to include additional low-level information in SRGAr.