Skip to content

Feature/adding blip to caf #603

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.

Sign up for GitHub

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

Open
Jjm321814 wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

Feature/adding blip to caf #603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
18b28ec
first draft of adding blips
Jjm321814 Oct 29, 2025
daa5c8e
Added blip library
Jjm321814 Oct 29, 2025
8dfaab0
forgot to add my blip to the vector
Jjm321814 Oct 29, 2025
23c5909
Adding blips to cafmaker
Jjm321814 Oct 30, 2025
6ec65f7
Merge branch 'develop' into feature/AddingBlipToCAF
Nov 10, 2025
3812b04
added blips to cafmaker
Nov 10, 2025
f830082
Merge branch 'develop' into feature/AddingBlipToCAF
Nov 13, 2025
e4cc10e
mistake in the cmakelist
Nov 13, 2025
0c62419
Updating include
Nov 13, 2025
97b9db8
Fixing comments that aren't tabs
Nov 20, 2025
44e03a4
Fixing indentation
Jjm321814 Nov 20, 2025
a44f4c6
part of general tvector3 replacement
Jjm321814 Nov 29, 2025
b6b0f4f
Missed one cmake list update
Jjm321814 Nov 29, 2025
ddda71a
Adjusting fillBlip to account for removed variables
Jjm321814 Nov 30, 2025
05878a3
change to camelcase
Jjm321814 Nov 30, 2025
3b3e41b
Merge branch 'develop' into feature/AddingBlipToCAF
Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions sbncode/CAFMaker/CAFMakerParams.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,10 @@ namespace caf
Comment("Label of CVN scores."),
"cvn"
};
Atom<std::string> fBlipTag { Name("BlipTag"),
Comment("Provide a string to label the blip input"), "blipreco"
};
Comment on lines +637 to +639
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 24, 2025

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]).



};
}
Expand Down
14 changes: 14 additions & 0 deletions sbncode/CAFMaker/CAFMaker_module.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "sbncode/CAFMaker/FillExposure.h"
#include "sbncode/CAFMaker/FillTrigger.h"
#include "sbncode/CAFMaker/Utils.h"
#include "sbncode/CAFMaker/FillBlip.h"

// C/C++ includes
#include <fenv.h>
Expand Down Expand Up @@ -1928,6 +1929,18 @@ void CAFMaker::produce(art::Event& evt) noexcept {
}
}

//Fill blips. art::handle for blips and then call fill blips for each one. Make a vector to hold all of them. I handle for loop in Fill blip
art::Handle<std::vector<blip::Blip>> blipHandle;
std::vector<caf::SRBlip> srblips;
if(evt.getByLabel( fParams.fBlipTag(), blipHandle)) //fill SR blips
{
FillBlip( (*blipHandle), srblips);
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 24, 2025

Choose a reason for hiding this comment

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

Parentheses are redundant here. Just saying.

Suggested change
FillBlip( (*blipHandle), srblips);
FillBlip( *blipHandle, srblips);

(this is a suggestion)

//for(int i=0; i<int(*(blipHandle)->size()); i++)
// {
// auto LarBlips = (*(blipHandle))[i];
// FillBlip( (LarBlips) , srblips);
// }
Comment on lines +1938 to +1942
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 24, 2025

Choose a reason for hiding this comment

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

Please remove obsolete, commented out code:

Suggested change
//for(int i=0; i<int(*(blipHandle)->size()); i++)
// {
// auto LarBlips = (*(blipHandle))[i];
// FillBlip( (LarBlips) , srblips);
// }

}
// collect the TPC slices
std::vector<art::Ptr<recob::Slice>> slices;
std::vector<std::string> slice_tag_suffixes;
Expand Down Expand Up @@ -2605,6 +2618,7 @@ void CAFMaker::produce(art::Event& evt) noexcept {
rec.sbnd_crt_veto = srsbndcrtveto;
rec.opflashes = srflashes;
rec.nopflashes = srflashes.size();
rec.blips = srblips;
rec.sbnd_frames = srsbndframeshiftinfo;
rec.sbnd_timings = srsbndtiminginfo;
rec.soft_trig = srsbndsofttrig;
Expand Down
1 change: 1 addition & 0 deletions sbncode/CAFMaker/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ art_make_library( LIBRARY_NAME sbncode_CAFMaker
sbnobj::Common_Trigger
sbnobj::SBND_CRT
sbnobj::SBND_Timing
sbnobj::SBND_Blip
lardataalg::DetectorInfo
art::Framework_Services_System_TriggerNamesService_service
sbncode_Metadata_MetadataSBN_service
Expand Down
99 changes: 99 additions & 0 deletions sbncode/CAFMaker/FillBlip.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
#include "sbncode/CAFMaker/FillBlip.h"
Copy link
Member

@henrylay97 henrylay97 Nov 19, 2025

Choose a reason for hiding this comment

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

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)
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 24, 2025

Choose a reason for hiding this comment

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

Having a consistent variable naming style would be good too (e.g. LAr_Blip after CAF_Blip — and LAr is usually capitalised that way).
If you change them, change also the declaration in the header file accordingly.

{
int NBlips = LarBlips.size();
for(int iBlip=0; iBlip<NBlips; iBlip++)
{
blip::Blip CurrentBlip = LarBlips[iBlip];
Comment on lines +7 to +10
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 24, 2025

Choose a reason for hiding this comment

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

I think this is equivalent to using a range-for loop:

Suggested change
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.

caf::SRBlip NewBlip;
NewBlip.ID = CurrentBlip.ID;
NewBlip.isValid = CurrentBlip.isValid;
NewBlip.cryostat = CurrentBlip.Cryostat;
NewBlip.TPC = CurrentBlip.TPC;
NewBlip.nPlanes = CurrentBlip.NPlanes;
NewBlip.maxWireSpan = CurrentBlip.MaxWireSpan;
NewBlip.timeTick = CurrentBlip.TimeTick;
NewBlip.time = CurrentBlip.Time;
NewBlip.charge = CurrentBlip.Charge;
NewBlip.energy = CurrentBlip.Energy/1000.; //convert to GeV
NewBlip.energyESTAR = CurrentBlip.EnergyESTAR/1000.; //convert to GeV
NewBlip.energyPSTAR = CurrentBlip.EnergyPSTAR/1000.; //convert to GeV
NewBlip.proxTrkDist = CurrentBlip.ProxTrkDist;
NewBlip.proxTrkID = CurrentBlip.ProxTrkID;
NewBlip.inCylinder = CurrentBlip.inCylinder;
NewBlip.position.SetXYZ(CurrentBlip.Position.X(), CurrentBlip.Position.Y(), CurrentBlip.Position.Z());
NewBlip.sigmaYZ = CurrentBlip.SigmaYZ;
NewBlip.dX = CurrentBlip.dX;
NewBlip.dYZ = CurrentBlip.dYZ;
if(CurrentBlip.truth.ID >= 0 ) //MC Blip
{
FillMCTruthBlip( CurrentBlip, NewBlip );
}
FillBlipRealtedHitCluster( CurrentBlip, NewBlip );
CAF_Blips.push_back(NewBlip);
}

}

void FillMCTruthBlip( blip::Blip& LarBlip, caf::SRBlip &CAF_Blip )
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 24, 2025

Choose a reason for hiding this comment

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

LarBlip is not modified, so it needs to be declared constant:

Suggested change
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.

{
CAF_Blip.truthBlip.ID = LarBlip.truth.ID;
CAF_Blip.truthBlip.cryostat =LarBlip.truth.Cryostat;
CAF_Blip.truthBlip.TPC =LarBlip.truth.TPC;
CAF_Blip.truthBlip.time =LarBlip.truth.Time;
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.SetXYZ(LarBlip.truth.Position.X(), LarBlip.truth.Position.Y(), LarBlip.truth.Position.Z());
CAF_Blip.truthBlip.energy = CAF_Blip.truthBlip.energy/1000.; //convert to GeV
}

void FillBlipRealtedHitCluster(blip::Blip& LarBlip, caf::SRBlip &CAF_Blip)
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 24, 2025

Choose a reason for hiding this comment

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

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.

{
int NumPlanes = sizeof(LarBlip.clusters)/sizeof(LarBlip.clusters[0]);
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 24, 2025

Choose a reason for hiding this comment

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

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.

for(int iPlane=0; iPlane<NumPlanes; iPlane++)
{
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].isMatched = LarBlip.clusters[iPlane].isMatched;
CAF_Blip.clusters[iPlane].deadWireSep = LarBlip.clusters[iPlane].DeadWireSep;
CAF_Blip.clusters[iPlane].cryostat = LarBlip.clusters[iPlane].Cryostat;
CAF_Blip.clusters[iPlane].TPC = LarBlip.clusters[iPlane].TPC;
CAF_Blip.clusters[iPlane].plane = LarBlip.clusters[iPlane].Plane;
CAF_Blip.clusters[iPlane].nHits = LarBlip.clusters[iPlane].NHits;
CAF_Blip.clusters[iPlane].nWires = LarBlip.clusters[iPlane].NWires;
CAF_Blip.clusters[iPlane].ADCs = LarBlip.clusters[iPlane].ADCs;
CAF_Blip.clusters[iPlane].amplitude = LarBlip.clusters[iPlane].Amplitude;
CAF_Blip.clusters[iPlane].charge = LarBlip.clusters[iPlane].Charge;
CAF_Blip.clusters[iPlane].sigmaCharge = LarBlip.clusters[iPlane].SigmaCharge;
CAF_Blip.clusters[iPlane].timeTick = LarBlip.clusters[iPlane].TimeTick;
CAF_Blip.clusters[iPlane].time = LarBlip.clusters[iPlane].Time;
CAF_Blip.clusters[iPlane].startTime = LarBlip.clusters[iPlane].StartTime;
CAF_Blip.clusters[iPlane].endTime = LarBlip.clusters[iPlane].EndTime;
CAF_Blip.clusters[iPlane].timespan = LarBlip.clusters[iPlane].Timespan;
CAF_Blip.clusters[iPlane].RMS = LarBlip.clusters[iPlane].RMS;
CAF_Blip.clusters[iPlane].startWire = LarBlip.clusters[iPlane].StartWire;
CAF_Blip.clusters[iPlane].endWire = LarBlip.clusters[iPlane].EndWire;
CAF_Blip.clusters[iPlane].nPulseTrainHits = LarBlip.clusters[iPlane].NPulseTrainHits;
CAF_Blip.clusters[iPlane].goodnessOfFit = LarBlip.clusters[iPlane].GoodnessOfFit;
CAF_Blip.clusters[iPlane].blipID = LarBlip.clusters[iPlane].BlipID;
CAF_Blip.clusters[iPlane].edepID = LarBlip.clusters[iPlane].EdepID;
//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);
for(auto Wire : LarBlip.clusters[iPlane].Wires) CAF_Blip.clusters[iPlane].wires.push_back(Wire);
for(auto Chan : LarBlip.clusters[iPlane].Chans) CAF_Blip.clusters[iPlane].chans.push_back(Chan);
for(auto G4ID : LarBlip.clusters[iPlane].G4IDs) CAF_Blip.clusters[iPlane].G4IDs.push_back(G4ID);
}
}
}
17 changes: 17 additions & 0 deletions sbncode/CAFMaker/FillBlip.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#ifndef CAF_FILLBLIP_H
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 25, 2025

Choose a reason for hiding this comment

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

It would be good to add a standard header with file name, brief description and author identification, like in:

Suggested change
#ifndef CAF_FILLBLIP_H
/**
* @file sbncode/CAFMaker/FillBlip.h
* @brief CAFMaker utilities for filling "blip" objects into CAF.
* @author Jacob McLaughlin (jmclaughlin2@illinoistech.edu)
/
#ifndef CAF_FILLBLIP_H

The same holds for the .cxx file and, in fact, all other headers and source files. This is not often done in SBN repositories, and it would be better if it were.

#define CAF_FILLBLIP_H
#include<iostream>
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 25, 2025

Choose a reason for hiding this comment

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

Unused header:

Suggested change
#include<iostream>

#include "sbnanaobj/StandardRecord/SRBlip.h"
#include "sbnobj/SBND/Blip/BlipDataTypes.h"

#include <vector>

namespace caf
{

void FillBlip( const std::vector<blip::Blip>& LarBlips, std::vector<caf::SRBlip>& CAF_Blips);
void FillMCTruthBlip(blip::Blip& LarBlip, caf::SRBlip& CAF_Blip );
void FillBlipRealtedHitCluster(blip::Blip& LarBlip, caf::SRBlip& CAF_Blip);
Comment on lines +13 to +14
Copy link
Member

@PetrilloAtWork PetrilloAtWork Nov 25, 2025

Choose a reason for hiding this comment

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

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.

}

#endif