Feature/adding blip to caf #603

Jjm321814 · 2025-11-13T15:19:50Z

Added CAFMaker interface to add blips to CAF files.
This PR must not be approved until SBNSoftware/sbnobj#155 and SBNSoftware/sbnanaobj#173 are approved/released.
Working on estimating the file size change now.

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

Adding a comment to each of these to track all 4 related PR.
sbncode: #603
sbndcode: SBNSoftware/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.

Jjm321814 · 2025-11-14T19:38:05Z

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

JosiePaton

Assuming reasonable file size change result!

henrylay97 · 2025-11-19T18:33:39Z

sbncode/CAFMaker/CAFMaker_module.cc

  int          fGenieEvtRec_brStdHepLd    [250] = {0}; ////< Last  daughter
  int          fGenieEvtRec_brStdHepFm    [250] = {0}; ////< First mother
  int          fGenieEvtRec_brStdHepLm    [250] = {0}; ////< Last  mother
+  //std::string fBlipTag;


Can you remove old commented code?

henrylay97 · 2025-11-19T18:36:52Z

sbncode/CAFMaker/CAFMakerParams.h

      "cvn" 
    };
+    Atom<std::string> fBlipTag { Name("BlipTag"),
+	Comment("Provide a string to label the blip input"), "reco2:Blip:BlipReco"


This should be the producer that made the blips.

Is that not defined as blipreco here?

Yes that is what it should be. I thought it would be the whole LArSoft label, so guessed this version. Its wrong and I am happy to update it to blipreco

henrylay97 · 2025-11-19T18:38:05Z

sbncode/CAFMaker/FillBlip.cxx

+	  CAF_Blip.clusters[iPlane].ID = LarBlip.clusters[iPlane].ID;
+	  CAF_Blip.clusters[iPlane].isValid = LarBlip.clusters[iPlane].isValid;
+	  CAF_Blip.clusters[iPlane].CenterChan = LarBlip.clusters[iPlane].CenterChan;
+	  CAF_Blip.clusters[iPlane].CenterWire = LarBlip.clusters[iPlane].CenterWire;
+	  CAF_Blip.clusters[iPlane].isTruthMatched = LarBlip.clusters[iPlane].isTruthMatched;
+	  CAF_Blip.clusters[iPlane].isMerged = LarBlip.clusters[iPlane].isMerged;
+          CAF_Blip.clusters[iPlane].isMatched = LarBlip.clusters[iPlane].isMatched;


In general there are quite a few indentation inconsistencies across these PRs. It's often different editors and how they treat tabs, there are normally ways to untabify though.

henrylay97 · 2025-11-19T18:38:42Z

sbncode/CAFMaker/FillBlip.cxx

@@ -0,0 +1,104 @@
+#include "sbncode/CAFMaker/FillBlip.h"


Thanks for putting in a separate file, FillReco is getting unwieldy.

Jjm321814 · 2025-11-21T16:34:10Z

Just noting on this PR as well that the above Henry comments should all be addressed.

PetrilloAtWork

A few things need to be changed (redundant headers, missing const parameters), others are recommended to.
Details in the comments.

PetrilloAtWork · 2025-11-24T17:41:24Z

sbncode/CAFMaker/CAFMaker_module.cc

+    //for(int i=0; i<int(*(blipHandle)->size()); i++)
+    //  {
+    //   auto LarBlips = (*(blipHandle))[i];
+    //	FillBlip(   (LarBlips) , srblips);
+    //  }


Please remove obsolete, commented out code:

Suggested change

//for(int i=0; i<int(*(blipHandle)->size()); i++)

// {

// auto LarBlips = (*(blipHandle))[i];

// FillBlip( (LarBlips) , srblips);

// }

PetrilloAtWork · 2025-11-24T17:55:05Z

sbncode/CAFMaker/CAFMaker_module.cc

+  std::vector<caf::SRBlip> srblips;
+  if(evt.getByLabel( fParams.fBlipTag(), blipHandle)) //fill SR blips
+  {
+    FillBlip( (*blipHandle), srblips);


Parentheses are redundant here. Just saying.

Suggested change

FillBlip( (*blipHandle), srblips);

FillBlip( *blipHandle, srblips);

(this is a suggestion)

PetrilloAtWork · 2025-11-24T17:58:52Z

sbncode/CAFMaker/CMakeLists.txt

                  sbnobj::Common_Trigger
                  sbnobj::SBND_CRT
                  sbnobj::SBND_Timing
+		  sbndobj_BlipDataTypes


Please fix the alignment. My suggestion is to copy the line above, to make sure the same indentation characters are use.

PetrilloAtWork · 2025-11-24T18:01:35Z

sbncode/CAFMaker/CAFMakerParams.h

+    Atom<std::string> fBlipTag { Name("BlipTag"),
+	Comment("Provide a string to label the blip input"), "blipreco"
+	};


Conform to the existing alignment ([CO-02]).

PetrilloAtWork · 2025-11-24T18:04:46Z

sbncode/CAFMaker/FillBlip.cxx

+        int NBlips = LarBlips.size();
+        for(int iBlip=0; iBlip<NBlips; iBlip++)
+        {
+            blip::Blip CurrentBlip = LarBlips[iBlip];


I think this is equivalent to using a range-for loop:

Suggested change

int NBlips = LarBlips.size();

for(int iBlip=0; iBlip<NBlips; iBlip++)

{

blip::Blip CurrentBlip = LarBlips[iBlip];

for(blip::Blip const& CurrentBlip: LarBlips)

{

This avoids copies (blip::Blip CurrentBlip = LarBlips[iBlip] copies) and the definition of two variables that are not used anywhere else.
I recommend to adopt the suggested syntax.

PetrilloAtWork · 2025-11-24T18:23:05Z

sbncode/CAFMaker/FillBlip.cxx

+
+    void FillBlipRealtedHitCluster(blip::Blip& LarBlip, caf::SRBlip &CAF_Blip)
+    {
+        int NumPlanes = sizeof(LarBlip.clusters)/sizeof(LarBlip.clusters[0]);


Since you defined a kNplanes constant, you can use that one directly.
Or, if you have adopted the suggestion of replacing the C array with a std::array, you do have LarBlip.clusters.size(). Both are more readable than the (functionally correct) code in this line.

PetrilloAtWork · 2025-11-25T00:23:15Z

sbncode/CAFMaker/FillBlip.cxx

+            //These are sets that need to become vectors so we need to do some loops
+            for(auto HitID : LarBlip.clusters[iPlane].HitIDs) CAF_Blip.clusters[iPlane].HitIDs.push_back(HitID);


I recommend to use the appropriate std::vector member function, assign():

Suggested change

//These are sets that need to become vectors so we need to do some loops

for(auto HitID : LarBlip.clusters[iPlane].HitIDs) CAF_Blip.clusters[iPlane].HitIDs.push_back(HitID);

// These are sets that need to become vectors

CAF_Blip.clusters[iPlane].HitIDs.assign(begin(LarBlip.clusters[iPlane].HitIDs), end(LarBlip.clusters[iPlane].HitIDs));

If this function is changed as recommended above to work on a single cluster, this becomes less wordy and more readable:

Suggested change

//These are sets that need to become vectors so we need to do some loops

for(auto HitID : LarBlip.clusters[iPlane].HitIDs) CAF_Blip.clusters[iPlane].HitIDs.push_back(HitID);

// These are sets that need to become vectors

CAFcluster.HitIDs.assign(begin(LArCluster.HitIDs), end(LArCluster.HitIDs));

same for the next three lines; in fact, you could even write a specific function and use it repeatedly:

Suggested change

//These are sets that need to become vectors so we need to do some loops

for(auto HitID : LarBlip.clusters[iPlane].HitIDs) CAF_Blip.clusters[iPlane].HitIDs.push_back(HitID);

// These are sets that need to become vectors

auto copyToVector = [](auto& dest, auto const& src)

{ dest.assign(begin(src), end(src)); };

copyToVector(CAF_Blip.clusters[iPlane].HitIDs, LarBlip.clusters[iPlane].HitIDs);

etc.

PetrilloAtWork · 2025-11-25T00:27:34Z

sbncode/CAFMaker/FillBlip.h

@@ -0,0 +1,17 @@
+#ifndef CAF_FILLBLIP_H
+#define CAF_FILLBLIP_H
+#include<iostream>


Unused header:

Suggested change

#include<iostream>

PetrilloAtWork · 2025-11-25T00:28:41Z

sbncode/CAFMaker/FillBlip.h

+  void FillMCTruthBlip(blip::Blip& LarBlip,  caf::SRBlip& CAF_Blip );
+  void FillBlipRealtedHitCluster(blip::Blip& LarBlip,  caf::SRBlip& CAF_Blip);


In both these functions one of the operands (the first one, I'd wage) must be declared const.
Also, it seems that the only function needed externally (by CAFMaker module) is the first one; the other two do not need to be publicly visible in the header, and their declaration can be either omitted or moved out of the header into the implementation (.cxx) file.

PetrilloAtWork · 2025-11-25T01:01:52Z

sbncode/CAFMaker/FillBlip.h

@@ -0,0 +1,17 @@
+#ifndef CAF_FILLBLIP_H


It would be good to add a standard header with file name, brief description and author identification, like in:

Suggested change

#ifndef CAF_FILLBLIP_H

/**

* @file sbncode/CAFMaker/FillBlip.h

* @brief CAFMaker utilities for filling "blip" objects into CAF.

* @author Jacob McLaughlin (jmclaughlin2@illinoistech.edu)

/

#ifndef CAF_FILLBLIP_H

The same holds for the .cxx file and, in fact, all other headers and source files. This is not often done in SBN repositories, and it would be better if it were.

Jjm321814 and others added 7 commits October 29, 2025 09:07

first draft of adding blips

18b28ec

Added blip library

daa5c8e

forgot to add my blip to the vector

8dfaab0

Adding blips to cafmaker

23c5909

Merge branch 'develop' into feature/AddingBlipToCAF

6ec65f7

added blips to cafmaker

3812b04

Merge branch 'develop' into feature/AddingBlipToCAF

f830082

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

Jjm321814 self-assigned this Nov 13, 2025

This was referenced Nov 13, 2025

Feature/adding blip to caf SBNSoftware/sbnanaobj#173

Open

Feature/adding blip to caf SBNSoftware/sbndcode#871

Open

Feature/adding blip to caf SBNSoftware/sbnobj#155

Open

Jacob McLaughlin added 2 commits November 13, 2025 16:47

mistake in the cmakelist

e4cc10e

Updating include

0c62419

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

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

Jacob McLaughlin and others added 2 commits November 20, 2025 08:57

Fixing comments that aren't tabs

97b9db8

Fixing indentation

44e03a4

PetrilloAtWork requested changes Nov 25, 2025

View reviewed changes

Jjm321814 added 3 commits November 29, 2025 12:54

part of general tvector3 replacement

a44f4c6

Missed one cmake list update

b6b0f4f

Adjusting fillBlip to account for removed variables

ddda71a

Jjm321814 and others added 2 commits November 30, 2025 17:20

change to camelcase

05878a3

Merge branch 'develop' into feature/AddingBlipToCAF

3b3e41b

	FillBlip( (*blipHandle), srblips);
	FillBlip( *blipHandle, srblips);

		//These are sets that need to become vectors so we need to do some loops
		for(auto HitID : LarBlip.clusters[iPlane].HitIDs) CAF_Blip.clusters[iPlane].HitIDs.push_back(HitID);

-            //These are sets that need to become vectors so we need to do some loops
-            for(auto HitID : LarBlip.clusters[iPlane].HitIDs) CAF_Blip.clusters[iPlane].HitIDs.push_back(HitID);
+            // These are sets that need to become vectors
+            auto copyToVector = [](auto& dest, auto const& src)
+              { dest.assign(begin(src), end(src)); };
+            copyToVector(CAF_Blip.clusters[iPlane].HitIDs, LarBlip.clusters[iPlane].HitIDs);

		void FillMCTruthBlip(blip::Blip& LarBlip, caf::SRBlip& CAF_Blip );
		void FillBlipRealtedHitCluster(blip::Blip& LarBlip, caf::SRBlip& CAF_Blip);

-#ifndef CAF_FILLBLIP_H
+/**
+ * @file   sbncode/CAFMaker/FillBlip.h
+ * @brief  CAFMaker utilities for filling "blip" objects into CAF.
+ * @author Jacob McLaughlin (jmclaughlin2@illinoistech.edu)
+ /
+#ifndef CAF_FILLBLIP_H

Feature/adding blip to caf #603

Are you sure you want to change the base?

Feature/adding blip to caf #603

Uh oh!

Conversation

Jjm321814 commented Nov 13, 2025

Uh oh!

Jjm321814 commented Nov 13, 2025

Uh oh!

Jjm321814 commented Nov 14, 2025

Uh oh!

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

Jjm321814 commented Nov 21, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants