Feature/adding blip to caf #155

Jjm321814 · 2025-11-10T15:08:52Z

This is part of a large array of PR to add blip objects to CAF files.
Previously the key blip structs were defined in sbndcode and this PR simply moves them to sbnobj.

Jjm321814 · 2025-11-10T15:11:02Z

An associated branch in sbndcode (also feature/AddingBlipToCAF) has the updated libraries linking necessary to point here. Once this one is in a release we can delete the file copies in sbndcode

kjplows · 2025-11-12T19:08:27Z

Hi @Jjm321814 , is there a required PR in sbndcode to accompany this? If yes, please point me to that PR. Thanks!

kjplows · 2025-11-12T19:10:46Z

Also summoning @PetrilloAtWork to take a look if there's time, not sure if ICARUS has something similar to this?

Jjm321814 · 2025-11-13T15:14:41Z

The related sbndcode changes are in SBNSoftware/sbndcode#871 but they depend on this PR, not the other way around.
This one just moves some struct definitions out of sbndcode and into sbnobj so that it can be imported into sbncode for CAFMaker.

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

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

Accidentally ran with the wrong blip tag and still produced a CAF file in sbndcode area, so I expect it will work with ICARUS but I can not test it.

Jjm321814 · 2025-11-24T17:03:08Z

Nope nothing is broken, just had a sbndcode area setup with an earlier LArSoft release that was causing me issues. I'll get back to the comments here

PetrilloAtWork · 2025-11-24T18:20:42Z

sbnobj/SBND/Blip/BlipDataTypes.h

+    float     dYZ             = -9;         // Approximate length scale in YZ space [cm]
+
+    // Plane/cluster-specific information
+    blip::HitClust clusters[kNplanes];


Consider:

Suggested change

blip::HitClust clusters[kNplanes];

std::array<blip::HitClust, kNplanes> clusters;

That object provides a size() member that is useful in user code.

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

sbnobj/SBND/Blip/CMakeLists.txt

@@ -0,0 +1,11 @@
+cet_make_library( LIBRARY_NAME sbndobj_BlipDataTypes


Suggested change

cet_make_library( LIBRARY_NAME sbndobj_BlipDataTypes

cet_make_library(

Library names are very hard to guess. A verbose but effective strategy is to leave it the default, which makes it easily predictable. And the most used convention guarantees at least the first element to be the repository the library belongs to ("sbndobj"?). I request it does not start with sbndobj, which is misleading, and strongly recommend to leave this library name the default:

in CMakeLists.txt link lists, people will have to write the annoyingly long sbnobj::SBND_Blip (could actually be worse);

when you #include "sbnobj/SBND/Blip/BlipDataTypes.h", you know that the library you need to add follows the path of the header (other example:
#include "lardataobj/RecoBase/Hit.h" #include "nusimdata/SimulationBase/MCParticle.h"
links to lardataobj::RecoBase and nusimdata::SimulationBase. Presto.)

Updated the library name here and in relevant include locations.

PetrilloAtWork · 2025-11-26T22:40:49Z

sbnobj/SBND/Blip/BlipDataTypes.h

+#ifndef BLIPDATATYPE
+#define BLIPDATATYPE
+#include "lardataobj/RecoBase/Hit.h"
+#include "nusimdata/SimulationBase/MCParticle.h"


If TVector3 is going to stay, #include "TVector3.h".
Much better if a much better data type is chosen though.

TVector3 was removed from here. It only remains as a temporary variable in some steps of sbndcode processing

PetrilloAtWork · 2025-11-26T22:41:11Z

sbnobj/SBND/Blip/classes.h

+template class art::Assns<blip::Blip,recob::Hit,void>;
+template class art::Wrapper<art::Assns<blip::Blip,recob::Hit,void> >;
+template class art::Assns<blip::Blip,recob::SpacePoint,void>;
+template class art::Wrapper<art::Assns<blip::Blip,recob::SpacePoint,void> >;


You don't need to bother specifying the void association metadata explicitly — it's a bit distracting, so I suggest the removal. Also, when you instantiate an art::Wrapper<T>, you also implicitly trigger the instantiation of T, so the others are redundant. This works because this is C++; classes_def.xml still needs all of them, unfortunately. Finally, the "partner association" instantiation is also not needed.

Suggested change

template class art::Wrapper<art::Assns<blip::Blip,recob::SpacePoint,void> >;

template class art::Wrapper<std::vector<blip::Blip> >;

template class art::Wrapper<art::Assns<blip::Blip,recob::Hit> >;

template class art::Wrapper<art::Assns<blip::Blip,recob::SpacePoint> >;

I think the map instantiation is also not needed — I am not sure where it comes from, so I might be wrong.

Removed voids and the map. Seems to build still

PetrilloAtWork

In addition to the previous comments, I have added some on files I had missed.

PetrilloAtWork · 2025-11-26T22:49:22Z

sbnobj/SBND/Blip/classes_def.xml

+<class name="art::Wrapper<std::vector<blip::Blip> >"/>
+<class name="blip::Blip"/>
+<class name="std::vector<blip::Blip>"/>
+<class name="blip::HitClust"/>
+<class name="vector<blip::HitClust>"/>
+<class name="art::Wrapper<std::vector<blip::HitClust> >"/>
+<class name="blip::TrueBlip"/>
+<class name="std::map<int,TVector3>"/>
+<class name="art::Assns<blip::Blip,recob::Hit,void>"/>
+<class name="art::Assns<recob::Hit, blip::Blip,void>"/>
+<class name="art::Assns<blip::Blip,recob::SpacePoint,void>"/>
+<class name="art::Wrapper<art::Assns<blip::Blip,recob::Hit,void> >"/>
+<class name="art::Wrapper<art::Assns<recob::Hit,blip::Blip,void> >"/>
+<class name="art::Wrapper<art::Assns<blip::Blip,recob::SpacePoint,void> >"/>


Some of these need to have ROOT I/O schema evolution enabled, that means that ROOT creates enough stuff so that old data can be read with newer code.
To do that, a class version needs to be added to the simple object (not std::vector, art::Assns etc.) and then the build needs to be run again. In that way, ROOT will extract and save the checksum of the current version of the class, and will do that again in the future each time the class changes.
This change is required.

Suggested change

<class name="art::Wrapper<std::vector<blip::Blip> >"/>

<class name="blip::Blip"/>

<class name="std::vector<blip::Blip>"/>

<class name="blip::HitClust"/>

<class name="vector<blip::HitClust>"/>

<class name="art::Wrapper<std::vector<blip::HitClust> >"/>

<class name="blip::TrueBlip"/>

<class name="std::map<int,TVector3>"/>

<class name="art::Assns<blip::Blip,recob::Hit,void>"/>

<class name="art::Assns<recob::Hit, blip::Blip,void>"/>

<class name="art::Assns<blip::Blip,recob::SpacePoint,void>"/>

<class name="art::Wrapper<art::Assns<blip::Blip,recob::Hit,void> >"/>

<class name="art::Wrapper<art::Assns<recob::Hit,blip::Blip,void> >"/>

<class name="art::Wrapper<art::Assns<blip::Blip,recob::SpacePoint,void> >"/>

<class name="art::Wrapper<std::vector<blip::Blip> >"/>

<class name="blip::Blip" ClassVersion="10" />

<class name="std::vector<blip::Blip>"/>

<class name="blip::HitClust" ClassVersion="10" />

<class name="vector<blip::HitClust>"/>

<class name="art::Wrapper<std::vector<blip::HitClust> >"/>

<class name="blip::TrueBlip" ClassVersion="10" />

<class name="std::map<int,TVector3>"/>

<class name="art::Assns<blip::Blip,recob::Hit,void>"/>

<class name="art::Assns<recob::Hit, blip::Blip,void>"/>

<class name="art::Assns<blip::Blip,recob::SpacePoint,void>"/>

<class name="art::Wrapper<art::Assns<blip::Blip,recob::Hit,void> >"/>

<class name="art::Wrapper<art::Assns<recob::Hit,blip::Blip,void> >"/>

<class name="art::Wrapper<art::Assns<blip::Blip,recob::SpacePoint,void> >"/>

Again, where is std::map<int,TVector3> needed?
Also, I don't know if the order matters here. In case, put the simplest objects first.

The map is used to record intersection points of induction/collection plane wires for interplane matching. IntersectLocations in the HitClust struct. I am updating it to geo::point_t in accordance with the other suggestions

Updated with class version labels, and removed std::map. Its not saved to the event so its not needed here.

PetrilloAtWork · 2025-11-26T22:50:16Z

sbnobj/SBND/Blip/CMakeLists.txt

+    lardataobj::RecoBase
+    nusimdata::SimulationBase
+)
+art_dictionary(DICTIONARY_LIBRARIES sbndobj_BlipDataTypes) 


(update this with the fixed library name)

Updated here and in other repo

PetrilloAtWork · 2025-11-26T22:51:03Z

sbnobj/SBND/Blip/classes.h

+
+// data-products
+// lardataobj
+//#include "lardata/Utilities/AssociationUtil.h"


This one is spurious here:

Suggested change

//#include "lardata/Utilities/AssociationUtil.h"

Removed this

PetrilloAtWork · 2025-11-26T22:51:26Z

sbnobj/SBND/Blip/classes.h

+// lardataobj
+//#include "lardata/Utilities/AssociationUtil.h"
+#include "canvas/Persistency/Common/Assns.h"
+#include "lardataobj/RecoBase/PFParticle.h"


Unused:

Suggested change

#include "lardataobj/RecoBase/PFParticle.h"

PetrilloAtWork · 2025-11-26T22:52:21Z

sbnobj/SBND/Blip/classes.h

+#include "canvas/Persistency/Common/Assns.h"
+#include "lardataobj/RecoBase/PFParticle.h"
+#include "lardataobj/RecoBase/Hit.h"
+#include "nusimdata/SimulationBase/MCTruth.h"


Also not needed:

Suggested change

#include "nusimdata/SimulationBase/MCTruth.h"

Jjm321814 · 2025-11-29T20:24:43Z

All the suggestions here have been committed. Every struct has doxygen comments, TVector3 has been generally replaced, and the few housekeeping comments have been accepted.
A later fix can remove TVector3 from blip processing steps too. May I ask what is the issue with TVector3?

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

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

Jjm321814 and others added 10 commits November 6, 2025 09:44

Adding blip data types to sbnobj to help with CAF addition

7f7a897

Moving blip utils over

18d8269

Moving blip utils

9751aa3

trying to remove depends

1387b2a

trying to remove depends

f5c315a

Im not sure how to link the dictionaries here anymore

2ce06f9

Simplifying includes

2d37c3f

trying to make a library

55d9f70

Still trying to make a library

343d755

sorting compiles

99a3c01

Jjm321814 self-assigned this Nov 10, 2025

Jjm321814 requested a review from sungbinoh November 10, 2025 15:09

compiled version

bcbb2aa

kjplows requested a review from PetrilloAtWork November 12, 2025 19:09

Jjm321814 mentioned this pull request Nov 13, 2025

Feature/adding blip to caf SBNSoftware/sbndcode#871

Open

This was referenced Nov 13, 2025

Feature/adding blip to caf SBNSoftware/sbncode#603

Open

Feature/adding blip to caf SBNSoftware/sbnanaobj#173

Open

Jacob McLaughlin and others added 2 commits November 13, 2025 18:47

small fix for sbndcode

5bfaf2c

Merge branch 'develop' into feature/AddingBlipToCAF

90a5dc2

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

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

linyan-w moved this to Waiting on Reviewer in SBND 2025 Fall Production Nov 17, 2025

nathanielerowe removed this from SBND 2025 Fall Production Nov 17, 2025

kjplows added this to SBN software development Nov 18, 2025

PetrilloAtWork reviewed Nov 24, 2025

View reviewed changes

PetrilloAtWork reviewed Nov 26, 2025

View reviewed changes

PetrilloAtWork requested changes Nov 26, 2025

View reviewed changes

Jjm321814 and others added 19 commits November 29, 2025 11:24

Added lots of doxygen comments. Need to test geo::Point_t still

bbd96f7

changed to default cmake library

8b3cb45

Updating order in classes.h and adding class version index

47f8630

These have to be in a specific order

d4991e5

Getting rid of ;

91eba8c

don't need this one

83efc10

Adding blank lines to be filled by compiler

3d4c322

Adding blank lines to be filled by compiler

4e06944

I don't understand this version tracker

5ae8d14

I was putting things in the wrong place

1fd419a

fixed stray typedef

913d94b

Added checksum

ec53ab8

Missed a few comments

1595e6c

Changed array initilization

6194c03

Removing some classes.h

41e66f4

Updated Checksum

3736ddd

Changing class version

cc24452

remove unsaved item

dfd4d1d

removed unused headers

4c9288e

Jjm321814 and others added 2 commits November 30, 2025 16:32

Fixed a comment

d460853

Merge branch 'develop' into feature/AddingBlipToCAF

bdd98cb

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

	blip::HitClust clusters[kNplanes];
	std::array<blip::HitClust, kNplanes> clusters;

		@@ -0,0 +1,11 @@
		cet_make_library( LIBRARY_NAME sbndobj_BlipDataTypes

	cet_make_library( LIBRARY_NAME sbndobj_BlipDataTypes
	cet_make_library(

-template class art::Wrapper<art::Assns<blip::Blip,recob::SpacePoint,void> >;
+template class art::Wrapper<std::vector<blip::Blip> >;
+template class art::Wrapper<art::Assns<blip::Blip,recob::Hit> >;
+template class art::Wrapper<art::Assns<blip::Blip,recob::SpacePoint> >;

Feature/adding blip to caf #155

Are you sure you want to change the base?

Feature/adding blip to caf #155

Uh oh!

Conversation

Jjm321814 commented Nov 10, 2025

Uh oh!

Jjm321814 commented Nov 10, 2025

Uh oh!

kjplows commented Nov 12, 2025

Uh oh!

kjplows commented Nov 12, 2025

Uh oh!

Jjm321814 commented Nov 13, 2025

Uh oh!

Jjm321814 commented Nov 13, 2025

Uh oh!

Jjm321814 commented Nov 14, 2025

Uh oh!

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

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!

Jjm321814 commented Nov 29, 2025

Uh oh!

kjplows commented Dec 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants