-
Notifications
You must be signed in to change notification settings - Fork 15
Update Pandora Reco branch filler to use new variables in Pandora output file #110
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?
Update Pandora Reco branch filler to use new variables in Pandora output file #110
Conversation
…r v0 tracks are either muons or protons.
…he track is contained. TODO add the dedicated trkfitIsContained branch in PandoraOuterface
…rface_new_fields_into_Pandora_filler adding shower variables
…d using branches different from the PandoraOuterface ones
…f code that skipped the showers
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.
I don't see any major issues here from the high-level CAFMaker perspective. I would feel better if @brucehoward-physics or @jback08 could give their okay that everything checks out before we merge it. But so long as it doesn't introduce any build issues, I'm okay with going forward if there are other downstream things waiting.
jback08
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.
Overall the code looks OK, but I have made a few minor suggestions.
The new variables require the Pandora ND outerface algorithm to be made available in the LArRecoND package, which has the PandoraInterface run application that is used by the ND production to run the Pandora reco. This is being handled by LArRecoND PR52.
We should check that this code can run OK with and without the outerface code variables being stored in the hierachy ROOT input file that is used by this CAF maker, and that the previously filled MC truth & reco variables remain the same before/after the update (for those expected to remain the same). The outerface variables should be set to sensible default values if they don't exist in the input file.
| // as version 0, the track is chosen between muon and proton | ||
| // TODO consider also pion and kaons | ||
|
|
||
| int assigned_pdg = ((*m_trkfitPID_Mu)[i] < (*m_trkfitPID_Pro)[i]) ? 13 : 2212; |
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.
We could use const int here if it remains unchanged after being defined. We could also have 13 and 2212 as named integer parameters e.g. m_muonPDG and m_protonPDG that are defined in the constructor?
| int assigned_pdg = ((*m_trkfitPID_Mu)[i] < (*m_trkfitPID_Pro)[i]) ? 13 : 2212; | ||
| recoParticle.pdg = assigned_pdg; | ||
|
|
||
| float trkfitStartX = (*m_trkfitStartX)[i]; |
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.
Add const for all variables here.
| p_mod = (*m_trkfitPFromLengthProton)[i]; | ||
| recoParticle.score = (*m_trkfitPID_Pro)[i]; | ||
| } | ||
| else |
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.
Is this empty else statement needed?
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.
I am putting a TODO inside this empty statement, just to say that once other particle options are available, we can add the piece of code there
|
|
||
| recoParticle.E_method = caf::PartEMethod::kRange; | ||
|
|
||
| if(assigned_pdg == 13) |
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.
Perhaps use m_muonPDG (initialized as 13)? Also define the masses as float variables defined in the constructor, e.g. initialize m_mMuon or similar as 0.1056583755 in the constructor, then use m_mMuon instead of the hardcoded numbers when setting the reco particle total energy?
| // ------------------------------------------------------------------------------ | ||
| bool PandoraLArRecoNDBranchFiller::FillShower(const int i, caf::SRRecoParticle& recoParticle) const | ||
| { | ||
| float shwrfitStartX = (*m_shwrfitStartX)[i]; |
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.
Make these variables const
|
|
||
| if ((*m_trackScoreVect)[i] >= m_TrackShowerCut) // track w/ failed trackfit: we still want to fill the recoParticle with the shower info (at least we have something) | ||
| { | ||
| recoParticle.pdg = -2212; // still a track --> assign as default antiproton |
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.
Use m_protonPDG for the proton PDG integer? Similarly for the other PDG numbers used below?
…nches are missing. I have copied the previous filler present in the main eliminating some code redundancy
|
Hi all, just putting some sanity-check plots here. These are all scatterplots with the x-axes being the input variables from the input file (Pandora outerface branch), and on the y-axes the corresponding variables as named in the caf output. If I didn’t inject any weirdness in the code, I should see the same numbers. I still have to do this check for the other variables, but these plots here are reassuring. |
In this draft PR we introduce updates to the Pandora Reco branch filler. Recent developments in the post-Pandora workflow (the so-called PandoraOuterface) have added new track/shower-discrimination variables to the Pandora output. The attached image shows the new branches now available in the Pandora output, which we intend to use to populate the SRRecoParticle via the branch filler.