New fields for PD compatibility#87
Conversation
chenel
left a comment
There was a problem hiding this comment.
Thanks for your work here (and in duneana, which I'll get to next)! Glad to see that we are approaching being able to make the CAFs work for ProtoDUNE too.
In general I appreciate the direction/intentions of what you've added here, but there are a few issues here that we'll need to address before we can merge.
The most significant is how this interacts with the overall design of the CAFs: it's important to remember that the structures here are shared across the whole experiment. Several of the new fields belong conceptually to ProtoDUNE-specific reconstruction but have been placed on general SR structures (SRInteraction, SRInteractionBranch) that are shared across all detectors. For isBeamSlice in particular, there is a valid general concept underneath---"did this reconstructed interaction arise from the physics process that triggered this readout?"---which would be useful across ND, FD, and ProtoDUNE alike (since cosmics can intrude anywhere). But the current name and documentation are too ProtoDUNE+Pandora-specific.
Another issue is storing info that can be trivially computed from the content that's already there. beamPandoraSliceIndex (which could be computed from the isBeamSlice) and truthMatchIndex (which could be computed from truthOverlap) seem like they should be helper functions that do that calculation instead.
Finally, the classes_def.xml changes need a closer look: several classes received many more version bumps than the number of fields added here would
explain, and two classes (SRPFP, SRTrueParticle) received version bumps for whitespace-only changes. Do the intermediate versions correspond to persistent changes that need to remain readable? If not, could you strip out the changes that aren't relevant, and renumber the ones that remain to be sequential again?
| bool contained() const; ///< Convenience function to check if the interaction is contained in the detector by checking the contained flag of all reco particles | ||
| }; | ||
|
|
||
| bool isBeamSlice = false; |
There was a problem hiding this comment.
See the more general comment above, but I think this should probably become isFromTrigger or something similar. It also needs a very clear explanatory comment. Here's one possibility:
| bool isBeamSlice = false; | |
| bool isFromTrigger = true; ///< Did this interaction arise from the physics process that triggered this readout? | |
| ///< e.g. for a neutrino beam trigger, this is true for beam-induced neutrino | |
| ///< interactions and false for cosmic overlays; for a ProtoDUNE test-beam trigger, | |
| ///< true for the beam particle interaction and false for cosmics in the readout window. | |
| ///< In the ND with significant pileup, multiple interactions in the same SR may be true. | |
| ///< Defaults to true since in most contexts beam-induced interactions are the expected case. |
|
|
||
| std::vector<SRInteraction> pandora; ///< Interactions from Pandora reconstruction | ||
| std::size_t npandora; | ||
| int beamPandoraSliceIndex = -1; ///< Index of the slice identified as the beam slice by the reconstruction (if any) |
There was a problem hiding this comment.
This field is too ProtoDUNE-specific for the SRInteractionBranch (see general comments). I think it should become a free function (not a SRInteractionBranch method, since it should work on any of the SRInteraction collections here) that computes it here (which you can cache at the call site if you'll otherwise need to do it repeatedly):
/// Returns the indices of all interactions in the collection that arose from
/// the physics process that triggered this readout (see SRInteraction::isFromTrigger).
inline std::vector<std::size_t> IxnsFromThisTrigger(const std::vector<SRInteraction>& interactions)
{
std::vector<std::size_t> indices;
for (std::size_t i = 0; i < interactions.size(); ++i)
if (interactions[i].isFromTrigger)
indices.push_back(i);
return indices;
}
Then it could be used like:
for (std::size_t idx : IxnsFromThisTrigger(sr.common.ixn.pandora))
{
const SRInteraction& ixn = sr.common.ixn.pandora[idx];
// analyse beam-induced interaction
}
or
std::vector<std::size_t> triggerIxns = IxnsFromThisTrigger(sr.common.ixn.pandora);
if (triggerIxns.size() != 1) // uh oh, unexpected for ProtoDUNE
throw ...;
// do something with triggerIxns[0]
| int truthMatchIndex = -1; ///< Index of the best truth match by hits in truth and truthOverlap array, defaults to -1 if no match | ||
|
|
There was a problem hiding this comment.
I think this needs to be removed (it's redundant), unless you have a good counterargument. You could easily write a helper function analogous to the one above.
This pull request adds several new fields to the CAF (Common Analysis Format) data structures to enhance interaction, reconstruction, and truth-matching information. These changes improve the ability to identify and track beam slices, match reconstructed particles to true particles, and maintain compatibility with serialization via updated class versions.
The most important changes are:
Interaction and Reconstruction Enhancements:
isBeamSliceboolean field toSRInteractionto indicate if an interaction is identified as the beam slice.beamPandoraSliceIndexinteger field toSRInteractionBranchto store the index of the slice identified as the beam slice by Pandora reconstruction.Truth Matching Improvements:
truthMatchIndexfield toSRRecoParticleto record the index of the best truth match by hits, defaulting to -1 if no match exists.Serialization and Versioning:
classes_def.xmlfor all modified classes to ensure correct serialization and backward compatibility. [1] [2] [3] [4]