Feature/adding blip to caf #603
Conversation
|
Adding a comment to each of these to track all 4 related PR. The changes to sbnobj, and sbnanaobj are fully independent of any other changes, so they can be approved first. sbndcode changes rely on sbnobj, so it will have to wait for the first approval. A later simple PR will delete the (now duplicated) class files in the BlipUtils folder here sbncode changes rely on both sbnobj and sbnanaobj, so that will have to wait for both of the first two approvals. |
|
Pushed all the fcl changes needed to run CAF on data and MC. |
JosiePaton
left a comment
JosiePaton
left a comment
There was a problem hiding this comment.
Assuming reasonable file size change result!
sbncode/CAFMaker/CAFMaker_module.cc
Outdated
| int fGenieEvtRec_brStdHepLd [250] = {0}; ////< Last daughter | ||
| int fGenieEvtRec_brStdHepFm [250] = {0}; ////< First mother | ||
| int fGenieEvtRec_brStdHepLm [250] = {0}; ////< Last mother | ||
| //std::string fBlipTag; |
There was a problem hiding this comment.
Can you remove old commented code?
sbncode/CAFMaker/CAFMakerParams.h
Outdated
| "cvn" | ||
| }; | ||
| Atom<std::string> fBlipTag { Name("BlipTag"), | ||
| Comment("Provide a string to label the blip input"), "reco2:Blip:BlipReco" |
There was a problem hiding this comment.
This should be the producer that made the blips.
Is that not defined as blipreco here?
There was a problem hiding this comment.
Yes that is what it should be. I thought it would be the whole LArSoft label, so guessed this version. Its wrong and I am happy to update it to blipreco
sbncode/CAFMaker/FillBlip.cxx
Outdated
| CAF_Blip.clusters[iPlane].ID = LarBlip.clusters[iPlane].ID; | ||
| CAF_Blip.clusters[iPlane].isValid = LarBlip.clusters[iPlane].isValid; | ||
| CAF_Blip.clusters[iPlane].CenterChan = LarBlip.clusters[iPlane].CenterChan; | ||
| CAF_Blip.clusters[iPlane].CenterWire = LarBlip.clusters[iPlane].CenterWire; | ||
| CAF_Blip.clusters[iPlane].isTruthMatched = LarBlip.clusters[iPlane].isTruthMatched; | ||
| CAF_Blip.clusters[iPlane].isMerged = LarBlip.clusters[iPlane].isMerged; | ||
| CAF_Blip.clusters[iPlane].isMatched = LarBlip.clusters[iPlane].isMatched; |
There was a problem hiding this comment.
In general there are quite a few indentation inconsistencies across these PRs. It's often different editors and how they treat tabs, there are normally ways to untabify though.
| @@ -0,0 +1,104 @@ | |||
| #include "sbncode/CAFMaker/FillBlip.h" | |||
There was a problem hiding this comment.
Thanks for putting in a separate file, FillReco is getting unwieldy.
|
Just noting on this PR as well that the above Henry comments should all be addressed. |
PetrilloAtWork
left a comment
PetrilloAtWork
left a comment
There was a problem hiding this comment.
A few things need to be changed (redundant headers, missing const parameters), others are recommended to.
Details in the comments.
sbncode/CAFMaker/FillBlip.cxx
Outdated
| //These are sets that need to become vectors so we need to do some loops | ||
| for(auto HitID : LarBlip.clusters[iPlane].HitIDs) CAF_Blip.clusters[iPlane].HitIDs.push_back(HitID); |
There was a problem hiding this comment.
I recommend to use the appropriate std::vector member function, assign():
| //These are sets that need to become vectors so we need to do some loops | |
| for(auto HitID : LarBlip.clusters[iPlane].HitIDs) CAF_Blip.clusters[iPlane].HitIDs.push_back(HitID); | |
| // These are sets that need to become vectors | |
| CAF_Blip.clusters[iPlane].HitIDs.assign(begin(LarBlip.clusters[iPlane].HitIDs), end(LarBlip.clusters[iPlane].HitIDs)); |
If this function is changed as recommended above to work on a single cluster, this becomes less wordy and more readable:
| //These are sets that need to become vectors so we need to do some loops | |
| for(auto HitID : LarBlip.clusters[iPlane].HitIDs) CAF_Blip.clusters[iPlane].HitIDs.push_back(HitID); | |
| // These are sets that need to become vectors | |
| CAFcluster.HitIDs.assign(begin(LArCluster.HitIDs), end(LArCluster.HitIDs)); |
same for the next three lines; in fact, you could even write a specific function and use it repeatedly:
| //These are sets that need to become vectors so we need to do some loops | |
| for(auto HitID : LarBlip.clusters[iPlane].HitIDs) CAF_Blip.clusters[iPlane].HitIDs.push_back(HitID); | |
| // These are sets that need to become vectors | |
| auto copyToVector = [](auto& dest, auto const& src) | |
| { dest.assign(begin(src), end(src)); }; | |
| copyToVector(CAF_Blip.clusters[iPlane].HitIDs, LarBlip.clusters[iPlane].HitIDs); |
etc.
sbncode/CAFMaker/FillBlip.h
Outdated
| void FillMCTruthBlip(blip::Blip& LarBlip, caf::SRBlip& CAF_Blip ); | ||
| void FillBlipRealtedHitCluster(blip::Blip& LarBlip, caf::SRBlip& CAF_Blip); |
There was a problem hiding this comment.
In both these functions one of the operands (the first one, I'd wage) must be declared const.
Also, it seems that the only function needed externally (by CAFMaker module) is the first one; the other two do not need to be publicly visible in the header, and their declaration can be either omitted or moved out of the header into the implementation (.cxx) file.
kjplows
moved this from Open pull requests
to Partially reviewed
in SBN software development
|
Hi @PetrilloAtWork , it looks like Jacob has implemented all the changes requested. How does the PR look now? Thanks! |
PetrilloAtWork
left a comment
PetrilloAtWork
left a comment
There was a problem hiding this comment.
Thank you for your changes.
|
Let me make one more push for changes to sbnanaobj |
|
Okay that compiles. We may have to edit how the ID get recorded for CAF. Please hold off on this PR until then |
Added CAFMaker interface to add blips to CAF files.
This PR must not be approved until SBNSoftware/sbnobj#155 and SBNSoftware/sbnanaobj#173 are approved/released.
Working on estimating the file size change now.