Feature/adding blip to caf #173

Jjm321814 · 2025-11-13T15:18:08Z

Added classes to hold blip information in CAF format.
This results in the normal amount of CAF code duplication and replaces a few std::map and std::set objects in the LArSoft side with vectors.
This PR is independent of the associated ones in sbnobj, sbndcode, and sbncode.

Jjm321814 · 2025-11-13T15:23:33Z

Adding a comment to each of these to track all 4 related PR.
sbncode: SBNSoftware/sbncode#603
sbndcode: SBNSoftware/sbndcode#871
sbnobj: SBNSoftware/sbnobj#155
sbnanaobj: #173

The changes to sbnobj, and sbnanaobj are fully independent of any other changes, so they can be approved first.

sbndcode changes rely on sbnobj, so it will have to wait for the first approval. A later simple PR will delete the (now duplicated) class files in the BlipUtils folder here

sbncode changes rely on both sbnobj and sbnanaobj, so that will have to wait for both of the first two approvals.

henrylay97 · 2025-11-19T18:25:45Z

sbnanaobj/StandardRecord/classes_def.xml

+   <version ClassVersion="17" checksum="1651571235"/>
+   <version ClassVersion="16" checksum="3738947418"/>
+   <version ClassVersion="15" checksum="2636549707"/>


Only need to retain the most recent checksum and set it to version 15

Updated to reflect this change

Surprisingly the changes to default values don't seem to impact the checksum?

henrylay97 · 2025-11-19T18:26:41Z

sbnanaobj/StandardRecord/SRBlip.cxx

+{
+    SRBlip::SRBlip()
+    {
+        ID              = -9;         // Blip ID / index


@PetrilloAtWork and @JosiePaton can comment but I believe there are some pre-defined defaults in CAF-land? Plus I believe -5 is often used as long as it is unphysical.

Same comment for other objs

Updated most values to -5. For time of blips/hits -5 is a physical value, so I left it at -999. For the time tick I think index 0 is time 0, but I am not sure so I left that at -999 as well.

henrylay97 · 2025-11-19T18:27:16Z

sbnanaobj/StandardRecord/SRBlip.h

+        int       ID              = -9;         // Blip ID / index
+        bool      isValid         = false;      // Blip passes basic checks
+        int       Cryostat        = -9;         // Cryostat
+        int       TPC             = -9;         // TPC
+        int       NPlanes         = -9;         // Num. matched planes
+        int       MaxWireSpan     = -9;         // Maximum span of wires on any plane cluster
+        float     TimeTick        = -999;       // Readout time [ticks]
+        float     Time            = -999;       // Drift time [us]
+        float     Charge          = -9;         // Charge on calorimetry plane
+        float     Energy          = -999;       // Energy (const dE/dx, fcl-configurable) [GeV]
+        float     EnergyESTAR     = -999;       // Energy (ESTAR method from ArgoNeuT) [GeV]
+        float     EnergyPSTAR     = -999;       // Energy (PSTAR method similar with ESTAR method from ArgoNeuT) [GeV]
+        float     ProxTrkDist     = -9;         // Distance to cloest track
+        int       ProxTrkID       = -9;         // ID of closest track
+        bool      inCylinder      = false;      // Is it in a cone/cylinder region? 
+        SRVector3D  Position;                     // 3D position TVector3
+        float     SigmaYZ         = -9.;        // Uncertainty in YZ intersect [cm]
+        float     dX              = -9;         // Equivalent length along drift direction [cm] 
+        float     dYZ             = -9;         // Approximate length scale in YZ space [cm]
+        SRBlipHitClust clusters[kNplanes]; // Plane/cluster-specific information
+        SRBlipTrueBlip truthBlip; // Truth-matched energy deposition


Defaults are set in the constructor, can remove them from here.

Same comment to other objs

For blips I removed these from here. The others are just structs so I kept the values in the header file.

PetrilloAtWork

I left some comments on how to make the new classes more closely follow CAF standards.

I am left wondering though: this is an analysis format, and it seems to me that the vast majority of the information in these objects is unlikely to be used in an analysis — it feels more like input for reconstruction.

PetrilloAtWork · 2025-11-21T05:18:28Z

sbnanaobj/StandardRecord/classes_def.xml

 <lcgdict>
  <class name="caf::StandardRecord" ClassVersion="15">
-   <version ClassVersion="15" checksum="1805006132"/>
+   <version ClassVersion="15" checksum="1651571235"/>


There is a change of checksum, which is Bad.
The recommendation is to restore the old line and then rebuild:

Suggested change

<version ClassVersion="15" checksum="1651571235"/>

At that point, ROOT will add another checksum and bump the version, and that new classes_def.xml file should be committed for this PR.

PetrilloAtWork · 2025-11-21T05:19:58Z

sbnanaobj/StandardRecord/SRBlip.cxx

+////////////////////////////////////////////////////////////////////////
+// \file    SRBlip.cxx
+// \brief   SRBlip object for localized energy deposits in bulk LAr
+// \author  $Author: jmclaughlin2@illinoistech.edu
+////////////////////////////////////////////////////////////////////////


Convert this header into Doxygen format.

Suggested change

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

// \file SRBlip.cxx

// \brief SRBlip object for localized energy deposits in bulk LAr

// \author $Author: jmclaughlin2@illinoistech.edu

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

/**

* @file SRBlip.h

* @brief SRBlip object for localized energy deposits in bulk LAr

* @author jmclaughlin2@illinoistech.edu

*/

(the full name would be also nice to add in the @author line, if you are ok with that)

sbnanaobj/StandardRecord/SRBlip.h

PetrilloAtWork · 2025-11-21T06:16:44Z

sbnanaobj/StandardRecord/SRBlipHitClust.h

+    int     NHits           = -5;
+    int     NWires          = -5;


If there is a relation between these counts and HitIDs/Wires vector size, write it explicitly.

Added comments through this file. Now the vectors include a detail on their size matching these int

sbnanaobj/StandardRecord/SRBlipHitClust.h

sbnanaobj/StandardRecord/SRBlipTrueBlip.h

Jjm321814 · 2025-12-01T00:35:11Z

Okay I think all the comments across each PR is now at least partly addressed, and the code compiles and seems to make sensible output when I run it.

kjplows · 2025-12-14T08:17:18Z

Hi @PetrilloAtWork , it looks like Jacob has implemented all the changes requested. How does the PR look now? Thanks!

PetrilloAtWork

classes_def.xml needs to be fixes.
I am recommending the renaming of one of the classes.
I have follow up on a couple of the comments, and added a few more.

PetrilloAtWork · 2026-01-08T19:15:27Z

sbnanaobj/StandardRecord/classes_def.xml

+  <class name="caf::SRBlip" ClassVersion="11">
+   <version ClassVersion="11" checksum="2097549339"/>
+   <version ClassVersion="10" checksum="3364148027"/>
+  </class>
+
+  <class name="caf::SRBlipHitClust" ClassVersion="12">
+   <version ClassVersion="12" checksum="1544666038"/>
+   <version ClassVersion="11" checksum="3137900176"/>
+   <version ClassVersion="10" checksum="1233871129"/>
+  </class>
+
+  <class name="caf::SRBlipTrueBlip" ClassVersion="11">
+   <version ClassVersion="11" checksum="451846977"/>
+   <version ClassVersion="10" checksum="984259034"/>
+  </class>


A single PR should increment a class version only by one unit.
These classes also list intermediate versions.
One way is to remove the checksums and let ROOT Cling do its job again (by building the library twice):

Suggested change

<class name="caf::SRBlip" ClassVersion="11">

<version ClassVersion="11" checksum="2097549339"/>

<version ClassVersion="10" checksum="3364148027"/>

</class>

<class name="caf::SRBlipHitClust" ClassVersion="12">

<version ClassVersion="12" checksum="1544666038"/>

<version ClassVersion="11" checksum="3137900176"/>

<version ClassVersion="10" checksum="1233871129"/>

</class>

<class name="caf::SRBlipTrueBlip" ClassVersion="11">

<version ClassVersion="11" checksum="451846977"/>

<version ClassVersion="10" checksum="984259034"/>

</class>

<class name="caf::SRBlip" ClassVersion="10" />

<class name="caf::SRBlipHitClust" ClassVersion="10" />

<class name="caf::SRBlipTrueBlip" ClassVersion="10" />

An alternative is to fix it by hand by keeping the latest checksum but naming it "version 10":

Suggested change

<class name="caf::SRBlip" ClassVersion="11">

<version ClassVersion="11" checksum="2097549339"/>

<version ClassVersion="10" checksum="3364148027"/>

</class>

<class name="caf::SRBlipHitClust" ClassVersion="12">

<version ClassVersion="12" checksum="1544666038"/>

<version ClassVersion="11" checksum="3137900176"/>

<version ClassVersion="10" checksum="1233871129"/>

</class>

<class name="caf::SRBlipTrueBlip" ClassVersion="11">

<version ClassVersion="11" checksum="451846977"/>

<version ClassVersion="10" checksum="984259034"/>

</class>

<class name="caf::SRBlip" ClassVersion="10">

<version ClassVersion="10" checksum="2097549339"/>

</class>

<class name="caf::SRBlipHitClust" ClassVersion="10">

<version ClassVersion="10" checksum="1544666038"/>

</class>

<class name="caf::SRBlipTrueBlip" ClassVersion="10">

<version ClassVersion="10" checksum="451846977"/>

</class>

It would be good to rebuild after this change too, for added safety.

PetrilloAtWork · 2026-01-08T19:22:02Z

sbnanaobj/StandardRecord/SRBlip.h

+        /*!
+          please note the blip X position is unreliable, so these distance and 3-d position derived variables may be incorrect
+        */
+        SRVector3D  position;                         ///< 3D position vector. Reconstructed with wrong t0! [cm]


Aggregate initialisation should work here:

Suggested change

SRVector3D position; ///< 3D position vector. Reconstructed with wrong t0! [cm]

SRVector3D position { -999., -999., -999. }; ///< 3D position vector. Reconstructed with wrong t0! [cm]

(if compiler complains about narrowing, you may need to explicitly use float constants, like in -999.0f).
This will allow the removal of the default constructor.
Also, z -999 [cm] is almost within ICARUS detector, and it's not a particularly crazy value. If you don't like the default (NaN), use a more extreme value (e.g. -9999. is well outside SBN-FD).

PetrilloAtWork · 2026-01-08T19:32:43Z

sbnanaobj/StandardRecord/SRBlip.h

+    class SRBlip
+    {
+    public:


Hmm... that means that the scores would be the only private members, and you would need a specific accessor only for that. That makes the data members asymmetric. Alternatively, you'd have to set all data private and provide one accessor per data member, which is tedious.
I would rather suggest that the new scores be a public data member of a new custom type, say, SRBlipScores (side effect is that this one can still be a struct), and that the sorting be handled within the custom type.

PetrilloAtWork · 2026-01-08T19:36:50Z

sbnanaobj/StandardRecord/SRBlip.h

+        /*!
+          please note the blip X position is unreliable, so these distance and 3-d position derived variables may be incorrect
+        */
+        SRVector3D  position;                         ///< 3D position vector. Reconstructed with wrong t0! [cm]


"wrong t0" is maybe too strong... "assuming the blip was coincident with event trigger", for the slice happening at trigger time that ends up the right time (provided that the blip is not a delayed product of some decay of long-wandering particle).
I can't think of a better way to say that compactly though, so if you want to keep it as is, I am also fine with that.

PetrilloAtWork · 2026-01-08T19:47:50Z

sbnanaobj/StandardRecord/SRBlipTrueBlip.h

+   * That blip reconstruction applies cuts to overall blip size/spread
+   * A single TrueBlip will be constructed for energy depositions within TrueBlipMergeDist (fcl set 0.3 cm by default)
+  */
+  struct SRBlipTrueBlip {


Something that I missed from the previous run: SRBlipTrueBlip... isn't there too much Blip in it? SRTrueBlip should be descriptive enough and consistent with, e.g., SRTrueParticle.
Please seriously consider renaming this one.

PetrilloAtWork · 2026-01-08T19:56:57Z

sbnanaobj/StandardRecord/SRBlip.cxx

@@ -0,0 +1,20 @@
+/**
+ * @file    SRBlip.h


This is the wrong file name (Doxygen will go crazy):

Suggested change

* @file SRBlip.h

* @file SRBlip.cxx

(and yes, I did mistype it in my suggestion)

PetrilloAtWork · 2026-01-08T20:32:14Z

sbnanaobj/StandardRecord/SRBlip.h

+        /*!
+          for properly flash matched out-of-time tracks this distance will be wrong! The blips have no such flash matching ability as of yet
+        */
+        int       proxTrkID=caf::kUninitializedInt;   ///< index of the of closest track, assuming the blip was concident with event trigger


Typo:

Suggested change

int proxTrkID=caf::kUninitializedInt; ///< index of the of closest track, assuming the blip was concident with event trigger

int proxTrkID=caf::kUninitializedInt; ///< index of the of closest track, assuming the blip was coincident with event trigger

PetrilloAtWork · 2026-01-08T20:37:39Z

sbnanaobj/StandardRecord/SRBlip.h

+        float     EnergyESTAR;       // Energy (ESTAR method from ArgoNeuT) [GeV]
+        float     EnergyPSTAR;       // Energy (PSTAR method similar with ESTAR method from ArgoNeuT) [GeV]
+        float     ProxTrkDist;         // Distance to cloest track
+        int       ProxTrkID;         // ID of closest track


So maybe just add in parentheses " (recob::Track::ID())" to the documentation?

PetrilloAtWork · 2026-01-09T00:58:30Z

sbnanaobj/StandardRecord/SRBlipHitClust.h

+    std::vector<int>    hitIDs; ///< Index of the recob::hit objects making up this cluster. Size should match nHits
+    std::vector<int>    wires;  ///<  Set of geo::wireIDs contributing hits to this cluster. Size should match nWires


Using the exact name if the classes will help Doxygen cross-link.

Suggested change

std::vector<int> hitIDs; ///< Index of the recob::hit objects making up this cluster. Size should match nHits

std::vector<int> wires; ///< Set of geo::wireIDs contributing hits to this cluster. Size should match nWires

std::vector<int> hitIDs; ///< Index of the `recob::Hit` objects making up this cluster. Size should match `nHits`.

std::vector<int> wires; ///< Set of `geo::WireID` contributing hits to this cluster. Size should match `nWires`.

Jjm321814 and others added 30 commits October 28, 2025 12:42

First try at adding blip data structure

528d5cc

Adjusted unit

1b1114b

Updated includes

62f6efb

Missing ;

06497ed

Missing #endif

a1441e6

added destructor def

8563371

Putting blip entry in standard record class

9c71906

Updated classes_def entries

1540dc9

removing typedef

5151a16

Removed destructor

7f9599b

ideas that didn't fix compilation

4442c5d

trying to simplify my SRBlip object

4563cf9

trying different checksum stuff

7f06505

Putting back destructor

fcf5303

guessing at some checksums

780bb3a

Trying to simplify build

a5088b8

replacing tvector3d with SRVector3D:

a8668d0

remove guess at checksum

3d28c4d

finally got mrb to give me the magic number

7e01d42

putting back the structs in blips

7fb37cc

Replaced TVector3d

142a08f

Trying one struct at a time

4665846

Changed some name scheme and includes

4bb677b

Another try

7ad0a53

trying other indef

1560a43

Adding a empty line

bbcffe8

Removing all pair information

041a6eb

Maybe its mad about the array

ebdbf7b

Removing both structs but keeping include

89317d4

The machine has given us new checksums

8684ef7

This was referenced Nov 13, 2025

Feature/adding blip to caf SBNSoftware/sbndcode#871

Open

Feature/adding blip to caf SBNSoftware/sbnobj#155

Open

Jjm321814 requested review from JosiePaton and PetrilloAtWork November 14, 2025 19:48

kjplows added this to SBN software development Nov 18, 2025

kjplows moved this to Open pull requests in SBN software development Nov 18, 2025

JosiePaton approved these changes Nov 19, 2025

View reviewed changes

henrylay97 reviewed Nov 19, 2025

View reviewed changes

Changing default values

04018f6

PetrilloAtWork requested changes Nov 21, 2025

View reviewed changes

Jjm321814 and others added 13 commits November 30, 2025 16:30

Removed depreciated variables and moved over comments

6b744a9

missed one save

009e568

Trying a different syntax for construction

849aaeb

Trying a different syntax for construction

013839e

Trying a different syntax for construction

f12e772

Changed defaults

f5a5bca

Changed to camelcase

a610c83

Change the array

368fb1a

Comment alignment

8e880ee

Alignment

8486049

fixing class versions

8b76963

Merge branch 'develop' into feature/AddingBlipToCAF

87333a5

Updated class version

8ce7a3a

kjplows moved this from Open pull requests to Partially reviewed in SBN software development Dec 14, 2025

Jjm321814 added 2 commits December 15, 2025 15:26

Minor updates to const var location and comments

b96866a

Minor updates to const var

b45980c

PetrilloAtWork requested changes Jan 9, 2026

View reviewed changes

	SRVector3D position; ///< 3D position vector. Reconstructed with wrong t0! [cm]
	SRVector3D position { -999., -999., -999. }; ///< 3D position vector. Reconstructed with wrong t0! [cm]

	int proxTrkID=caf::kUninitializedInt; ///< index of the of closest track, assuming the blip was concident with event trigger
	int proxTrkID=caf::kUninitializedInt; ///< index of the of closest track, assuming the blip was coincident with event trigger

		std::vector<int> hitIDs; ///< Index of the recob::hit objects making up this cluster. Size should match nHits
		std::vector<int> wires; ///< Set of geo::wireIDs contributing hits to this cluster. Size should match nWires

Feature/adding blip to caf #173

Are you sure you want to change the base?

Feature/adding blip to caf #173

Uh oh!

Conversation

Jjm321814 commented Nov 13, 2025

Uh oh!

Jjm321814 commented Nov 13, 2025

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jjm321814 commented Dec 1, 2025

Uh oh!

kjplows commented Dec 14, 2025

Uh oh!

PetrilloAtWork left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants