Handle CAF making when Pandora runs after the NuGraph2 filter by rtriozzi · Pull Request #616 · SBNSoftware/sbncode

rtriozzi · 2025-12-05T07:21:25Z

This PR adds a mode governed by the UsePandoraAfterNuGraph FHiCL flag to handle NuGraph2 variables in the CAF maker, when using a second-pass Pandora after filtering noise with NuGraph2, based on work by summer intern Leonardo Lena.

This PR also provides code to fill new NuGraph2 slice-level variables in CAF files.

Relevant information about the NuGraph2 chain is provided in icaruscode PR #868.

Associated PRs

Review

Tagging for review @acampani and @PetrilloAtWork as reconstruction and infrastructure experts. Thanks a lot!

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.

Have you added a label? (bug/enhancement/physics etc.)
Have you assigned at least 1 reviewer?
[] Is this PR related to an open issue / project?
Does this PR affect CAF data format? If so, please assign a CAF maintainer as additional reviewer.
Does this PR require merging another PR in a different repository (such as sbnanobj/sbnobj etc.)? If so, please link it in the description.
Are you submitting this PR on behalf of someone else who made the code changes? If so, please mention them in the description.

cerati

Thanks, overall it looks great -- especially removing the need for the hit mapping makes things much simpler.

The two things I would suggest to change are:

Avoid erasing elements from vectors. I think it should be much easier to simply add a check that the element is non-null in the Fill*NuGraph method. Or is there another reason for erasing the elements with null nugraph score?
There is no need to get the NuGraph slices from the event if then we just use slices (when UsePandoraAfterNuGraph=true)

rtriozzi · 2026-01-12T11:49:35Z

I have addressed Giuseppe’s comments (with which I agree). I also added more code to fill some new NuGraph2 CAF variables we developed. I opened the corresponding sbnanaobj PR #183.

kjplows · 2026-01-15T20:40:13Z

Thanks! @cerati do the changes look good to you?

cerati · 2026-01-15T23:37:45Z

Looks great! I only left a single comment above

cerati

Thanks for addressing my comments!

kjplows · 2026-02-11T15:46:55Z

@PetrilloAtWork sorry to add more to your plate, just bumping this 🙂 -- could you take a look please? Thanks!

kjplows · 2026-03-12T12:52:58Z

@PetrilloAtWork , @acampani -- could you please take a look at this? Thanks.

sparshitadey · 2026-05-30T21:08:19Z

Hi!
Small addition to fix ng_filt_pass_frac (currently always 1.0 in the NuGraph workflow):
In the workflow, slcHits comes from hits that already passed the filter by construction, so ng_filt_pass_frac computed from ngFilterResult is trivially 1.0.

The fix is to compute it directly from collection sizes - see commit 363b5120 on my branch. Gives mean ~0.81, varying event to event, consistent with Stage 1 filter retention (and the printouts). Happy to open a separate PR if preferred!

PetrilloAtWork

My usual burst of comments.
There is at least one which must be addressed, about uninitialised variables.

PetrilloAtWork · 2026-07-08T23:18:47Z

+        nuGraphSlices = slices;
+      } else {
+        nuGraphSlices = evt.getProduct<std::vector<art::Ptr<recob::Slice>>>(fParams.NuGraphSlicesLabel().label() + pandora_tag_suffix);
+      }


I don't understand this logic.
If fParams.UsePandoraAfterNuGraph() is true, nugraphSlices will be the same as slices (so that case could easily live outside the loop); in the other case, nuGraphSlices is overwritten with the content of each tag until the last one, hence holding only slices from the data product with the last tag.
I suspect what was meant to add slices, something like

if (!fParams.UsePandoraAfterNuGraph()) { auto const& moreSlices = evt.getProduct<std::vector<art::Ptr<recob::Slice>>>(fParams.NuGraphSlicesLabel().label() + pandora_tag_suffix); nuGraphSlices.insert(nuGraphSlices.end(), moreSlices.begin(), moreSlices.end()); }

PetrilloAtWork · 2026-07-08T23:19:26Z

-	}
-	fmPFPartHits.push_back(pfphits);
+        std::vector<art::Ptr<recob::Hit>> pfphits;
+        std::vector<art::Ptr<recob::Cluster>> pfclusters = fmPFPClusters.at(ipf);


Avoid copies:

Suggested change

std::vector<art::Ptr<recob::Cluster>> pfclusters = fmPFPClusters.at(ipf);

std::vector<art::Ptr<recob::Cluster>> const& pfclusters = fmPFPClusters.at(ipf);

PetrilloAtWork · 2026-07-08T23:26:12Z

+        std::vector<art::Ptr<recob::Cluster>> pfclusters = fmPFPClusters.at(ipf);
+        art::FindManyP<recob::Hit> fmCluHits = FindManyPStrict<recob::Hit>(pfclusters, evt, fParams.PFParticleLabel() + slice_tag_suff);
+        for (size_t icl=0; icl<fmCluHits.size();icl++) {
+          for (auto hit : fmCluHits.at(icl)) {


Avoid copies:

Suggested change

for (auto hit : fmCluHits.at(icl)) {

for (auto const& hit : fmCluHits.at(icl)) {

PetrilloAtWork · 2026-07-08T23:31:57Z

+      art::FindOneP<anab::FeatureVector<1>> findOneFilter(slcHits, evt, fParams.NuGraphFilterLabel().label() + slice_tag_suff + ":" + fParams.NuGraphFilterLabel().instance());
+      art::FindOneP<anab::FeatureVector<5>> findOneSemantic(slcHits, evt, fParams.NuGraphSemanticLabel().label() + slice_tag_suff + ":" + fParams.NuGraphSemanticLabel().instance());


It's hard to see whether the labels are the same. Define the tag first:

Suggested change

art::FindOneP<anab::FeatureVector<1>> findOneFilter(slcHits, evt, fParams.NuGraphFilterLabel().label() + slice_tag_suff + ":" + fParams.NuGraphFilterLabel().instance());

art::FindOneP<anab::FeatureVector<5>> findOneSemantic(slcHits, evt, fParams.NuGraphSemanticLabel().label() + slice_tag_suff + ":" + fParams.NuGraphSemanticLabel().instance());

art::InputTag const filtTag{

fParams.NuGraphFilterLabel().label() + slice_tag_suff,

fParams.NuGraphFilterLabel().instance()

};

art::FindOneP<anab::FeatureVector<1>> findOneFilter(slcHits, filtTag);

art::FindOneP<anab::FeatureVector<5>> findOneSemantic(slcHits, filtTag);

PetrilloAtWork · 2026-07-08T23:41:14Z

+      float vtx_wire[3];
+      float vtx_tick[3];
+
+      if (vertex != NULL) {


What kind of garbage is going to end into the tree if the vertex is null? The arrays need to be initialised somehow.
This is a request, as we don't want uninitialised values to end into the trees as it compromises tree comparisons.
Also, NULL is C, and we need to keep a façade here.

Suggested change

if (vertex != NULL) {

if (vertex) {

going through the comments right now! about this: every slice has to have a vertex, sort of by definition in Pandora (e.g., even if the reconstruction is sure that the slice is a cosmic, the vertex will just be the highest-y hit)

I think there are also protections for failed vertexing (which should be very rare: we really just need a hit to define a vertex...) within Pandora, so that we always get something when we grab the vertex in the CAF maker...

anyway, I agree it's bad practice from the CAF maker stand point (+ the NULL check was therefore strange), I'll fix this! thanks!

PetrilloAtWork · 2026-07-09T00:20:24Z

+          std::max_element(semVec.begin(), semVec.end())
+        );
+
+        // `MIP` hits


This sequence may be better served with a switch block.

PetrilloAtWork · 2026-07-09T00:22:48Z

+      slice.ng_plane[plane].mip_hits = nMIPHits;
+      slice.ng_plane[plane].hip_hits = nHIPHits;
+      slice.ng_plane[plane].shr_hits = nShrHits;
+      slice.ng_plane[plane].mhl_hits = nMhlHits;
+      slice.ng_plane[plane].dif_hits = nDifHits;
+      slice.ng_plane[plane].ng_vtx_hip_hits = nVtxHIPHits;
+      slice.ng_plane[plane].unclustered_shr_hits = nUnclusteredShrHits;


Suggested change

slice.ng_plane[plane].mip_hits = nMIPHits;

slice.ng_plane[plane].hip_hits = nHIPHits;

slice.ng_plane[plane].shr_hits = nShrHits;

slice.ng_plane[plane].mhl_hits = nMhlHits;

slice.ng_plane[plane].dif_hits = nDifHits;

slice.ng_plane[plane].ng_vtx_hip_hits = nVtxHIPHits;

slice.ng_plane[plane].unclustered_shr_hits = nUnclusteredShrHits;

auto& hitInfo = slice.ng_plane[plane];

hitInfo.mip_hits = nMIPHits;

hitInfo.hip_hits = nHIPHits;

hitInfo.shr_hits = nShrHits;

hitInfo.mhl_hits = nMhlHits;

hitInfo.dif_hits = nDifHits;

hitInfo.ng_vtx_hip_hits = nVtxHIPHits;

hitInfo.unclustered_shr_hits = nUnclusteredShrHits;

PetrilloAtWork · 2026-07-09T00:23:57Z

-	  size_t sem_label = std::distance(ng2semscores.begin(), std::max_element(ng2semscores.begin(), ng2semscores.end()));//arg_max(ng2semscores);
-	  ng2sempfpcounts[sem_label]++;
-	}
+        if (ngFilterResult.at(pos).isNull()) continue;


Indentation seems off.

PetrilloAtWork · 2026-07-09T00:25:01Z

+          std::vector<float> ng2semscores;
+          for (size_t i=0;i<scores->size();i++) ng2semscores.push_back(scores->at(i));


Suggested change

std::vector<float> ng2semscores;

for (size_t i=0;i<scores->size();i++) ng2semscores.push_back(scores->at(i));

std::vector<float> ng2semscores{ scores.begin(), scores.end() };

PetrilloAtWork · 2026-07-09T00:25:45Z

 		      const std::vector<art::Ptr<anab::FeatureVector<1>>> &ngFilterResult,
 		      const std::vector<art::Ptr<anab::FeatureVector<5>>> &ngSemanticResult,
-		      const std::vector<art::Ptr<recob::Hit>> &pfpHits,
+          const float filter_cut,


Fix the indentation.

leonardo-lena added 3 commits October 6, 2025 15:24

Updated CAFMaker to work with NuGraph inference on multiple slices.

351d379

Fixed memory leak issues.

762e6ed

FEATURE: implemented CAFMaker compatibility with PandoraAfterNuGraph.

78a0667

rtriozzi requested review from PetrilloAtWork and acampani December 5, 2025 07:21

rtriozzi assigned cerati and rtriozzi Dec 5, 2025

rtriozzi added the enhancement New feature or request label Dec 5, 2025

rtriozzi mentioned this pull request Dec 5, 2025

Introduce multi-slice NuGraph2 inference and filtering SBNSoftware/icaruscode#868

Open

cerati requested changes Dec 8, 2025

View reviewed changes

kjplows added this to SBN software development Dec 14, 2025

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

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

rtriozzi added 6 commits December 24, 2025 11:54

fill nugraph hip tagger

d5d32a0

change to int to allow for negative default

cf283cb

more shower vars

98a4654

move HIP tagging params to fcl

612ae6a

move more params to fcl, address Giuseppe's comments

55812bc

removed spare comments

d483222

rtriozzi mentioned this pull request Jan 12, 2026

Add more NuGraph2 variables to CAFs SBNSoftware/sbnanaobj#183

Open

4 tasks

cerati reviewed Jan 15, 2026

View reviewed changes

Comment thread sbncode/CAFMaker/FillReco.cxx Outdated

use NuGraphCategory enum and add more vars

deb9088

cerati approved these changes Jan 16, 2026

View reviewed changes

kjplows added this to PR archaeology Feb 11, 2026

github-project-automation Bot moved this to Todo in PR archaeology Feb 11, 2026

kjplows moved this from Todo to In Progress in PR archaeology Feb 11, 2026

added guard for tpcID

6aa4a4c

rtriozzi force-pushed the feature/llena_CAFMaker_PandoraAfterNuGraph branch from 3017213 to 6aa4a4c Compare March 16, 2026 21:26

acampani reviewed Mar 23, 2026

View reviewed changes

Comment thread sbncode/CAFMaker/CAFMaker_module.cc

acampani reviewed Mar 23, 2026

View reviewed changes

Comment thread sbncode/CAFMaker/CAFMakerParams.h

rtriozzi added 2 commits June 4, 2026 09:44

add post-NG2-filter filtered hits count

074258e

correct typo

0800060

PetrilloAtWork requested changes Jul 9, 2026

View reviewed changes

	std::vector<art::Ptr<recob::Cluster>> pfclusters = fmPFPClusters.at(ipf);
	std::vector<art::Ptr<recob::Cluster>> const& pfclusters = fmPFPClusters.at(ipf);

	for (auto hit : fmCluHits.at(icl)) {
	for (auto const& hit : fmCluHits.at(icl)) {

		art::FindOneP<anab::FeatureVector<1>> findOneFilter(slcHits, evt, fParams.NuGraphFilterLabel().label() + slice_tag_suff + ":" + fParams.NuGraphFilterLabel().instance());
		art::FindOneP<anab::FeatureVector<5>> findOneSemantic(slcHits, evt, fParams.NuGraphSemanticLabel().label() + slice_tag_suff + ":" + fParams.NuGraphSemanticLabel().instance());

-      art::FindOneP<anab::FeatureVector<1>> findOneFilter(slcHits, evt, fParams.NuGraphFilterLabel().label() + slice_tag_suff + ":" + fParams.NuGraphFilterLabel().instance());
-      art::FindOneP<anab::FeatureVector<5>> findOneSemantic(slcHits, evt, fParams.NuGraphSemanticLabel().label() + slice_tag_suff + ":" + fParams.NuGraphSemanticLabel().instance());
+      art::InputTag const filtTag{
+        fParams.NuGraphFilterLabel().label() + slice_tag_suff,
+        fParams.NuGraphFilterLabel().instance()
+        };
+      art::FindOneP<anab::FeatureVector<1>> findOneFilter(slcHits, filtTag);
+      art::FindOneP<anab::FeatureVector<5>> findOneSemantic(slcHits, filtTag);

-      slice.ng_plane[plane].mip_hits = nMIPHits;
-      slice.ng_plane[plane].hip_hits = nHIPHits;
-      slice.ng_plane[plane].shr_hits = nShrHits;
-      slice.ng_plane[plane].mhl_hits = nMhlHits;
-      slice.ng_plane[plane].dif_hits = nDifHits;
-      slice.ng_plane[plane].ng_vtx_hip_hits = nVtxHIPHits;
-      slice.ng_plane[plane].unclustered_shr_hits = nUnclusteredShrHits;
+      auto& hitInfo = slice.ng_plane[plane];
+      hitInfo.mip_hits = nMIPHits;
+      hitInfo.hip_hits = nHIPHits;
+      hitInfo.shr_hits = nShrHits;
+      hitInfo.mhl_hits = nMhlHits;
+      hitInfo.dif_hits = nDifHits;
+      hitInfo.ng_vtx_hip_hits = nVtxHIPHits;
+      hitInfo.unclustered_shr_hits = nUnclusteredShrHits;

		std::vector<float> ng2semscores;
		for (size_t i=0;i<scores->size();i++) ng2semscores.push_back(scores->at(i));

	std::vector<float> ng2semscores;
	for (size_t i=0;i<scores->size();i++) ng2semscores.push_back(scores->at(i));
	std::vector<float> ng2semscores{ scores.begin(), scores.end() };

Uh oh!

Conversation

rtriozzi commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Associated PRs

Review

Description

Uh oh!

cerati left a comment

Choose a reason for hiding this comment

Uh oh!

rtriozzi commented Jan 12, 2026

Uh oh!

kjplows commented Jan 15, 2026

Uh oh!

Uh oh!

cerati commented Jan 15, 2026

Uh oh!

cerati left a comment

Choose a reason for hiding this comment

Uh oh!

kjplows commented Feb 11, 2026

Uh oh!

kjplows commented Mar 12, 2026

Uh oh!

Uh oh!

Uh oh!

sparshitadey commented May 30, 2026

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!

rtriozzi Jul 9, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rtriozzi commented Dec 5, 2025 •

edited

Loading

rtriozzi Jul 9, 2026 •

edited

Loading