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

Updated to -- "Reconstructed assuming that the blip was coincident with the event trigger"

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.

Updated to SRTrueBlip

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)

The earlier comment suggested eliminating this file so I am trying that first.

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

Fixed both typo of coincident

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

Updated these. Are the ` necessary? I also added them to the simb::MCParticle and raw::channelID references

…truct vs class for blips

Jjm321814 · 2026-01-09T21:06:14Z

Above comments have been addressed and the CAF file gets made and looks generally good.
One wrinkle is that blips can be made with 2 or 3 plane matches. Since we are making the array of SRHitClusters with a fixed size we end up filling in a bunch of default values. That might be a nice feature to build loops in a really standard way for CAF/FlatCAF but it does mean people will run into default values frequently.

…NaN default

Jjm321814 · 2026-01-14T22:35:28Z

Noticed the flat caf wasn't generated correctly so I used mrb z and recompiled.
That lead to issues with the SRVector3D constructor when trying to default to -9999, -9999, -9999.
Instead I am using the blank constructor, filled with NaN values.

Now the compilation finishes and the output CAF + FlatCAF look good.

kjplows · 2026-01-15T21:00:44Z

Calling reviewers to check if these PRs are OK, @PetrilloAtWork , @henrylay97 , @sungbinoh

sungbinoh

Everything looks very good. Thank a lot for the huge efforts!

henrylay97 · 2026-01-16T09:25:31Z

sbnanaobj/StandardRecord/SRBlipHitClust.h

+    float   time            = caf::kSignalingNaN; ///< charge-weighted average hit-peak-times for this hit-cluster [us]
+    float   startTime       = caf::kSignalingNaN; ///< Minimum -1 sigma time of a hit in this cluster [us]
+    float   endTime         = caf::kSignalingNaN; ///< Max +1 sigma time of a hit in this cluster [us]
+    float   timespan        = caf::kSignalingNaN; ///< Hit cluster EndTime - StartTime [us]


In general I would say it's better practice not to store things like this as separate variables given it is fully defined by two other variables. Instead have a member function that returns the difference between the start and end time. There may well be other examples of similar occurrences.

I'm not going to hold the PR up for this, but something to think about for future.

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

Jjm321814 and others added 10 commits November 30, 2025 16:45

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

Jjm321814 added 4 commits January 9, 2026 13:26

Adjusted position initilization

ef4554e

responding to comments on class version, documentation linking, and s…

41645a2

…truct vs class for blips

Missed two name changes

c0fdfa2

Updating checksums

9e6c3e4

Jjm321814 added 5 commits January 14, 2026 15:24

Updated position constructor call

20082f0

Updated position constructor call

aade1bb

Updated position constructor call

6075123

Maybe I have to cast things as float?

9b8cdbc

Other constructor just doesn't work for some reason. I will take the …

990624c

…NaN default

sungbinoh approved these changes Jan 15, 2026

View reviewed changes

henrylay97 approved these changes Jan 16, 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

Conversation

Jjm321814 commented Nov 13, 2025

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!

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!

Jjm321814 commented Jan 9, 2026

Uh oh!

Jjm321814 commented Jan 14, 2026

Uh oh!

kjplows commented Jan 15, 2026

Uh oh!

sungbinoh left a comment

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!

7 participants