Skip to content

Adding SRTrkLikelihoodPID class into the StandardRecord #168

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 11 commits into from
Nov 21, 2025
Merged

Adding SRTrkLikelihoodPID class into the StandardRecord #168

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
aae18a6
Adding SRTrkLikePID for likelihod based PID
sungbinoh Oct 20, 2025
efc6a8d
More updates for TrkLikePID
sungbinoh Oct 20, 2025
c599296
updating SRTrkLikePID to SRTrkLikelihoodPID
sungbinoh Oct 21, 2025
1f8c899
more updates for SRTrkLikelihoodPID
sungbinoh Oct 21, 2025
69f7b45
removing void SRTrkLikelihoodPID::setDefault()
sungbinoh Oct 23, 2025
bacfd71
removing void SRTrkLikelihoodPID::setDefault()
sungbinoh Oct 23, 2025
6907b18
Following Gianluca's comments
sungbinoh Oct 28, 2025
82276e3
Update sbnanaobj/StandardRecord/classes_def.xml
sungbinoh Nov 18, 2025
f26feaf
Update sbnanaobj/StandardRecord/classes_def.xml
sungbinoh Nov 19, 2025
8420aeb
Merge branch 'develop' into feature/sungbino_likepid_into_cafmaker
kjplows Nov 20, 2025
a4d4562
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
2 changes: 2 additions & 0 deletions sbnanaobj/StandardRecord/SRTrack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "sbnanaobj/StandardRecord/SRTrackTruth.h"
#include "sbnanaobj/StandardRecord/SRTrkChi2PID.h"
#include "sbnanaobj/StandardRecord/SRTrkLikelihoodPID.h"
#include "sbnanaobj/StandardRecord/SRTrkMCS.h"
#include "sbnanaobj/StandardRecord/SRTrkRange.h"
#include "sbnanaobj/StandardRecord/SRCRTHitMatch.h"
Expand Down Expand Up @@ -45,6 +46,7 @@ namespace caf
SRVector3D end; ///< End point of track

SRTrkChi2PID chi2pid[3]; ///< Per-plane Chi2 Particle ID
SRTrkLikelihoodPID likepid[3]; ///< Per-plane likelihood-based Particle ID variabeles (lambda = \Sigma -lnL/L_max where \Sigma is done over hits)
Comment thread
PetrilloAtWork marked this conversation as resolved.
Outdated
SRTrackCalo calo[3]; ///< Per-plane Calorimetry information
Plane_t bestplane; ///< Plane index with the most hits. -1 if no calorimetry

Expand Down
34 changes: 34 additions & 0 deletions sbnanaobj/StandardRecord/SRTrkLikelihoodPID.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
////////////////////////////////////////////////////////////////////////
// \file SRTrkLikelihoodPID.cxx
// \brief An SRTrkLikelihoodPID is a high level track ParticlePID object for
// for larana LikelihoodPIDAlg results.
////////////////////////////////////////////////////////////////////////

@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.

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)

@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 following the suggested change.
(added my email tooo :) )

@PetrilloAtWork PetrilloAtWork Nov 18, 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.

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


#include "sbnanaobj/StandardRecord/SRTrkLikelihoodPID.h"

#include <limits>
Comment thread
PetrilloAtWork marked this conversation as resolved.
Outdated

namespace caf
{

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())
Comment thread
PetrilloAtWork marked this conversation as resolved.
Outdated
{ }

SRTrkLikelihoodPID::~SRTrkLikelihoodPID(){ }

void SRTrkLikelihoodPID::setDefault()
{
pdg = -5;
Comment thread
PetrilloAtWork marked this conversation as resolved.
Outdated
pid_ndof = -5;
lambda_muon = -5.0;
lambda_pion = -5.0;
lambda_proton = -5.0;
}
Comment on lines +15 to +30

@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.

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?

@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 , no problem at all!
I've removed the other two places setting the default values, also removing void SRTrkLikelihoodPID::setDefault().

@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.

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.


} // end namespace caf
////////////////////////////////////////////////////////////////////////
29 changes: 29 additions & 0 deletions sbnanaobj/StandardRecord/SRTrkLikelihoodPID.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
////////////////////////////////////////////////////////////////////////
// \file SRTrkLikelihoodPID.h
////////////////////////////////////////////////////////////////////////

@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.

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)

@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.

#ifndef SRTRKLIKELIHOODPID_H
#define SRTRKLIKELIHOODPID_H

namespace caf
{
/// Track PID from dE/dx v. likelihood from dE/dx PDF measured with residual range-based kinetic energy and hit pitch
class SRTrkLikelihoodPID
{
public:

SRTrkLikelihoodPID();
virtual ~SRTrkLikelihoodPID();

@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.

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.

@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.

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


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

@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.

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

@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 the space

float lambda_muon; ///< Sum of -lnL/L_max (muon hypothesis)
float lambda_pion; ///< Sum of -lnL/L_max (charged pion hypothesis)
float lambda_proton; ///< Sum of -lnL/L_max (proton hypothesis)

void setDefault();
};

} // end namespace

#endif // SRTRKLIKELIHOODPID_H
//////////////////////////////////////////////////////////////////////////////
8 changes: 7 additions & 1 deletion sbnanaobj/StandardRecord/classes_def.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@
<version ClassVersion="10" checksum="3777054621"/>
</class>

<class name="caf::SRTrack" ClassVersion="15">
<class name="caf::SRTrack" ClassVersion="17">
<version ClassVersion="17" checksum="4075719476"/>
<version ClassVersion="16" checksum="2837285941"/>
Comment thread
sungbinoh marked this conversation as resolved.
Outdated
<version ClassVersion="15" checksum="175611744"/>
<version ClassVersion="14" checksum="3934033130"/>
<version ClassVersion="13" checksum="522362999"/>
Comment thread
henrylay97 marked this conversation as resolved.
Expand Down Expand Up @@ -165,6 +167,10 @@
<version ClassVersion="10" checksum="3162028429"/>
</class>

<class name="caf::SRTrkLikelihoodPID" ClassVersion="10">
<version ClassVersion="10" checksum="2386830952"/>
</class>

<class name="caf::SRTrkMCS" ClassVersion="11">
<version ClassVersion="11" checksum="2241616344"/>
<version ClassVersion="10" checksum="4182678053"/>
Expand Down