Skip to content

Feature/sungbino likepid into cafmaker #593

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

Merged
kjplows merged 12 commits into from
Nov 21, 2025
Merged

Feature/sungbino likepid into cafmaker #593

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9bea391
adding TrackLikePidLabel in CAFMakerParams.h
sungbinoh Oct 20, 2025
2af2c08
updating FillReco for likelihood pid
sungbinoh Oct 20, 2025
3eeef15
update CAFMaker_module.cc for likelihood pid
sungbinoh Oct 20, 2025
2af891e
likepid default values to -5.
sungbinoh Oct 21, 2025
2651da1
like pid lambda default value to -99999.
sungbinoh Oct 21, 2025
ff96233
updating SRTrkLikePID to SRTrkLikelihoodPID
sungbinoh Oct 21, 2025
264bd4f
removing defaulting part for likelihood pid
sungbinoh Oct 23, 2025
4c47688
Following Gianluca's comments
sungbinoh Oct 28, 2025
ad25a04
adding setDefault for likelihood pid
sungbinoh Oct 28, 2025
c48e4bf
Merge branch 'develop' into feature/sungbino_likepid_into_cafmaker
kjplows Nov 4, 2025
4346c58
Merge branch 'develop' into feature/sungbino_likepid_into_cafmaker
kjplows Nov 20, 2025
4309dfe
Merge branch 'develop' into feature/sungbino_likepid_into_cafmaker
kjplows Nov 21, 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
6 changes: 6 additions & 0 deletions sbncode/CAFMaker/CAFMakerParams.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,12 @@ namespace caf
"pandoraPid"
};

Atom<string> TrackLikePidLabel {
Name("TrackLikePidLabel"),
Comment("Base label of track likelihood particle-id producer."),
"pandoraLikePid"
};

Atom<string> TrackScatterClosestApproachLabel {
Name("TrackScatterClosestApproachLabel"),
Comment("Base label of track track scatter closestapproach producer."),
Expand Down
7 changes: 7 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 @@ -2125,6 +2125,10 @@ void CAFMaker::produce(art::Event& evt) noexcept {
FindManyPStrict<anab::ParticleID>(slcTracks, evt,
fParams.TrackChi2PidLabel() + slice_tag_suff);

art::FindManyP<anab::ParticleID> fmLikePID =
FindManyPStrict<anab::ParticleID>(slcTracks, evt,
fParams.TrackLikePidLabel() + slice_tag_suff);

art::FindManyP<sbn::ScatterClosestApproach> fmScatterClosestApproach =
FindManyPStrict<sbn::ScatterClosestApproach>(slcTracks, evt,
fParams.TrackScatterClosestApproachLabel() + slice_tag_suff);
Expand Down Expand Up @@ -2416,6 +2420,9 @@ void CAFMaker::produce(art::Event& evt) noexcept {
if (fmChi2PID.isValid()) {
FillTrackChi2PID(fmChi2PID.at(iPart), trk);
}
if (fmLikePID.isValid()) {
FillTrackLikePID(fmLikePID.at(iPart), trk);
Comment thread
henrylay97 marked this conversation as resolved.
}
if (fmScatterClosestApproach.isValid() && fmScatterClosestApproach.at(iPart).size()==1) {
FillTrackScatterClosestApproach(fmScatterClosestApproach.at(iPart).front(), trk);
}
Expand Down
47 changes: 47 additions & 0 deletions sbncode/CAFMaker/FillReco.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,53 @@ namespace caf
}
}

void FillPlaneLikePID(const anab::ParticleID &particle_id, caf::SRTrkLikePID &srlikepid) {

// Assign dummy values.

srlikepid.lambda_muon = 0.;
srlikepid.lambda_pion = 0.;
srlikepid.lambda_proton = 0.;
srlikepid.pid_ndof = 0;

@henrylay97 henrylay97 Oct 21, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we change from the previous defaults here?

The object fills the defaults as -5

@sungbinoh sungbinoh Oct 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @henrylay97 ,
Thank you for the review!
That is a good catch. I did kind of no-brain copy of chi2pid part.
I agree that the default value should be distinguishable from the actual values.
Updating default values for lambdas to -9999. and ndof to -5!


@henrylay97 henrylay97 Oct 23, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would remove this completely.

The values will be set to be the defaults by the default constructor anyway so let's avoid multiple points of maintenance. (See my comment here for more thoughts SBNSoftware/sbnanaobj#168 (comment))

@sungbinoh sungbinoh Oct 23, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @henrylay97 , I've removed these lines following your comment in the SBNSoftware/sbnanaobj#168 (comment). Thank you!

// Loop over algorithm scores and extract the ones we want.
// Get the ndof from any likelihood pid algorithm

std::vector<anab::sParticleIDAlgScores> AlgScoresVec = particle_id.ParticleIDAlgScores();

@PetrilloAtWork PetrilloAtWork Oct 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Capture it by reference, avoid copies ([CF-111]):

Suggested change
std::vector<anab::sParticleIDAlgScores> AlgScoresVec = particle_id.ParticleIDAlgScores();
std::vector<anab::sParticleIDAlgScores> const& AlgScoresVec = particle_id.ParticleIDAlgScores();

@sungbinoh sungbinoh Oct 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to the suggested change.

for (size_t i_algscore=0; i_algscore<AlgScoresVec.size(); i_algscore++){
anab::sParticleIDAlgScores AlgScore = AlgScoresVec.at(i_algscore);

@PetrilloAtWork PetrilloAtWork Oct 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Avoid copying the objects, and also compact the notation:

Suggested change
for (size_t i_algscore=0; i_algscore<AlgScoresVec.size(); i_algscore++){
anab::sParticleIDAlgScores AlgScore = AlgScoresVec.at(i_algscore);
for (anab::sParticleIDAlgScores const& AlgScore: AlgScoresVec){

@sungbinoh sungbinoh Oct 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to the suggested change.

if (AlgScore.fAlgName == "Likelihood"){
if (TMath::Abs(AlgScore.fAssumedPdg) == 13) { // lambda_mu
srlikepid.lambda_muon = AlgScore.fValue;
srlikepid.pid_ndof = AlgScore.fNdf;
}
else if (TMath::Abs(AlgScore.fAssumedPdg) == 211) { // lambda_pi
srlikepid.lambda_pion = AlgScore.fValue;
srlikepid.pid_ndof = AlgScore.fNdf;
}
else if (TMath::Abs(AlgScore.fAssumedPdg) == 2212) { // lambda_pr
srlikepid.lambda_proton = AlgScore.fValue;
srlikepid.pid_ndof = AlgScore.fNdf;
}

@PetrilloAtWork PetrilloAtWork Oct 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since the cases are known integral numbers, it's more efficient to use switch:

Suggested change
if (TMath::Abs(AlgScore.fAssumedPdg) == 13) { // lambda_mu
srlikepid.lambda_muon = AlgScore.fValue;
srlikepid.pid_ndof = AlgScore.fNdf;
}
else if (TMath::Abs(AlgScore.fAssumedPdg) == 211) { // lambda_pi
srlikepid.lambda_pion = AlgScore.fValue;
srlikepid.pid_ndof = AlgScore.fNdf;
}
else if (TMath::Abs(AlgScore.fAssumedPdg) == 2212) { // lambda_pr
srlikepid.lambda_proton = AlgScore.fValue;
srlikepid.pid_ndof = AlgScore.fNdf;
}
switch (std::abs(AlgScore.fAssumedPdg)) {
case 13: // lambda_mu
srlikepid.lambda_muon = AlgScore.fValue;
srlikepid.pid_ndof = AlgScore.fNdf;
break;
case 211: // lambda_pi
srlikepid.lambda_pion = AlgScore.fValue;
srlikepid.pid_ndof = AlgScore.fNdf;
break;
case 2212: // lambda_pr
srlikepid.lambda_proton = AlgScore.fValue;
srlikepid.pid_ndof = AlgScore.fNdf;
break;
}

@sungbinoh sungbinoh Oct 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to the suggested change.

}
}
}

void FillTrackLikePID(const std::vector<art::Ptr<anab::ParticleID>> particleIDs,

@PetrilloAtWork PetrilloAtWork Oct 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pass the particle ID pointer objects by reference ([CF-112]):

Suggested change
void FillTrackLikePID(const std::vector<art::Ptr<anab::ParticleID>> particleIDs,
void FillTrackLikePID(const std::vector<art::Ptr<anab::ParticleID>>& particleIDs,

@sungbinoh sungbinoh Oct 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added &.

caf::SRTrack& srtrack,
bool allowEmpty)
{
// get the particle ID's
for (unsigned i = 0; i < particleIDs.size(); i++) {
const anab::ParticleID &particle_id = *particleIDs[i];

@PetrilloAtWork PetrilloAtWork Oct 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can compact this avoiding i:

Suggested change
for (unsigned i = 0; i < particleIDs.size(); i++) {
const anab::ParticleID &particle_id = *particleIDs[i];
for (art::Ptr<anab::ParticleID> const& pidPtr: particleIDs) {
const anab::ParticleID &particle_id = *pidPtr;

@sungbinoh sungbinoh Oct 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to the suggested change.

if (particle_id.PlaneID()) {
unsigned plane_id = particle_id.PlaneID().Plane;
assert(plane_id < 3);
FillPlaneLikePID(particle_id, srtrack.likepid[plane_id]);
}
}
}

void FillTrackPlaneCalo(const anab::Calorimetry &calo,
const std::vector<art::Ptr<recob::Hit>> &hits,
bool fill_calo_points, float fillhit_rrstart, float fillhit_rrend,
Expand Down
4 changes: 4 additions & 0 deletions sbncode/CAFMaker/FillReco.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,10 @@ namespace caf
void FillTrackChi2PID(const std::vector<art::Ptr<anab::ParticleID>> particleIDs,
caf::SRTrack& srtrack,
bool allowEmpty = false);
void FillPlaneLikePID(const anab::ParticleID &particle_id, caf::SRTrkLikePID &srlikepid);
void FillTrackLikePID(const std::vector<art::Ptr<anab::ParticleID>> particleIDs,

@PetrilloAtWork PetrilloAtWork Oct 24, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You forgot the reference:

Suggested change
void FillTrackLikePID(const std::vector<art::Ptr<anab::ParticleID>> particleIDs,
void FillTrackLikePID(const std::vector<art::Ptr<anab::ParticleID>>& particleIDs,

@sungbinoh sungbinoh Oct 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added &.

caf::SRTrack& srtrack,
bool allowEmpty = false);

void FillTrackPlaneCalo(const anab::Calorimetry &calo,
const std::vector<art::Ptr<recob::Hit>> &hits,
Expand Down