Skip to content
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

Add PandoraLArRecoNDBranchFiller #76

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

jback08
Copy link
Member

@jback08 jback08 commented Aug 22, 2024

Created the PandoraLArRecoNDBranchFiller class to store the reconstruction information from Pandora's LArRecoND package (used for the DUNE ND). It requires a ROOT file created by the HierarchyAnalysisAlgorithm, which uses Pandora's Hierarchy Tools.

The filled reco branches are rec.nd.lar.pandora.tracks and .rec.nd.lar.pandora.showers. No distinction is made (yet) between tracks and showers, and so the same 3D-cluster Particle Flow Objects (PFOs) are used for both. Here is the list of reco variables used:

  1. Start position = cluster vertex point or the first hit position if no vertex is available
  2. End position = cluster end position
  3. Energy = charge Q
  4. Length = primary principal axis length
  5. Quality = number of 3D hits.

Any changes needed here can be done mostly in the LArRecoND package, since the CAF just retrieves the output stored by Pandora's hierarchy algorithm.

The MC truth matching uses Pandora's Hierarchy Tools and not TruthMatcher. Pandora requires all MC particles to have a unique ID (even if they originate from different neutrinos), and so the following offsets are applied to the original (ndlar-flow) MC Ids:

  1. nuId = orig_nuId + 10^8
  2. mcId = orig_mcId + nuIndex * 10^6

where nuIndex = 0 to N-1 for an event with N neutrinos, and the orig_mcId number range restarts from zero for each neutrino. This ensures all IDs are unique, for up to 100 neutrino interactions per event, each containing up to 10^6 hits.

These ID offsets are reversed when the Pandora CAF truth information is filled using the best reco-MC match achieved with Hierarchy Tools, and so the mcId's should then have values consistent with the equivalent input ndlar-flow files. The filled truth information corresponds to:

  1. truth.ixn = orig_nuId
  2. truth.part = orig_mcId (best match)
  3. truth.type = primary, secondary or other
  4. truthOverlap = completeness (best match)

H5 input files are first converted to ROOT using h5_to_root_ndlarflow.py before they are used by LArRecoND & Pandora, which in turn creates the hierarchy analysis output file that can be used to make the equivalent CAFs.

For the trigger, the event run numbers are propagated using those originally from the input ndar_flow ROOT files. Also, the start time is currently set using the ts_start variable, but this needs to be updated (in LArRecoND) to use the correct time and units.

Also updated ndcaf_setup.sh and build.sh to use a consistent environment.

@noeroy noeroy self-assigned this Aug 22, 2024
@noeroy
Copy link
Contributor

noeroy commented Aug 22, 2024

I think that the truth part of it could be an issue, as the truth branches are shared by all reconstructions.

The particles are uniquely identified with two ids, one that identifies the interaction inside a given spill: vertex_id in the flow files, and one that is uniquely identifying for a given interaction which is the Geant4 trajectory id, I believe it is traj_id in the flow files. Those IDs are shared within MLReco and MINERvA so that truth particles are matched to the same id. Would it be possible to have a match between orig_nuId and vertex_id and between orig_mcId and traj_id?

@jback08
Copy link
Member Author

jback08 commented Oct 3, 2024

I think this is ready for review now. We have added the unix_ts variable to set the trigger time, and we are using the same MC Id variables as the other reco methods.

@noeroy noeroy self-requested a review October 7, 2024 16:17
src/reco/PandoraLArRecoNDBranchFiller.cxx Outdated Show resolved Hide resolved
src/reco/PandoraLArRecoNDBranchFiller.cxx Outdated Show resolved Hide resolved
src/reco/PandoraLArRecoNDBranchFiller.cxx Outdated Show resolved Hide resolved
src/reco/PandoraLArRecoNDBranchFiller.cxx Outdated Show resolved Hide resolved
@noeroy noeroy requested review from chenel and noeroy November 7, 2024 15:29
Copy link
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.

Overall looks fine. Just a couple of small grumbles.

ndcaf_setup.sh Outdated
@@ -14,6 +14,7 @@ setup duneanaobj v03_06_01b -q e20:prof
setup hdf5 v1_10_5a -q e20
setup fhiclcpp v4_15_03 -q debug:e20
setup edepsim v3_2_0c -q debug:e20
setup root v6_26_06b -q e20:p3913:prof
Copy link
Collaborator

Choose a reason for hiding this comment

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

A ROOT setup already comes implicitly from the duneanaobj and edepsim requirements (which each depend on ROOT). In the abstract I would prefer to have it explicit, as here, but in practice, if this line conflicts with dependencies inherited from the other products, the resulting environment will have subtle and difficult-to-diagnose inconsistencies. (I've personally wasted hours chasing this sort of thing down in the past.) So unless there's a strong reason to include, I'd like to not add this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, removed.

Comment on lines 8 to 12
if (m_LArRecoNDFile && m_LArRecoNDFile->IsOpen())
{
delete m_LArRecoNDTree; m_LArRecoNDTree = nullptr;
}
delete m_LArRecoNDFile; m_LArRecoNDFile = nullptr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

any chance we could use unique_ptrs here, instead of manual memory management in 2024? suggestions for the knock-on changes for this suggestion below

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to unique pointers.

m_LastTriggerReqd(m_Triggers.end())
{
// Open Pandora LArRecoND hierarchy analysis ROOT file
m_LArRecoNDFile = TFile::Open(pandoraLArRecoNDFilename.c_str(), "read");
Copy link
Collaborator

Choose a reason for hiding this comment

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

to use a unique_ptr, this line would look something like

Suggested change
m_LArRecoNDFile = TFile::Open(pandoraLArRecoNDFilename.c_str(), "read");
m_LArRecoNDFile.reset(TFile::Open(pandoraLArRecoNDFilename.c_str(), "read"));

LOG.VERBOSE() << " Using PandoraLArRecoND file " << pandoraLArRecoNDFilename << "\n";

// Input tree
m_LArRecoNDTree = dynamic_cast<TTree*>(m_LArRecoNDFile->Get("LArRecoND"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

similarly, here, you'd have

Suggested change
m_LArRecoNDTree = dynamic_cast<TTree*>(m_LArRecoNDFile->Get("LArRecoND"));
m_LArRecoNDTree.reset(dynamic_cast<TTree*>(m_LArRecoNDFile->Get("LArRecoND")));

std::vector<int> *m_isPrimaryVect = nullptr;
std::vector<float> *m_completenessVect = nullptr;

float m_LArRho{1.3973f}; // LAr density (g/cm3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be better as a parameter, rather than hard-coded

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the LAr density as an optional config parameter with a default value.

Unique pointers for the ROOT file/tree & LAr density pararmeter
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.

3 participants