Adding SRTrkLikelihoodPID class into the StandardRecord by sungbinoh · Pull Request #168 · SBNSoftware/sbnanaobj

sungbinoh · 2025-10-20T16:52:13Z

Adding a new StandardRecord SRTrkLikePID for likelihood-based PID variables.
It saves sum of the normalized log likelihood (lambda = -lnL/L_max) for muon, charged pion and proton hypotheses.

Relevant PR in larana is PR41.
Also relevant: SBNSoftware/sbncode#593

henrylay97

Couple of comments, one critical, one not.

You also need to request a review from either @JosiePaton or @PetrilloAtWork as it affects the CAF data format.

sbnanaobj/StandardRecord/classes_def.xml

sbnanaobj/StandardRecord/SRTrkLikePID.cxx

sungbinoh · 2025-10-21T15:26:00Z

Couple of comments, one critical, one not.

You also need to request a review from either @JosiePaton or @PetrilloAtWork as it affects the CAF data format.

Hi @henrylay97 , thank you for the review!
Please have a look to my responses to your comments.
In addition, requesting a review from @PetrilloAtWork too!

henrylay97

Looking good. Thanks for updating the name.

I'm still being picky about defaults sorry!

sbnanaobj/StandardRecord/classes_def.xml

henrylay97 · 2025-10-23T08:55:35Z

sbnanaobj/StandardRecord/SRTrkLikelihoodPID.cxx

+  SRTrkLikelihoodPID::SRTrkLikelihoodPID():
+    pdg(-999),
+    pid_ndof(-99),
+    lambda_muon(std::numeric_limits<float>::signaling_NaN()),
+    lambda_pion(std::numeric_limits<float>::signaling_NaN()),
+    lambda_proton(std::numeric_limits<float>::signaling_NaN())
+  {  }
+
+  SRTrkLikelihoodPID::~SRTrkLikelihoodPID(){  }
+
+  void SRTrkLikelihoodPID::setDefault()
+  {
+    pdg           = -5;
+    pid_ndof      = -5;
+    lambda_muon   = -5.0;
+    lambda_pion   = -5.0;
+    lambda_proton = -5.0;
+  }


Sorry to be picky. I still don't like that we have default values set in 3 places. In the default constructor, in the setDefault function and in the corresponding sbncode PR they get set explicitly when making the object.

I like the defaults used in the default constructor here so my suggestion is to remove the other two instances completely. For this PR this means removing the setDefault() function, it doesn't appear to be used anywhere?

Hi @henrylay97 , no problem at all!
I've removed the other two places setting the default values, also removing void SRTrkLikelihoodPID::setDefault().

I don't like it either, but there was a rationale that aimed to distinguish data that was never initialised (values from constructor), and the data that was defaulted by the user (setDefault()).
Without the former, if code omits an initialisation we end up with random values in the data files, and when we try comparing two versions of data we may see red-herring differences because of that.

PetrilloAtWork

I have left some comments.
@henrylay97 has a (partially) different opinion on initialisation than I do, and you are getting caught in the middle... but uninitialised data is bad, so we need to avoid it.

sbnanaobj/StandardRecord/SRTrack.h

PetrilloAtWork · 2025-10-24T00:11:52Z

sbnanaobj/StandardRecord/SRTrkLikelihoodPID.cxx

+////////////////////////////////////////////////////////////////////////
+// \file    SRTrkLikelihoodPID.cxx
+// \brief   An SRTrkLikelihoodPID is a high level track ParticlePID object for
+//          for larana LikelihoodPIDAlg results.
+////////////////////////////////////////////////////////////////////////


Please turn this into a Doxygen comment and add your name for glory/blame:

Suggested change

////////////////////////////////////////////////////////////////////////

// \file SRTrkLikelihoodPID.cxx

// \brief An SRTrkLikelihoodPID is a high level track ParticlePID object for

// for larana LikelihoodPIDAlg results.

////////////////////////////////////////////////////////////////////////

/**

* @file sbnanaobj/StandardRecord/SRTrkLikelihoodPID.cxx

* @brief An `SRTrkLikelihoodPID` is a high level track ParticlePID object for

* for larana LikelihoodPIDAlg results.

* @author Sungbin Oh

*/

(you can add your e-mail in the @author line if you are comfortable with it)

Updated following the suggested change.
(added my email tooo :) )

Good! If you can fix the repeated "for" in the brief description it's even better.

sbnanaobj/StandardRecord/SRTrkLikelihoodPID.cxx

PetrilloAtWork · 2025-10-24T00:24:48Z

sbnanaobj/StandardRecord/SRTrkLikelihoodPID.cxx

+  SRTrkLikelihoodPID::SRTrkLikelihoodPID():
+    pdg(-999),
+    pid_ndof(-99),
+    lambda_muon(std::numeric_limits<float>::signaling_NaN()),
+    lambda_pion(std::numeric_limits<float>::signaling_NaN()),
+    lambda_proton(std::numeric_limits<float>::signaling_NaN())
+  {  }
+
+  SRTrkLikelihoodPID::~SRTrkLikelihoodPID(){  }
+
+  void SRTrkLikelihoodPID::setDefault()
+  {
+    pdg           = -5;
+    pid_ndof      = -5;
+    lambda_muon   = -5.0;
+    lambda_pion   = -5.0;
+    lambda_proton = -5.0;
+  }


I don't like it either, but there was a rationale that aimed to distinguish data that was never initialised (values from constructor), and the data that was defaulted by the user (setDefault()).
Without the former, if code omits an initialisation we end up with random values in the data files, and when we try comparing two versions of data we may see red-herring differences because of that.

PetrilloAtWork · 2025-10-24T00:27:20Z

sbnanaobj/StandardRecord/SRTrkLikelihoodPID.h

+    public:
+
+      SRTrkLikelihoodPID();
+      virtual ~SRTrkLikelihoodPID();


Remove the destructor, since it does not do anything (CF-151).
Also I would like to know if you copied that idea from anywhere in our code, so that we can fix that too.

Suggested change

virtual ~SRTrkLikelihoodPID();

[edit] I guess that's SRTrkChi2PID.

Removing the destructor in both .h and .cxx.
And yes, I've copied a structure in SRTrkChi2PID .

PetrilloAtWork · 2025-10-24T00:28:01Z

sbnanaobj/StandardRecord/SRTrkLikelihoodPID.h

+      virtual ~SRTrkLikelihoodPID();
+
+      int            pdg;          ///< Likelihood PID pdg
+      int            pid_ndof;    ///< Number of degress of freedom in likelihood PID


Suggested change

int pid_ndof; ///< Number of degress of freedom in likelihood PID

int pid_ndof; ///< Number of degrees of freedom in likelihood PID

Added the space

PetrilloAtWork · 2025-10-24T00:32:56Z

sbnanaobj/StandardRecord/SRTrkLikelihoodPID.h

+////////////////////////////////////////////////////////////////////////
+// \file    SRTrkLikelihoodPID.h
+////////////////////////////////////////////////////////////////////////


Please turn this comment block into Doxygen format:

Suggested change

////////////////////////////////////////////////////////////////////////

// \file SRTrkLikelihoodPID.h

////////////////////////////////////////////////////////////////////////

/**

* @file sbnanaobj/StandardRecord/SRTrkLikelihoodPID.h

* @brief An `SRTrkLikelihoodPID` is a high level track ParticlePID object for

* for larana LikelihoodPIDAlg results.

* @author Sungbin Oh

*/

(like in the class implementation file)

Updated to the suggested change.

sungbinoh · 2025-10-28T16:12:25Z

Hi @PetrilloAtWork , thanks a lot for your review.
I have made updates following your comments.

For default values, I am reviving void SRTrkLikelihoodPID::setDefault() function.
In sbncode , I have added this function in FillReco.cxx so that variables to be initialized with values in the void SRTrkLikelihoodPID::setDefault(). Pinging @henrylay97 for this update!

Co-authored-by: Gianluca Petrillo <petrillo.at.work@gmail.com>

sungbinoh · 2025-11-18T19:23:38Z

Hi @PetrilloAtWork , suggested update for class_def.xml is committed!
Thank you!

PetrilloAtWork

Approved.

henrylay97

One technical change still needed, but with that addressed I'm happy to approve.

sbnanaobj/StandardRecord/classes_def.xml

Co-authored-by: Henry Lay <h.lay@sheffield.ac.uk>

sungbinoh · 2025-11-19T14:24:57Z

One technical change still needed, but with that addressed I'm happy to approve.

Hi @henrylay97 , the suggested update is committed. Thank you!

kjplows · 2025-11-20T22:19:05Z

trigger build LArSoft/lar*@LARSOFT_SUITE_v10_12_02

FNALbuild · 2025-11-20T22:31:47Z

✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

FNALbuild · 2025-11-20T22:36:25Z

✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

FNALbuild · 2025-11-20T22:42:11Z

❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the build SBND phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2025-11-20T22:42:14Z

❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the build ICARUS phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2025-11-20T23:04:16Z

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2025-11-20T23:04:47Z

❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

sungbinoh added 2 commits October 20, 2025 10:33

Adding SRTrkLikePID for likelihod based PID

aae18a6

More updates for TrkLikePID

efc6a8d

sungbinoh requested a review from henrylay97 October 20, 2025 16:52

sungbinoh self-assigned this Oct 20, 2025

sungbinoh added the enhancement New feature or request label Oct 20, 2025

This was referenced Oct 20, 2025

Feature/sungbino likepid into cafmaker SBNSoftware/sbncode#593

Merged

adding TrackLikePidLabel: to cafmakerjob fcl files SBNSoftware/sbndcode#853

Merged

henrylay97 reviewed Oct 21, 2025

View reviewed changes

sbnanaobj/StandardRecord/classes_def.xml Show resolved Hide resolved

sbnanaobj/StandardRecord/SRTrkLikePID.cxx Outdated Show resolved Hide resolved

sungbinoh changed the title ~~Feature/sungbino likepid into cafmaker~~ Adding SRTrkLikelihoodPID class into the StandardRecord Oct 21, 2025

sungbinoh added 2 commits October 21, 2025 10:00

updating SRTrkLikePID to SRTrkLikelihoodPID

c599296

more updates for SRTrkLikelihoodPID

1f8c899

sungbinoh requested a review from PetrilloAtWork October 21, 2025 15:24

henrylay97 reviewed Oct 23, 2025

View reviewed changes

sungbinoh added 2 commits October 23, 2025 08:58

removing void SRTrkLikelihoodPID::setDefault()

69f7b45

removing void SRTrkLikelihoodPID::setDefault()

bacfd71

PetrilloAtWork requested changes Oct 24, 2025

View reviewed changes

Following Gianluca's comments

6907b18

kjplows added this to SBN software development Nov 4, 2025

kjplows moved this to Partially reviewed in SBN software development Nov 4, 2025

Update sbnanaobj/StandardRecord/classes_def.xml

82276e3

Co-authored-by: Gianluca Petrillo <petrillo.at.work@gmail.com>

PetrilloAtWork mentioned this pull request Nov 18, 2025

Standardization of caf::SRTrkChi2PID form #175

Open

PetrilloAtWork approved these changes Nov 19, 2025

View reviewed changes

henrylay97 requested changes Nov 19, 2025

View reviewed changes

sbnanaobj/StandardRecord/classes_def.xml Outdated Show resolved Hide resolved

Update sbnanaobj/StandardRecord/classes_def.xml

f26feaf

Co-authored-by: Henry Lay <h.lay@sheffield.ac.uk>

henrylay97 approved these changes Nov 19, 2025

View reviewed changes

Merge branch 'develop' into feature/sungbino_likepid_into_cafmaker

8420aeb

kjplows moved this from Partially reviewed to Testing in SBN software development Nov 20, 2025

kjplows moved this from Testing to Urgent checks in SBN software development Nov 20, 2025

kjplows moved this from Urgent checks to To merge in SBN software development Nov 20, 2025

kjplows added this to SBND 2025 Fall Production Nov 20, 2025

kjplows moved this to RM Approved in SBND 2025 Fall Production Nov 20, 2025

Merge branch 'develop' into feature/sungbino_likepid_into_cafmaker

a4d4562

kjplows merged commit f1ff49c into SBNSoftware:develop Nov 21, 2025

github-project-automation bot moved this from To merge to Done in SBN software development Nov 21, 2025

kjplows moved this from Done to Recently done in SBN software development Nov 21, 2025

nathanielerowe moved this from RM Approved to In tagged release in SBND 2025 Fall Production Nov 22, 2025

kjplows moved this from Recently done to Done in SBN software development Dec 8, 2025

kjplows moved this from Done to 2025 PRs in SBN software development Jan 16, 2026

kjplows added this to PR archaeology Jan 16, 2026

github-project-automation bot moved this from Todo to Done in PR archaeology Jan 16, 2026

github-project-automation bot moved this to Todo in PR archaeology Jan 16, 2026

kjplows added this to the v10_14_00 milestone Jan 16, 2026

	int pid_ndof; ///< Number of degress of freedom in likelihood PID
	int pid_ndof; ///< Number of degrees of freedom in likelihood PID

-////////////////////////////////////////////////////////////////////////
-// \file    SRTrkLikelihoodPID.h
-////////////////////////////////////////////////////////////////////////
+/**
+ * @file   sbnanaobj/StandardRecord/SRTrkLikelihoodPID.h
+ * @brief  An `SRTrkLikelihoodPID` is a high level track ParticlePID object for
+ *         for larana LikelihoodPIDAlg results.
+ * @author Sungbin Oh
+ */

Conversation

sungbinoh commented Oct 20, 2025 • edited by PetrilloAtWork Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henrylay97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sungbinoh commented Oct 21, 2025

Uh oh!

henrylay97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sungbinoh commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sungbinoh commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

henrylay97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sungbinoh commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjplows commented Nov 20, 2025

Uh oh!

FNALbuild commented Nov 20, 2025

Uh oh!

FNALbuild commented Nov 20, 2025

Uh oh!

FNALbuild commented Nov 20, 2025

Uh oh!

FNALbuild commented Nov 20, 2025

Uh oh!

FNALbuild commented Nov 20, 2025

Uh oh!

FNALbuild commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

sungbinoh commented Oct 20, 2025 •

edited by PetrilloAtWork

Loading

sungbinoh commented Oct 28, 2025 •

edited

Loading

sungbinoh commented Nov 18, 2025 •

edited

Loading

sungbinoh commented Nov 19, 2025 •

edited

Loading