Feature/adding blip to caf #871

Jjm321814 · 2025-11-13T15:13:26Z

Moved the blip-related structs and classes to sbnobj.
This PR must not be approved until SBNSoftware/sbnobj#155 is approved/released! Otherwise it will break blip production.
I also had to make a few CRT changes to successfully compile off the current sbnobj file.

https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=44445

… be happy

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

Adding a comment to each of these to track all 4 related PR.
sbncode: SBNSoftware/sbncode#603
sbndcode: #871
sbnobj: SBNSoftware/sbnobj#155
sbnanaobj: SBNSoftware/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-14T09:15:53Z

Hi @Jjm321814 can you remove the CRT changes from this PR and instead merge in develop which has the required updates needed to be compatible with the sbnobj changes. Thanks!

Jjm321814 · 2025-11-14T16:41:24Z

Merged in develop. Testing Compile now

Jjm321814 · 2025-11-14T16:42:34Z

Compiled fine

Jjm321814 · 2025-11-14T17:18:39Z

Let me make fcl changes to the cafmaker, but a dumped fcl version worked fine

Jjm321814 · 2025-11-14T19:35:25Z

Pushed all the fcl changes needed to run CAF on data and MC.
Have only tested on data. We see a 19% increase in the size of a standard CAF data file (3.7 MB to 4.4 MB) because of blips

henrylay97 · 2025-11-17T14:02:09Z

Hi Jacob, I will review properly after tomorrow's reconstruction meeting. In the meanwhile there are still changes to CRT and LightPropagation files that shouldn't be necessary. Can you remove these? Thanks!

PetrilloAtWork

Left three comments.

PetrilloAtWork · 2025-11-26T22:31:05Z

sbndcode/BlipRecoSBND/Alg/CMakeLists.txt

                           sbnobj::Common_CRT
                           #sbndcode_CosmicIdUtils
                           sbndcode_BlipUtils
+			               sbndobj_BlipDataTypes


I requested the change of this library name.

PetrilloAtWork · 2025-11-26T22:45:07Z

sbndcode/BlipRecoSBND/CMakeLists.txt

                           sbndcode_RecoUtils
                           sbndcode_OpDetSim
                           sbndcode_BlipUtils
+			   sbndobj_BlipDataTypes


Please fix the indentation: copy the line above verbatim, then adjust the library name.

PetrilloAtWork · 2025-11-26T23:00:29Z

sbndcode/BlipRecoSBND/Utils/CMakeLists.txt

                #sbndcode_CosmicIdUtils
 )

 art_dictionary(DICTIONARY_LIBRARIES sbndcode_BlipUtils)


No dictionary is produced any more: just remove this art_dictionary line.

Suggested change

I still want to import BlipUtils.h to the Alg folder though, so don't I still need this dictionary to access functions defined there?

Yes, the dictionary does not help with that.
You will need to add sbndcode_BlipUtils in the LIBRARIES list of CMakeLists.txt in the algorithm folder, but the dictionary won't be involved.

…ixed a mislabel in the fcl

…s. This is useful if we were to rerurn older sbndcode reco1 output over a new reco2 version. Thats a really limited usecase but it could come up

henrylay97

A few small comments

henrylay97 · 2025-12-10T09:31:51Z

sbndcode/BlipRecoSBND/Utils/BlipUtils.h

 //#include "larcorealg/Geometry/GeometryCore.h"
 //#include "larcorealg/Geometry/WireReadoutGeom.h"


In general can we remove commented headers :)

henrylay97 · 2025-12-10T09:33:15Z

sbndcode/BlipRecoSBND/Utils/CMakeLists.txt

                sbndcode_CRTUtils
                sbnobj::Common_CRT
-                #sbndcode_CosmicIdUtils
+		sbnobj::SBND_Blip


indentation

I'm not sure how to fix this -- It looks normal in all my emacs, vim, and vscode. Can you please check out the branch to check if its just a github display issue?

henrylay97 · 2025-12-10T09:34:18Z

sbndcode/BlipRecoSBND/BlipAna_module.cc

 const int kMaxEDeps   =   10000;
-const int kMaxTrkPts  =   2000;  
+const int kMaxTrkPts  =   2000; 
+const int kNplanes = blip::kNplanes;


Can we align equals and values inkeeping with previous lines.

henrylay97 · 2025-12-10T09:34:46Z

sbndcode/JobConfigurations/standard/caf/cafmakerjob_sbnd.fcl

 physics.producers.pandoraShowerRazzle.SimChannelLabel: "simtpc2d:simpleSC"
 physics.producers.pandoraTrackDazzle.SimChannelLabel: "simtpc2d:simpleSC"
-physics.producers.cnnid.WireLabel: "simtpc2d:gauss"
+#physics.producers.cnnid.WireLabel: "simtpc2d:gauss"


Reiterating this comment - I see there is now a line below that points to "simtpc2d:dnnsp" so L129 can just be removed.

henrylay97 · 2025-12-10T09:37:45Z

sbndcode/JobConfigurations/standard/reco/config/workflow_reco1.fcl

+sbnd_reco1_producers.specialblipgaushit.HitFinderToolVec.CandidateHitsPlane0.MinHitHeight: 8
+sbnd_reco1_producers.specialblipgaushit.HitFinderToolVec.CandidateHitsPlane1.MinHitHeight: 15
+sbnd_reco1_producers.specialblipgaushit.HitFinderToolVec.CandidateHitsPlane2.MinHitHeight: 5


There will be conflicts with #887 just be careful during merging that things don't get reverted.

I'll merge that branch in here to make sure they match

PetrilloAtWork

First, thank you for taking the effort of reducing the TVector3 footprint in our environment.

One problem you are trying to solve here is that you were strongly recommended to drop TVector3 for GenVector but then you have to interface with existing code that still uses the former, and you don't want to undertake the effort of refactoring the whole library.
Conversion routines from and to geo::Point_t and geo::Vector_t are available in the geo::vect namespace. Many of them are straightforward, with some exceptions (notably, the "averaging" of points). I will leave comments around to facilitate the adoption of these tools. If you find that the specification of geo::vect is too frequent in a function, you may also consider to add in the function using namespace geo::vect;, but use it sparsely since it has the potential to change the meaning of existing code.

Another remark is that the "sigma" of the blips seems to me to have an odd definition. This is preexisting the pull request.

PetrilloAtWork · 2025-12-10T17:15:51Z

sbndcode/BlipRecoSBND/Utils/BlipUtils.cc

+    pinfo.position.SetXYZ(0.5*(pinfo.startPoint.X()+pinfo.endPoint.X()),
+                         0.5*(pinfo.startPoint.Y()+pinfo.endPoint.Y()),
+                         0.5*(pinfo.startPoint.Z()+pinfo.endPoint.Z()) );



This is an unweighted middle point, so:

Suggested change

pinfo.position.SetXYZ(0.5*(pinfo.startPoint.X()+pinfo.endPoint.X()),

0.5*(pinfo.startPoint.Y()+pinfo.endPoint.Y()),

0.5*(pinfo.startPoint.Z()+pinfo.endPoint.Z()) );

pinfo.position = geo::vect::middlePoint({ pinfo.startPoint, pinfo.endPoint });

(#include "larcorealg/Geometry/geo_vector_utils.h")
Note the braces around the list of points (you can have any number of them).
This function is a simpler interface to MiddlePointAccumulator, which I'll mention below.

PetrilloAtWork · 2025-12-10T17:17:16Z

sbndcode/BlipRecoSBND/Utils/BlipUtils.cc

+      tblip.Position.SetXYZ( w1*tblip.Position.X() + w2*pinfo.position.X(), 
+                            w1*tblip.Position.Y() + w2*pinfo.position.Y(),
+                            w1*tblip.Position.Z() + w2*pinfo.position.Z());


This is the hardest one, being weighted. I would use the accumulator directly:

Suggested change

tblip.Position.SetXYZ( w1*tblip.Position.X() + w2*pinfo.position.X(),

w1*tblip.Position.Y() + w2*pinfo.position.Y(),

w1*tblip.Position.Z() + w2*pinfo.position.Z());

geo::vect::MiddlePointAccumulator mpalg;

mpalg.add(tblip.Position, w1);

mpalg.add(pinfo.Position, w2);

tblip.Position = mpalg.middlePoint();

(#include "larcorealg/Geometry/geo_vector_utils.h")

PetrilloAtWork · 2025-12-10T17:22:11Z

sbndcode/BlipRecoSBND/Utils/BlipUtils.cc

        // together a blip that happened much later but in the same spot)
        if( fabs(blip_i.Time - blip_j.Time) > 5 ) continue;
-        float d = (blip_i.Position-blip_j.Position).Mag();
+        float d = TMath::Sqrt((blip_i.Position-blip_j.Position).Mag2());


You can directly use .R():

Suggested change

float d = TMath::Sqrt((blip_i.Position-blip_j.Position).Mag2());

float d = (blip_i.Position-blip_j.Position).R();

Also note that for determining the closest point, you don't really need to take the square root of the square distance, because the minimum distance is also the minimum distance square, so you can look for the latter and save a tiny bit of computing time.
An awkward feature of the new vectors is that they have Mag2() and R(), but not Mag() and R2(). 🤷

PetrilloAtWork · 2025-12-10T17:23:03Z

sbndcode/BlipRecoSBND/Utils/BlipUtils.cc

+          blip_i.Position.SetXYZ( w1*blip_i.Position.X() + w2*blip_j.Position.X(), 
+                                  w1*blip_i.Position.Y() + w2*blip_j.Position.Y(),
+                                  w1*blip_i.Position.Z() + w2*blip_j.Position.Z());


As above:

Suggested change

blip_i.Position.SetXYZ( w1*blip_i.Position.X() + w2*blip_j.Position.X(),

w1*blip_i.Position.Y() + w2*blip_j.Position.Y(),

w1*blip_i.Position.Z() + w2*blip_j.Position.Z());

geo::vect::MiddlePointAccumulator mpalg;

mpalg.add(blip_i.Position, w1);

mpalg.add(blip_j.Position, w2);

blip_i.Position = mpalg.middlePoint();

However, here we are in a loop context, so I recommend that the accumulation is done first, and then the results saved. You can declare your accumulators (MiddlePointAccumulator, StatCollector) in the outer loop, dealing with blip_i (and feeding in there the blip i), and then fill them in the inner loop:

initialization in outer loop
for(size_t i=0; i<vtb.size(); i++){ if( isGrouped.at(i) ) continue; else isGrouped.at(i) = true; auto& blip_i = vtb.at(i); geo::vect::MiddlePointAccumulator position; lar::util::StatCollector<double> time; float const weight_i = blip_i.DepElectrons; position.add(blip_i.Position, weight_i); time.add(blip_i.time, weight_i); // ... for(size_t j=i+1; j<vtb.size(); j++){

filling in here:

Suggested change

blip_i.Position.SetXYZ( w1*blip_i.Position.X() + w2*blip_j.Position.X(),

w1*blip_i.Position.Y() + w2*blip_j.Position.Y(),

w1*blip_i.Position.Z() + w2*blip_j.Position.Z());

float const weight_j = blip_j.DepElectrons;

position.add(blip_j.Position.X(), weight_j);

time.add(blip_j.time, weight_j);

// ...

putting back into blip_i at the end of the loop:
}//loop over blip_j blip_i.Position = position.middlePoint(); blip_i.time = time.Average(); // ... blip_i.ID = vtb_merged.size();

PetrilloAtWork · 2025-12-10T19:20:22Z

sbndcode/BlipRecoSBND/Alg/BlipRecoAlg.cc

              // TO-DO: if this track starts or ends at a TPC boundary, 
              // we should extend p1 or p2 to outside the AV to avoid blind spots
-              TVector3 bp = newBlip.Position;
+              TVector3 bp(newBlip.Position.X(), newBlip.Position.Y(), newBlip.Position.Z());


Conversion geo::Point_t → TVector3:

Suggested change

TVector3 bp(newBlip.Position.X(), newBlip.Position.Y(), newBlip.Position.Z());

TVector3 bp = geo::vect::toTVector3(newBlip.Position);

(#include "larcorealg/Geometry/geo_vectors_utils_TVector.h"); or using the more generic convertTo():

Suggested change

TVector3 bp(newBlip.Position.X(), newBlip.Position.Y(), newBlip.Position.Z());

TVector3 bp = geo::vect::convertTo<TVector3>(newBlip.Position);

(for this one: #include "larcorealg/Geometry/geo_vectors_utils.h").

PetrilloAtWork · 2025-12-10T20:33:16Z

sbndcode/BlipRecoSBND/Utils/BlipUtils.cc

+      for(auto& v : wirex ) newblip.Position.SetXYZ( newblip.Position.X()  + v.X() * fact, 
+                                                     newblip.Position.Y()  + v.Y() * fact, 
+                                                     newblip.Position.Z()  + v.Z() * fact);


This should work:

Suggested change

for(auto& v : wirex ) newblip.Position.SetXYZ( newblip.Position.X() + v.X() * fact,

newblip.Position.Y() + v.Y() * fact,

newblip.Position.Z() + v.Z() * fact);

geo::vect::MiddlePointAccumulator position;

position.add(begin(wirex), end(wirex));

newblip.Position = position.middlePoint();

I think this will work, still, given the following, it would be better having wirex redefined as std::vector<geo::Point_t>, especially considering that it is used only for this computation, and much better to have two accumulators initialised before the loop (a StatCollector2D sc for y and z — x is 0 —, and optionally a MiddlePointAccumulator for the position — but that one can be extracted by the other two), collect the statistics adding the coordinates in the loop, and finally computing the σ (sqrt(sc.VarianceX() + sc.VarianceY())) and average position ({ 0, sc.AverageX(), sc.AverageY() }). Also, the definition of σ feels off to me: it's the average of distances, while the usual RMS is the square root of the average of the squared distances (as in my example with VarianceX()/Y()).

PetrilloAtWork · 2025-12-10T20:38:54Z

sbndcode/BlipRecoSBND/Utils/CMakeLists.txt

                #sbndcode_CosmicIdUtils
 )

 art_dictionary(DICTIONARY_LIBRARIES sbndcode_BlipUtils)


Yes, the dictionary does not help with that.
You will need to add sbndcode_BlipUtils in the LIBRARIES list of CMakeLists.txt in the algorithm folder, but the dictionary won't be involved.

Jacob McLaughlin and others added 4 commits November 6, 2025 11:02

adjustments to new paths

3abd95c

Trying to get compile to solve

4d2df24

Assorted changes to things I don't actually use so the compiler would…

619cc9d

… be happy

updated sbnobj

8c71d0b

Jjm321814 requested review from henrylay97 and sungbinoh November 13, 2025 15:13

Jjm321814 self-assigned this Nov 13, 2025

This was referenced Nov 13, 2025

Feature/adding blip to caf SBNSoftware/sbnobj#155

Open

Feature/adding blip to caf SBNSoftware/sbncode#603

Open

Feature/adding blip to caf SBNSoftware/sbnanaobj#173

Open

Jacob McLaughlin and others added 5 commits November 13, 2025 16:28

Forgot to take out cmake lists

bb7fca3

CMAKEList updates

04973a2

Updating includes

a8816a2

Tried to separate the removal of classes.h but it doesn't actually work

7a1e130

compiler is so fussy

70ee378

Jacob McLaughlin added 2 commits November 14, 2025 10:30

compile changes

9a41019

merge issues

8ab2400

Jacob McLaughlin added 3 commits November 14, 2025 11:55

updated caf maker fcl

b2c99ca

fcl changes to include blips

2736872

MC caf updates

dbcec4b

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

linyan-w added this to SBND 2025 Fall Production Nov 17, 2025

Merge branch 'feature/BlipBugFix' into feature/AddingBlipToCAF

90533bc

PetrilloAtWork reviewed Nov 26, 2025

View reviewed changes

Jjm321814 added 17 commits November 29, 2025 11:36

fixed library name

382de1d

TVector3 Replacement updates

1808514

more TVector 3 conversion

d137623

Update Power calls

184ba70

Not sure this should function

ae53aa5

One more cmake update

785cf59

How did I miss so many

a23b8fe

How did I miss so many

311ee02

Weird these cmake issues appeared now and not earlier

a36a2b5

Adding includes that are probably unneeded

e7951b5

Oh I just updated one spot

1db5fd5

Oh I just updated one spot

813ff32

I hope I don't just replace all these tvector 3

d075348

updated middle induction plane hit amplitude to match Maria's work. F…

cbcb056

…ixed a mislabel in the fcl

Added two reco2 fcl that let us move back to single hitfinding stream…

d7ca349

…s. This is useful if we were to rerurn older sbndcode reco1 output over a new reco2 version. Thats a really limited usecase but it could come up

Being extra careful

794d240

fixing relabel in the new fcl

f886d99

linyan-w requested review from PetrilloAtWork, henrylay97 and wjdanswjddl December 9, 2025 15:49

henrylay97 reviewed Dec 10, 2025

View reviewed changes

Jjm321814 added 4 commits December 10, 2025 08:48

addressing Henry comments

3a30ea5

Merge branch 'feature/BlipBugFix' into feature/AddingBlipToCAF

4b0b4c5

Tab align issue

298f171

Tab align issue

9c94f32

linyan-w moved this from Waiting on Reviewer to Draft in SBND 2025 Fall Production Dec 10, 2025

PetrilloAtWork reviewed Dec 10, 2025

View reviewed changes

linyan-w moved this from Draft to Expected for Later in SBND 2025 Fall Production Dec 15, 2025

		//#include "larcorealg/Geometry/GeometryCore.h"
		//#include "larcorealg/Geometry/WireReadoutGeom.h"

-      tblip.Position.SetXYZ( w1*tblip.Position.X() + w2*pinfo.position.X(),
-                            w1*tblip.Position.Y() + w2*pinfo.position.Y(),
-                            w1*tblip.Position.Z() + w2*pinfo.position.Z());
+      geo::vect::MiddlePointAccumulator mpalg;
+      mpalg.add(tblip.Position, w1);
+      mpalg.add(pinfo.Position, w2);
+      tblip.Position = mpalg.middlePoint();

	float d = TMath::Sqrt((blip_i.Position-blip_j.Position).Mag2());
	float d = (blip_i.Position-blip_j.Position).R();

	TVector3 bp(newBlip.Position.X(), newBlip.Position.Y(), newBlip.Position.Z());
	TVector3 bp = geo::vect::toTVector3(newBlip.Position);

	TVector3 bp(newBlip.Position.X(), newBlip.Position.Y(), newBlip.Position.Z());
	TVector3 bp = geo::vect::convertTo<TVector3>(newBlip.Position);

Feature/adding blip to caf #871

Are you sure you want to change the base?

Feature/adding blip to caf #871

Uh oh!

Conversation

Jjm321814 commented Nov 13, 2025 • edited by henrylay97 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jjm321814 commented Nov 13, 2025

Uh oh!

henrylay97 commented Nov 14, 2025

Uh oh!

Jjm321814 commented Nov 14, 2025

Uh oh!

Jjm321814 commented Nov 14, 2025

Uh oh!

Jjm321814 commented Nov 14, 2025

Uh oh!

Jjm321814 commented Nov 14, 2025

Uh oh!

henrylay97 commented Nov 17, 2025

Uh oh!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

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

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!

henrylay97 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Jjm321814 commented Nov 13, 2025 •

edited by henrylay97

Loading

PetrilloAtWork Nov 26, 2025 •

edited

Loading