Feature/aantonak crtveto caf #578

aantonakis · 2025-09-18T03:28:45Z

Description

Please provide a detailed description of the changes this pull request introduces. If available, also link to a docdb link where the issue/change have been presented on/discussed.

This PR adds the SBND CRT Veto from reco2 to CAF. It needs a Standard Record CRT Veto class to propagate the Veto variables. This adds branches to the recTree that consist of boolean values at the Event level and limited information related to the CRT SpacePoints that were involved in triggering the analysis veto. Here is a link to the original PR for reco2: SBNSoftware/sbndcode#707

Additionally, here is a docdb entry: SBN Document 40318-v1

Checklist Items:

[yes] Have you added a label? (bug/enhancement/physics etc.)
[yes] Have you assigned at least 1 reviewer?
[maybe] Is this PR related to an open issue / project?
May be part of the Spring Production ?
[yes] Does this PR affect CAF data format? If so, please assign a CAF maintainer as additional reviewer.
May need to update reviewers?
[yes] Does this PR require merging another PR in a different repository (such as sbnanobj/sbnobj etc.)? If so, please link it in the description.
It requires sbnanobj: Feature/aantonak crtveto caf sbnanaobj#159
[No] Are you submitting this PR on behalf of someone else who made the code changes? If so, please mention them in the description.

…ree yet

…how empty associations are handled.

henrylay97

Thanks for this work Alex, very grateful this is making it to CAF now!

Looks like there are a few merge conflict resolutions have left a few missing curly brackets.

Also a couple of suggestions for cleaning up the main section that accesses the products. Arguably picky but it's better to make it resilient now.

sbncode/CAFMaker/CAFMakerParams.h

henrylay97 · 2025-09-18T09:04:42Z

sbncode/CAFMaker/CAFMaker_module.cc

+  	// And associated SpacePoint objects
+	art::FindManyP<sbnd::crt::CRTSpacePoint> spAssoc(sbndcrtveto_handle, evt, fParams.SBNDCRTVetoLabel());


Sort indentation

Code structure has changed a bit in the latest commit but I believe this issue is gone

henrylay97 · 2025-09-18T09:06:20Z

sbncode/CAFMaker/CAFMaker_module.cc

+	  // Assume there is only one Veto per event    
+	  const std::vector<art::Ptr<sbnd::crt::CRTSpacePoint>> veto_sp_v(spAssoc.at(0)); 


I don't really like the hard-coded use of 0 as the key even if we know one is produced.

Would it be possible to check the size of the handle and then get the art::Ptr<> to the veto object and use the key from that?

Is there only one associated pointer? then use art::FindOneP (CL-04); or art::FindOne if you don't care of the art pointer but only of the data product.

In the latest commit I check the size of the veto vector and pass the key at index 0 from the vector of veto Ptrs. I stuck with FindManyP for the moment. Will FindOne or FindOneP work if there are multiple space points per veto? If so I can try to make that work but it seemed like it would grab just a single space point from my first attempt. Latest commit still uses FindManyP

Yeah, FindOneP is indeed not appropriate here due to the 1-to-many relationship between CRTVeto and CRTSpacePoints. The handling of the single CRTVeto-per-event is now improved though, thank you!

henrylay97 · 2025-09-18T09:07:11Z

sbncode/CAFMaker/CAFMaker_module.cc

+            // TODO ? Need to decide how to handle this
+            // This version seems to work in skipping empty associations. It does break the rec.sbnd_crt_veto.sp_pe..length and 
+            // rec.sbnd_crt_veto.sp_time..length branches? However, the rec.sbnd_crt_veto.sp_position..length still works and can be used?
+            std::cout << "No Veto Space Points" << std::endl;
+            //vetoSpacePoints.emplace_back(); // nullptr 


I don't understand this - shouldn't need a special case, the vector branches should just be size 0?

Also in the final version remove the print statement :)

Should be resolved. I have a few new debug statements momentarily that will be removed soon. I was testing the functionality of passing the vector of space point Ptrs directly. That block of code might change with new comments and I'll remove any print out statements after that

henrylay97 · 2025-09-18T09:08:09Z

sbncode/CAFMaker/CAFMaker_module.cc

+            for (auto const& sp: veto_sp_v) {
+
+              vetoSpacePoints.push_back(*sp);
+
+            }


Why do we need this? We can pass the vector of art::Ptr<>'s

should be resolved

henrylay97 · 2025-09-18T09:08:43Z

sbncode/CAFMaker/CAFMaker_module.cc

  rec.ncrt_spacepoints = srcrtspacepoints.size();
  rec.sbnd_crt_tracks  = srsbndcrttracks;
  rec.nsbnd_crt_tracks = srsbndcrttracks.size();
+  rec.sbnd_crt_veto  = srsbndcrtveto;


This is me nit-picking but can you align = as all the others are ;P

Yes of course, good catch thanks!

should be resolved

sbncode/CAFMaker/FillReco.cxx

henrylay97 · 2025-09-18T09:09:36Z

sbncode/CAFMaker/FillReco.h

                        bool allowEmpty = false);
+
+  void FillSBNDCRTVeto(const sbnd::crt::CRTVeto &veto,
+		       const std::vector<sbnd::crt::CRTSpacePoint> &points,


Indentation

…ecked the veto size is exactly 1 before passing index 0 to other parts of the code

henrylay97 · 2025-09-25T12:27:47Z

sbncode/CAFMaker/CAFMaker_module.cc

+          if (spAssoc.isValid()) {
+	    // There is one vector of SpacePoints per Veto --> can be empty if no veto condition was satisfied     
+	    const std::vector<art::Ptr<sbnd::crt::CRTSpacePoint>> veto_sp_v(spAssoc.at(vetoPtrs[0].key())); 
+            FillSBNDCRTVeto(sbndcrtvetos[0], veto_sp_v, srsbndcrtveto);


Any reason why you can just pass vetoPtrs[0] here? Save having a vector of pointers and a vector of non-pointers?

Yes, I was actually thinking the same thing but tried to make minimal changes first to see what feedback I would get. I will try to get this changed today, thanks Henry!

henrylay97 · 2025-09-25T12:27:49Z

sbncode/CAFMaker/CAFMaker_module.cc

+	  // Assume there is only one Veto per event    
+	  const std::vector<art::Ptr<sbnd::crt::CRTSpacePoint>> veto_sp_v(spAssoc.at(0)); 


Yeah, FindOneP is indeed not appropriate here due to the 1-to-many relationship between CRTVeto and CRTSpacePoints. The handling of the single CRTVeto-per-event is now improved though, thank you!

henrylay97 · 2025-09-25T12:29:07Z

sbncode/CAFMaker/CAFMaker_module.cc

+  	  // And associated SpacePoint objects
+	  art::FindManyP<sbnd::crt::CRTSpacePoint> spAssoc(sbndcrtveto_handle, evt, fParams.SBNDCRTVetoLabel());
+          if (spAssoc.isValid()) {
+	    // There is one vector of SpacePoints per Veto --> can be empty if no veto condition was satisfied     


Across quite a few locations there are still quite a few indentation issues, this probably comes from tab size in the editor you are using - try and untabify to solve.

I think you're right. I see something completely different on my end. The indentation shown here doesn't look like what's shown in my text editor (I've been using vim)

Still needs resolving

…ng still

…eds to be tested with sbndcode.

henrylay97

I'm now happy with this subject to sorting indentation issues.

Thanks for working on the review comments @aantonakis!

henrylay97 · 2025-10-23T08:22:19Z

sbncode/CAFMaker/CAFMaker_module.cc

+  	  // And associated SpacePoint objects
+	  art::FindManyP<sbnd::crt::CRTSpacePoint> spAssoc(sbndcrtveto_handle, evt, fParams.SBNDCRTVetoLabel());
+          if (spAssoc.isValid()) {
+	    // There is one vector of SpacePoints per Veto --> can be empty if no veto condition was satisfied     


Still needs resolving

henrylay97 · 2025-10-23T08:22:48Z

sbncode/CAFMaker/FillReco.h

+
+  void FillSBNDCRTVeto(const art::Ptr<sbnd::crt::CRTVeto> &veto,
+		       const std::vector<art::Ptr<sbnd::crt::CRTSpacePoint>> &points,
+                       caf::SRSBNDCRTVeto &srsbndcrtveto,
+                       bool allowEmpty = false);


More indentation

henrylay97 · 2025-10-23T08:23:05Z

sbncode/CAFMaker/FillReco.cxx

+  void FillSBNDCRTVeto(const art::Ptr<sbnd::crt::CRTVeto> &veto,
+		       const std::vector<art::Ptr<sbnd::crt::CRTSpacePoint>> &points,
+                       caf::SRSBNDCRTVeto &srsbndcrtveto,
+                       bool allowEmpty)


Indentation

PetrilloAtWork

I request the removal of art::Ptr vector in the implementation.
Creating those vectors causes a overhead and in most of the cases is not necessary and then becomes a bad example (in fact, if you could tell where you have seen that used, we may also consider fixing the inspiring code as needed).

Also the indentation needs to be fixed, but @henrylay97 has already covered that.

PetrilloAtWork · 2025-10-23T21:47:04Z

sbncode/CAFMaker/CAFMaker_module.cc

+      std::vector<art::Ptr<sbnd::crt::CRTVeto>> vetoPtrs;
+      // fill into event
+      if (sbndcrtveto_handle.isValid()) {
+        art::fill_ptr_vector(vetoPtrs, sbndcrtveto_handle);
+	// Only one valid veto per event
+        if (vetoPtrs.size() == 1) {


Please do not create art pointers unless they are really necessary.
Here:

Suggested change

std::vector<art::Ptr<sbnd::crt::CRTVeto>> vetoPtrs;

// fill into event

if (sbndcrtveto_handle.isValid()) {

art::fill_ptr_vector(vetoPtrs, sbndcrtveto_handle);

// Only one valid veto per event

if (vetoPtrs.size() == 1) {

// fill into event

if (sbndcrtveto_handle.isValid()) {

// Only one valid veto per event

if (sbndcrtveto_handle->size() == 1) {

and then pass to the function a simple sbnd::crt::CRTVeto const* pointer (&sbndcrtveto_handle->front()) or, better, a reference sbnd::crt::CRTVeto const& (sbndcrtveto_handle->front()).
The reference is "better" than a pointer in that the latter would convey the message that the object could be absent (null pointer), which is not the case in your code.

PetrilloAtWork · 2025-10-23T21:53:47Z

sbncode/CAFMaker/CAFMaker_module.cc

+	  art::FindManyP<sbnd::crt::CRTSpacePoint> spAssoc(sbndcrtveto_handle, evt, fParams.SBNDCRTVetoLabel());
+          if (spAssoc.isValid()) {
+	    // There is one vector of SpacePoints per Veto --> can be empty if no veto condition was satisfied     
+	    const std::vector<art::Ptr<sbnd::crt::CRTSpacePoint>> veto_sp_v(spAssoc.at(vetoPtrs[0].key())); 


You are asking the key() of the art pointer created from the first element of a data product. The value of such key is always 0.
Also, avoid copying the vector of space point pointers: use a reference.

Suggested change

const std::vector<art::Ptr<sbnd::crt::CRTSpacePoint>> veto_sp_v(spAssoc.at(vetoPtrs[0].key()));

const std::vector<art::Ptr<sbnd::crt::CRTSpacePoint>>& veto_sp_v(spAssoc.at(0));

PetrilloAtWork · 2025-10-23T21:57:06Z

sbncode/CAFMaker/FillReco.cxx

+      srsbndcrtveto.sp_time.emplace_back(sp->Ts0()); /// ns for SBND CRT SpacePoints   
+      srsbndcrtveto.sp_pe.emplace_back(sp->PE());   


I suggest the use of good old push_back() when the purpose is just to copy the argument into the vector. emplace_back() works too, but it has the slightly different purpose of creating a new object into the vector rather than copying an existing one.

Suggested change

srsbndcrtveto.sp_time.emplace_back(sp->Ts0()); /// ns for SBND CRT SpacePoints

srsbndcrtveto.sp_pe.emplace_back(sp->PE());

srsbndcrtveto.sp_time.push_back(sp->Ts0()); /// ns for SBND CRT SpacePoints

srsbndcrtveto.sp_pe.push_back(sp->PE());

aantonakis added 4 commits September 9, 2025 00:55

Starting point for CRTVeto in caf

b256c77

Added veto values in a different way for testing

6cbfa8d

Added starting point for sp associations. Does not store info in recT…

3a6116e

…ree yet

Cleaned up debug statements and code structure. Might need to adjust …

9254c24

…how empty associations are handled.

aantonakis requested a review from henrylay97 September 18, 2025 03:28

aantonakis self-assigned this Sep 18, 2025

aantonakis added the enhancement New feature or request label Sep 18, 2025

Merge branch 'develop' into feature/aantonak_crtveto_caf

2d109dc

henrylay97 requested changes Sep 18, 2025

View reviewed changes

linyan-w added this to SBND 2025 Fall Production - Validation Sep 18, 2025

linyan-w moved this to Wait for last validation I patch (for MC) in SBND 2025 Fall Production - Validation Sep 18, 2025

linyan-w linked an issue Sep 18, 2025 that may be closed by this pull request

CRT Veto in CAFs #548

Open

linyan-w moved this from Wait for next validation I patch (for MC) to Wait for validation II in SBND 2025 Fall Production - Validation Sep 23, 2025

kjplows added this to SBN software development Sep 24, 2025

kjplows moved this to Open pull requests in SBN software development Sep 24, 2025

aantonakis and others added 4 commits September 24, 2025 10:20

Merge branch 'develop' into feature/aantonak_crtveto_caf

21befcb

Fixed curly braces from last merge conflict

7e3023c

Removed unnecessary loop. passed veto key directly to association. Ch…

d03d9f6

…ecked the veto size is exactly 1 before passing index 0 to other parts of the code

cleaned up print out statements

387d4ad

henrylay97 reviewed Sep 25, 2025

View reviewed changes

linyan-w moved this from Wait for validation II to Waiting on Reviewer in SBND 2025 Fall Production - Validation Sep 29, 2025

linyan-w moved this from Waiting on Reviewer to Wait for next patch in SBND 2025 Fall Production - Validation Sep 29, 2025

Changed the FillReco function to have just the veto ptrs. Needs testi…

25ef6cc

…ng still

linyan-w moved this from Wait for next patch to Wait for validation II in SBND 2025 Fall Production - Validation Oct 7, 2025

linyan-w moved this from Wait for validation II to Waiting on Reviewer in SBND 2025 Fall Production - Validation Oct 7, 2025

aantonakis and others added 3 commits October 7, 2025 15:43

Merge branch 'develop' into feature/aantonak_crtveto_caf

1ab38e5

Cleaned up comments. Only a vector of veto ptrs is used now. Still ne…

c011954

…eds to be tested with sbndcode.

Merge branch 'develop' into feature/aantonak_crtveto_caf

4aac8d8

henrylay97 approved these changes Oct 23, 2025

View reviewed changes

Changed SpacePoint times back to ns

89516ab

aantonakis added 2 commits October 23, 2025 16:14

Fixed white space for FillReco?

7cdb76e

Fixed white space for CAFMaker module

1b4390b

PetrilloAtWork requested changes Oct 23, 2025

View reviewed changes

		// And associated SpacePoint objects
		art::FindManyP<sbnd::crt::CRTSpacePoint> spAssoc(sbndcrtveto_handle, evt, fParams.SBNDCRTVetoLabel());

		// Assume there is only one Veto per event
		const std::vector<art::Ptr<sbnd::crt::CRTSpacePoint>> veto_sp_v(spAssoc.at(0));

		srsbndcrtveto.sp_time.emplace_back(sp->Ts0()); /// ns for SBND CRT SpacePoints
		srsbndcrtveto.sp_pe.emplace_back(sp->PE());

Uh oh!

Feature/aantonak crtveto caf #578

Are you sure you want to change the base?

Feature/aantonak crtveto caf #578

Conversation

aantonakis commented Sep 18, 2025

Description

Uh oh!

henrylay97 left a comment • 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!

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

henrylay97 left a comment •

edited

Loading