Feature/adding blip to caf #603
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,9 +39,11 @@ art_make_library( LIBRARY_NAME sbncode_CAFMaker | |
| sbnobj::Common_Trigger | ||
| sbnobj::SBND_CRT | ||
| sbnobj::SBND_Timing | ||
| sbndobj_BlipDataTypes | ||
|
||
| lardataalg::DetectorInfo | ||
| art::Framework_Services_System_TriggerNamesService_service | ||
| sbncode_Metadata_MetadataSBN_service | ||
| #sbndcode_BlipUtils | ||
| larsim::Utils | ||
| larevt::SpaceCharge | ||
| systematicstools::interface | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,104 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "sbncode/CAFMaker/FillBlip.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Thanks for putting in a separate file, FillReco is getting unwieldy. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| namespace caf | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void FillBlip( const std::vector<blip::Blip>& LarBlips, std::vector<caf::SRBlip>& CAF_Blips) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int NBlips = LarBlips.size(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for(int iBlip=0; iBlip<NBlips; iBlip++) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| blip::Blip CurrentBlip = LarBlips[iBlip]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int NBlips = LarBlips.size(); | |
| for(int iBlip=0; iBlip<NBlips; iBlip++) | |
| { | |
| blip::Blip CurrentBlip = LarBlips[iBlip]; | |
| for(blip::Blip const& CurrentBlip: LarBlips) | |
| { |
This avoids copies (blip::Blip CurrentBlip = LarBlips[iBlip] copies) and the definition of two variables that are not used anywhere else.
I recommend to adopt the suggested syntax.
Outdated
There was a problem hiding this comment.
LarBlip is not modified, so it needs to be declared constant:
| void FillMCTruthBlip( blip::Blip& LarBlip, caf::SRBlip &CAF_Blip ) | |
| void FillMCTruthBlip( blip::Blip const& LarBlip, caf::SRBlip &CAF_Blip ) |
Also, since the function is dealing only with LarBlip.truth and CAF_Blip.truthBlip, the arguments should be these two rather than LarBlip and CAF_Blip.
Same argument name comment as above, too.
There was a problem hiding this comment.
Updated this and hitCluster functions
Outdated
There was a problem hiding this comment.
Remove obsolete, commented out code.
| //CAF_Blip.truthBlip = LarBlip.truth; |
Outdated
There was a problem hiding this comment.
Please use consistent spacing:
| CAF_Blip.truthBlip.Cryostat =LarBlip.truth.Cryostat; | |
| CAF_Blip.truthBlip.TPC =LarBlip.truth.TPC; | |
| CAF_Blip.truthBlip.Time =LarBlip.truth.Time; | |
| CAF_Blip.truthBlip.TimeTick =LarBlip.truth.TimeTick; | |
| CAF_Blip.truthBlip.DriftTime =LarBlip.truth.DriftTime; | |
| CAF_Blip.truthBlip.Energy =LarBlip.truth.Energy; | |
| CAF_Blip.truthBlip.DepElectrons =LarBlip.truth.DepElectrons; | |
| CAF_Blip.truthBlip.NumElectrons =LarBlip.truth.NumElectrons; | |
| CAF_Blip.truthBlip.LeadG4ID =LarBlip.truth.LeadG4ID; | |
| CAF_Blip.truthBlip.LeadG4Index =LarBlip.truth.LeadG4Index; | |
| CAF_Blip.truthBlip.LeadG4PDG =LarBlip.truth.LeadG4PDG; | |
| CAF_Blip.truthBlip.LeadCharge =LarBlip.truth.LeadCharge; | |
| CAF_Blip.truthBlip.Position =LarBlip.truth.Position; | |
| CAF_Blip.truthBlip.Cryostat = LarBlip.truth.Cryostat; | |
| CAF_Blip.truthBlip.TPC = LarBlip.truth.TPC; | |
| CAF_Blip.truthBlip.Time = LarBlip.truth.Time; | |
| CAF_Blip.truthBlip.TimeTick = LarBlip.truth.TimeTick; | |
| CAF_Blip.truthBlip.DriftTime = LarBlip.truth.DriftTime; | |
| CAF_Blip.truthBlip.Energy = LarBlip.truth.Energy; | |
| CAF_Blip.truthBlip.DepElectrons = LarBlip.truth.DepElectrons; | |
| CAF_Blip.truthBlip.NumElectrons = LarBlip.truth.NumElectrons; | |
| CAF_Blip.truthBlip.LeadG4ID = LarBlip.truth.LeadG4ID; | |
| CAF_Blip.truthBlip.LeadG4Index = LarBlip.truth.LeadG4Index; | |
| CAF_Blip.truthBlip.LeadG4PDG = LarBlip.truth.LeadG4PDG; | |
| CAF_Blip.truthBlip.LeadCharge = LarBlip.truth.LeadCharge; | |
| CAF_Blip.truthBlip.Position = LarBlip.truth.Position; |
Outdated
There was a problem hiding this comment.
You can also go
| CAF_Blip.truthBlip.Energy = CAF_Blip.truthBlip.Energy/1000.; //convert to GeV | |
| CAF_Blip.truthBlip.Energy /= 1000.; //convert to GeV |
Outdated
There was a problem hiding this comment.
Pass clusters to this function rather than the full blip objects, since clusters are the only object this function concerns with.
I also suggest that this function be changed to operate on a single cluster, and the loop be moved to the caller.
Outdated
There was a problem hiding this comment.
Since you defined a kNplanes constant, you can use that one directly.
Or, if you have adopted the suggestion of replacing the C array with a std::array, you do have LarBlip.clusters.size(). Both are more readable than the (functionally correct) code in this line.
There was a problem hiding this comment.
Replaced with the kNplanes variable
Outdated
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.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,17 @@ | ||||||||||||||||||
| #ifndef CAF_FILLBLIP_H | ||||||||||||||||||
|
Member
There was a problem hiding this comment. It would be good to add a standard header with file name, brief description and author identification, like in:
Suggested change
The same holds for the |
||||||||||||||||||
| #define CAF_FILLBLIP_H | ||||||||||||||||||
| #include<iostream> | ||||||||||||||||||
|
||||||||||||||||||
| #include<iostream> |
Outdated
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.
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.
Conform to the existing alignment (
[CO-02]).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.
Updated to consistent indent