Feature/cerati ng2caf #532

cerati · 2025-04-11T03:41:36Z

Description

This PR adds a few NuGraph-related variables to CAFs, along the lines of what presented on docdb 40585. Once icaruscode PR815 is merged the needed data products can be produced, but it won't break CAF making if they are not there. Requires sbnanaobj PR137.

brucehoward-physics · 2025-05-05T17:39:04Z

sbncode/CAFMaker/CAFMaker_module.cc

@@ -95,6 +95,7 @@
 #include "lardataobj/RecoBase/Vertex.h"
 #include "lardataobj/RecoBase/Shower.h"
 #include "lardataobj/RecoBase/MCSFitResult.h"
+#include "lardataobj/RecoBase/Cluster.h"


I guess the inclusion of FillReco prevents this from being an actual problem, but I am trying to recall if our style guidelines suggest that

#include "lardataobj/AnalysisBase/MVAOutput.h"

should still be included here too since FeatureVector is used directly in CAFMaker_module?

brucehoward-physics · 2025-05-06T04:42:38Z

sbncode/CAFMaker/CAFMaker_module.cc

@@ -95,6 +95,7 @@
 #include "lardataobj/RecoBase/Vertex.h"
 #include "lardataobj/RecoBase/Shower.h"
 #include "lardataobj/RecoBase/MCSFitResult.h"
+#include "lardataobj/RecoBase/Cluster.h"


I'm only leaving this as comments for now as I have more pieces to look at and so don't want to leave an actual "review" result yet, but I'll put a couple thoughts here.

I guess the inclusion of FillReco prevents this from being an actual problem, but I'm trying to recall if our style guidelines suggest that you should actually put

#include "lardataobj/AnalysisBase/MVAOutput.h"

here as an include too since FeatureVector is directly used in the CAFMaker module.

sure, I'll add this include!

Thanks for adding!

brucehoward-physics · 2025-05-06T04:44:02Z

sbncode/CAFMaker/FillReco.cxx

+    //need to double check that the slice processed by NuGraph is the same under consideration
+    //std::cout << "sizes=" << inputHits.size() << " " << sliceHitsMap.size() << " " << ngFilterResult.size() << " " << ngSemanticResult.size() << std::endl;
+    unsigned int nHits = inputHits.size();
+    if (nHits==0 || nHits!=sliceHitsMap.size() || inputHits[0].key()!=sliceHitsMap[0]) return;//not the same slice!


should this be a silent error or would it be a sign of a bigger problem?

It's not an error. We just need to skip the slices that were not considered by NuGraph

Oh right, since NuGraph only runs on one slice!

brucehoward-physics · 2025-05-06T04:44:38Z

sbncode/CAFMaker/FillReco.cxx

+    auto arg_max = [](std::vector<float> const& vec) {
+      return std::distance(vec.begin(), std::max_element(vec.begin(), vec.end()));
+    };


Another question on the SBN coding conventions: are lambdas okay within other methods? (If @cerati himself isn't sure then maybe @PetrilloAtWork is the best person to answer this (especially as he is also tagged as the other reviewer))

This is resolved by Giuseppe's recent commit, which just uses this one-liner in place of the 2 lines this function would have been used.

There is nothing about this case in our guidelines.
Personally I do recommend the use of lambdas in methods where it makes sense (typically, to avoid the repetition of simple code). However, if the same lambda appears in different methods, then it should likely be a member function if possible.

brucehoward-physics · 2025-05-06T04:50:33Z

sbncode/CAFMaker/FillReco.cxx

+	for (size_t i=0;i<scores->size();i++) ng2semscores.push_back(scores->at(i));
+	size_t sem_label = arg_max(ng2semscores);
+	ng2sempfpcounts[sem_label]++;
+	auto bkgscore = ngFilterResult.at(pos);
+	if (bkgscore->at(0)<0.5) ng2bkgpfpcount++;


This seems fine, but I was thinking: is there an argument to be made that one should only fill the semantic label fractions for hits that are not tagged as background?

Yes, that's a good point. Ideally this should be studied, to find what works best. In general, the semantic labels can be meaningful also when the filter does not pass (e.g. for cosmics). But since noise is a bigger issues than cosmics (unlike MicroBooNE), your suggestion makes sense and I will implement it

I was thinking more about this. I think I agree with your point if the cosmic hits were considered background. Isn't the current training considering them signal though? Apologies if I'm misremembering?

yes, the current training is considering them signal but this is going to change soon!

Ah then I guess I'm okay with either way, though I suppose in this updated schema the background fraction would still be meaningful to distinguish a cosmic PFP from others? And the semantic label fractions would help distinguish if there is some overlapping particle what it was, no?

right. My point is that for ICARUS noise is likely more of an issue than cosmics, so the semantic labels for the background hits are more likely to be meaningless (as they are more likely to be noise and not real cosmic particles). So the new version makes more sense as we don't want to pollute the semantic counts with noise. But of course the background fraction is still useful to reject cosmics!

…o feature/cerati_ng2caf

brucehoward-physics · 2025-05-07T16:15:40Z

sbncode/CAFMaker/CAFMaker_module.cc

+    art::FindManyP<recob::Cluster> fmPFPClusters =
+      FindManyPStrict<recob::Cluster>(fmPFPart, evt, fParams.PFParticleLabel() + slice_tag_suff);
+
+    std::vector<std::vector<art::Ptr<recob::Hit>>> fmPFPartHits;
+    // make Ptr's to clusters for cluster -> other object associations
+    if (fmPFPClusters.isValid()) {
+      for (size_t ipf=0; ipf<fmPFPart.size();++ipf) {
+	std::vector<art::Ptr<recob::Hit>> pfphits;
+	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)) {
+	    pfphits.push_back(hit);
+	  }
+	}
+	fmPFPartHits.push_back(pfphits);
+      }
+    }
+


I believe we had a quick chat once about using index directly vs using the art Ptr key() and that both of us are more used to key, but you found that index was working okay (and seems to be used elsewhere here). Please remind me if I'm missing details? I wanted to ask just to double check that this is guaranteed to work as desired!

when looping plainly over a collection, the way one would use key is to first create an art::Ptr based on the handle and the index, which guarantees that the key is the same as the index

brucehoward-physics · 2025-05-07T16:26:18Z

sbncode/CAFMaker/FillReco.cxx

@@ -950,6 +970,58 @@ namespace caf
    srpfp.cnnscore.nclusters = cnnscore->nClusters;
  }

+  void FillPPFNuGraph(const std::vector<unsigned int> &sliceHitsMap,


I just noticed this is PPF not PFP

nice catch!

brucehoward-physics · 2025-05-07T16:28:26Z

sbncode/CAFMaker/FillReco.cxx

+    std::vector<size_t> mappedhits;
+    for (auto& hit : pfpHits) {
+      auto it = std::find(sliceHitsMap.begin(), sliceHitsMap.end(), hit.key());
+      if (it != sliceHitsMap.end()) {
+	size_t index = std::distance(sliceHitsMap.begin(), it);
+	mappedhits.push_back(index);
+      }
+    }


Just double checking my understanding, I assume that the NuGraph producer is filling up the map that you grab above with the hit keys for the slice and so then this is why you grab things in this way from the map?

Yes. See icaruscode/NuGraphIcarus/IcarusNuSliceHitsProducer_module.cc being added in the icaruscode PR. NuGraph runs over a hit collection. This hit collection is created based on the hits in the best neutrino slice. This means that NuGraph labels are applied to a hit collection which is a subset of the one used by Pandora. So in order to get the NuGraph label of hits in Pandora PFPs, a mapping is needed

…o feature/cerati_ng2caf

PetrilloAtWork

No mistakes spotted, asking for some documentation and a bit of code cleanup (in particular, documentation about the hard-coded values and what they do).

PetrilloAtWork · 2025-05-07T18:39:27Z

sbncode/CAFMaker/FillReco.cxx

+
+    unsigned int npass = 0;
+    for ( unsigned int i = 0; i < nHits; i++ ) {
+      if (ngFilterResult.at(i)->at(0)>0.5) npass++;


Hard-coded 0.5: can you document it and maybe turn its value into a C++ constant?

PetrilloAtWork · 2025-05-07T18:48:34Z

sbncode/CAFMaker/FillReco.cxx

+
+    unsigned int npass = 0;
+    for ( unsigned int i = 0; i < nHits; i++ ) {
+      if (ngFilterResult.at(i)->at(0)>0.5) npass++;


Usually the cut value is included:

Suggested change

if (ngFilterResult.at(i)->at(0)>0.5) npass++;

if (ngFilterResult.at(i)->at(0)>=0.5) npass++;

Of course practically it seldom matters, but it would be good to pick a convention and stick with it (below there is a < 0.5).

PetrilloAtWork · 2025-05-07T18:48:54Z

sbncode/CAFMaker/FillReco.cxx

+      size_t ng2bkgpfpcount = 0;
+      for (size_t pos : mappedhits) {
+	auto bkgscore = ngFilterResult.at(pos);
+	if (bkgscore->at(0)<0.5) {


"Is this the same 0.5 as above?"
Please use a constant to describe this one too (I suppose it is different than the one before).

PetrilloAtWork · 2025-05-07T19:26:27Z

sbncode/CAFMaker/FillReco.cxx

+      srpfp.ng_dif_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[4])/(pfpHits.size()-ng2bkgpfpcount) : -1.);
+      srpfp.ng_bkg_frac = float(ng2bkgpfpcount)/pfpHits.size();
+    } else {
+      srpfp.ng_sem_cat = -1;


See the enum suggestion on the other PR.

PetrilloAtWork · 2025-05-07T19:32:03Z

sbncode/CAFMaker/FillReco.cxx

+      std::vector<float> ng2sempfpcounts(5,0);
+      size_t ng2bkgpfpcount = 0;
+      for (size_t pos : mappedhits) {
+	auto bkgscore = ngFilterResult.at(pos);


Avoid unnecessary copies:

Suggested change

auto bkgscore = ngFilterResult.at(pos);

auto const& bkgscore = ngFilterResult.at(pos);

(this is probably still fine, since it's implemented as a static array).

sbncode/CAFMaker/FillReco.h

brucehoward-physics · 2025-05-07T21:51:49Z

I think I am happy to consider this approved, based on Giuseppe's responses and changes e.g., once Gianluca is satisfied. I don't know if me hitting "approve" and being otherwise silent will somehow supersede Gianluca's comments in the eyes of @kjplows or other manager, so remarking this here :)

brucehoward-physics

Approved with the comment given about pending Gianluca's satisfaction as well.

kjplows · 2025-05-07T22:03:05Z

trigger build SBNSoftware/sbnanaobj#137

FNALbuild · 2025-05-07T22:06:05Z

✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

FNALbuild · 2025-05-07T22:06:05Z

✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

FNALbuild · 2025-05-07T22:07:05Z

❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the build ICARUS phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2025-05-07T22:07:18Z

❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the build SBND phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2025-05-07T22:20:02Z

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2025-05-07T22:23:05Z

⚠️ CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for e26:prof - ignored failure for unit_test -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

…o feature/cerati_ng2caf

PetrilloAtWork · 2025-05-08T00:13:30Z

sbncode/CAFMaker/FillReco.cxx

+      srpfp.ngscore.mip_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[0])/(pfpHits.size()-ng2bkgpfpcount) : -1.);
+      srpfp.ngscore.hip_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[1])/(pfpHits.size()-ng2bkgpfpcount) : -1.);
+      srpfp.ngscore.shr_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[2])/(pfpHits.size()-ng2bkgpfpcount) : -1.);
+      srpfp.ngscore.mhl_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[3])/(pfpHits.size()-ng2bkgpfpcount) : -1.);
+      srpfp.ngscore.dif_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[4])/(pfpHits.size()-ng2bkgpfpcount) : -1.);


More maintenance- and reader-friendly:

Suggested change

srpfp.ngscore.mip_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[0])/(pfpHits.size()-ng2bkgpfpcount) : -1.);

srpfp.ngscore.hip_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[1])/(pfpHits.size()-ng2bkgpfpcount) : -1.);

srpfp.ngscore.shr_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[2])/(pfpHits.size()-ng2bkgpfpcount) : -1.);

srpfp.ngscore.mhl_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[3])/(pfpHits.size()-ng2bkgpfpcount) : -1.);

srpfp.ngscore.dif_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[4])/(pfpHits.size()-ng2bkgpfpcount) : -1.);

size_t hitExcess = pfpHits.size()>ng2bkgpfpcount ? pfpHits.size()-ng2bkgpfpcount: 0;

srpfp.ngscore.mip_frac = hitExcess > 0 ? float(ng2sempfpcounts[0])/hitExcess : -1.;

srpfp.ngscore.hip_frac = hitExcess > 0 ? float(ng2sempfpcounts[1])/hitExcess : -1.;

srpfp.ngscore.shr_frac = hitExcess > 0 ? float(ng2sempfpcounts[2])/hitExcess : -1.;

srpfp.ngscore.mhl_frac = hitExcess > 0 ? float(ng2sempfpcounts[3])/hitExcess : -1.;

srpfp.ngscore.dif_frac = hitExcess > 0 ? float(ng2sempfpcounts[4])/hitExcess : -1.;

kjplows · 2025-05-08T05:17:47Z

trigger build SBNSoftware/sbnanaobj#137

FNALbuild · 2025-05-08T05:20:48Z

✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

FNALbuild · 2025-05-08T05:20:53Z

✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

FNALbuild · 2025-05-08T05:21:46Z

❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the build SBND phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2025-05-08T05:21:48Z

❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the build ICARUS phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2025-05-08T05:37:56Z

⚠️ CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for e26:prof - ignored failure for unit_test -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard

FNALbuild · 2025-05-08T05:57:27Z

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

cerati added 2 commits April 10, 2025 21:25

add first NuGraph2 info to CAF

eade067

ensure CAF works if NuGraph2 products are not found

33f56fb

cerati added the enhancement New feature or request label Apr 11, 2025

cerati requested review from PetrilloAtWork and brucehoward-physics April 11, 2025 03:41

kjplows added this to SBN software development Apr 17, 2025

kjplows moved this to Open pull requests in SBN software development Apr 17, 2025

Merge branch 'develop' into feature/cerati_ng2caf

bb1e587

brucehoward-physics reviewed May 6, 2025

View reviewed changes

cerati added 2 commits May 6, 2025 12:02

address BH comments

0b5ad31

Merge branch 'feature/cerati_ng2caf' of github.com:cerati/sbncode int…

790ac10

…o feature/cerati_ng2caf

PetrilloAtWork mentioned this pull request May 7, 2025

add first NuGraph2 info to CAF SBNSoftware/sbnanaobj#137

Merged

brucehoward-physics reviewed May 7, 2025

View reviewed changes

kjplows and others added 3 commits May 7, 2025 19:19

Merge branch 'develop' into feature/cerati_ng2caf

8f0c667

fix typo

b36ce76

Merge branch 'feature/cerati_ng2caf' of github.com:cerati/sbncode int…

9e274d8

…o feature/cerati_ng2caf

PetrilloAtWork reviewed May 7, 2025

View reviewed changes

brucehoward-physics approved these changes May 7, 2025

View reviewed changes

kjplows moved this from Open pull requests to Testing in SBN software development May 7, 2025

cerati added 2 commits May 7, 2025 19:00

address GP's comments

0157111

Merge branch 'feature/cerati_ng2caf' of github.com:cerati/sbncode int…

fcff962

…o feature/cerati_ng2caf

PetrilloAtWork reviewed May 8, 2025

View reviewed changes

PetrilloAtWork approved these changes May 8, 2025

View reviewed changes

address GP's comments

cae5b11

kjplows moved this from Testing to To merge in SBN software development May 8, 2025

Merge branch 'develop' into feature/cerati_ng2caf

c7343d5

kjplows merged commit fb52788 into SBNSoftware:develop May 8, 2025

github-project-automation bot moved this from To merge to Done in SBN software development May 8, 2025

kjplows moved this from Done to Recently done in SBN software development May 8, 2025

	if (ngFilterResult.at(i)->at(0)>0.5) npass++;
	if (ngFilterResult.at(i)->at(0)>=0.5) npass++;

	auto bkgscore = ngFilterResult.at(pos);
	auto const& bkgscore = ngFilterResult.at(pos);

-      srpfp.ngscore.mip_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[0])/(pfpHits.size()-ng2bkgpfpcount) : -1.);
-      srpfp.ngscore.hip_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[1])/(pfpHits.size()-ng2bkgpfpcount) : -1.);
-      srpfp.ngscore.shr_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[2])/(pfpHits.size()-ng2bkgpfpcount) : -1.);
-      srpfp.ngscore.mhl_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[3])/(pfpHits.size()-ng2bkgpfpcount) : -1.);
-      srpfp.ngscore.dif_frac = (pfpHits.size()>ng2bkgpfpcount ? float(ng2sempfpcounts[4])/(pfpHits.size()-ng2bkgpfpcount) : -1.);
+      size_t hitExcess = pfpHits.size()>ng2bkgpfpcount ? pfpHits.size()-ng2bkgpfpcount: 0;
+      srpfp.ngscore.mip_frac = hitExcess > 0 ? float(ng2sempfpcounts[0])/hitExcess : -1.;
+      srpfp.ngscore.hip_frac = hitExcess > 0 ? float(ng2sempfpcounts[1])/hitExcess : -1.;
+      srpfp.ngscore.shr_frac = hitExcess > 0 ? float(ng2sempfpcounts[2])/hitExcess : -1.;
+      srpfp.ngscore.mhl_frac = hitExcess > 0 ? float(ng2sempfpcounts[3])/hitExcess : -1.;
+      srpfp.ngscore.dif_frac = hitExcess > 0 ? float(ng2sempfpcounts[4])/hitExcess : -1.;

Feature/cerati ng2caf #532

Feature/cerati ng2caf #532

Uh oh!

Conversation

cerati commented Apr 11, 2025 • edited by PetrilloAtWork Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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!

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!

Uh oh!

brucehoward-physics commented May 7, 2025

Uh oh!

brucehoward-physics left a comment

Choose a reason for hiding this comment

Uh oh!

kjplows commented May 7, 2025

Uh oh!

FNALbuild commented May 7, 2025

Uh oh!

FNALbuild commented May 7, 2025

Uh oh!

FNALbuild commented May 7, 2025

Uh oh!

FNALbuild commented May 7, 2025

cerati commented Apr 11, 2025 •

edited by PetrilloAtWork

Loading