Skip to content

New fields for PD compatibility#87

Open
dariopullia wants to merge 3 commits into
mainfrom
working_pd_cafmake_duneanaobj
Open

New fields for PD compatibility#87
dariopullia wants to merge 3 commits into
mainfrom
working_pd_cafmake_duneanaobj

Conversation

@dariopullia
Copy link
Copy Markdown
Member

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:

  • Added a new isBeamSlice boolean field to SRInteraction to indicate if an interaction is identified as the beam slice.
  • Added a beamPandoraSliceIndex integer field to SRInteractionBranch to store the index of the slice identified as the beam slice by Pandora reconstruction.

Truth Matching Improvements:

  • Added a truthMatchIndex field to SRRecoParticle to record the index of the best truth match by hits, defaulting to -1 if no match exists.

Serialization and Versioning:

  • Updated class versions and checksums in classes_def.xml for all modified classes to ensure correct serialization and backward compatibility. [1] [2] [3] [4]

Copy link
Copy Markdown
Collaborator

@chenel chenel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Comment on lines +52 to +53
int truthMatchIndex = -1; ///< Index of the best truth match by hits in truth and truthOverlap array, defaults to -1 if no match

Copy link
Copy Markdown
Collaborator

@chenel chenel May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants