Feature/cerati ng2caf #532
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -544,6 +544,26 @@ namespace caf | |||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| void FillSliceNuGraph(const std::vector<art::Ptr<recob::Hit>> &inputHits, | ||||||
| const std::vector<unsigned int> &sliceHitsMap, | ||||||
| const std::vector<art::Ptr<anab::FeatureVector<1>>> &ngFilterResult, | ||||||
| const std::vector<art::Ptr<anab::FeatureVector<5>>> &ngSemanticResult, | ||||||
| caf::SRSlice &slice) | ||||||
| { | ||||||
|
|
||||||
| //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! | ||||||
|
There was a problem hiding this comment. should this be a silent error or would it be a sign of a bigger problem? There was a problem hiding this comment. It's not an error. We just need to skip the slices that were not considered by NuGraph There was a problem hiding this comment. Oh right, since NuGraph only runs on one slice! |
||||||
|
|
||||||
| unsigned int npass = 0; | ||||||
| for ( unsigned int i = 0; i < nHits; i++ ) { | ||||||
| if (ngFilterResult.at(i)->at(0)>0.5) npass++; | ||||||
|
There was a problem hiding this comment. Hard-coded There was a problem hiding this comment. Usually the cut value is included:
Suggested change
Of course practically it seldom matters, but it would be good to pick a convention and stick with it (below there is a |
||||||
| } | ||||||
| slice.ng_filt_pass_frac = float(npass)/nHits; | ||||||
| } | ||||||
|
|
||||||
| //...................................................................... | ||||||
|
|
||||||
| void FillTrackCRTHit(const std::vector<art::Ptr<anab::T0>> &t0match, | ||||||
|
|
@@ -950,6 +970,59 @@ namespace caf | |||||
| srpfp.cnnscore.nclusters = cnnscore->nClusters; | ||||||
| } | ||||||
|
|
||||||
| void FillPPFNuGraph(const std::vector<unsigned int> &sliceHitsMap, | ||||||
|
There was a problem hiding this comment. I just noticed this is |
||||||
| 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, | ||||||
| caf::SRPFP& srpfp, | ||||||
| bool allowEmpty) | ||||||
| { | ||||||
|
|
||||||
| auto arg_max = [](std::vector<float> const& vec) { | ||||||
| return std::distance(vec.begin(), std::max_element(vec.begin(), vec.end())); | ||||||
| }; | ||||||
|
There was a problem hiding this comment. 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)) There was a problem hiding this comment. 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 was a problem hiding this comment. There is nothing about this case in our guidelines. |
||||||
|
|
||||||
| // the nugraph elements are ordered the same as the sliceHitsMap | ||||||
| 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); | ||||||
| } | ||||||
| } | ||||||
|
Comment on lines
+1036
to
+1043
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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 |
||||||
|
|
||||||
| if (mappedhits.size()>0) { | ||||||
| std::vector<float> ng2sempfpcounts(5,0); | ||||||
| size_t ng2bkgpfpcount = 0; | ||||||
| for (size_t pos : mappedhits) { | ||||||
| auto scores = ngSemanticResult.at(pos); | ||||||
| std::vector<float> ng2semscores; | ||||||
| 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); | ||||||
|
There was a problem hiding this comment. Avoid unnecessary copies:
Suggested change
(this is probably still fine, since it's implemented as a static array). |
||||||
| if (bkgscore->at(0)<0.5) ng2bkgpfpcount++; | ||||||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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 There was a problem hiding this comment. 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? There was a problem hiding this comment. yes, the current training is considering them signal but this is going to change soon! There was a problem hiding this comment. 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? There was a problem hiding this comment. 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! |
||||||
| } | ||||||
| srpfp.ng_sem_cat = arg_max(ng2sempfpcounts); | ||||||
| srpfp.ng_mip_frac = float(ng2sempfpcounts[0])/pfpHits.size(); | ||||||
| srpfp.ng_hip_frac = float(ng2sempfpcounts[1])/pfpHits.size(); | ||||||
| srpfp.ng_shr_frac = float(ng2sempfpcounts[2])/pfpHits.size(); | ||||||
| srpfp.ng_mhl_frac = float(ng2sempfpcounts[3])/pfpHits.size(); | ||||||
| srpfp.ng_dif_frac = float(ng2sempfpcounts[4])/pfpHits.size(); | ||||||
| srpfp.ng_bkg_frac = float(ng2bkgpfpcount)/pfpHits.size(); | ||||||
| } else { | ||||||
| srpfp.ng_sem_cat = -1; | ||||||
|
There was a problem hiding this comment. See the |
||||||
| srpfp.ng_mip_frac = -1.; | ||||||
| srpfp.ng_hip_frac = -1.; | ||||||
| srpfp.ng_shr_frac = -1.; | ||||||
| srpfp.ng_mhl_frac = -1.; | ||||||
| srpfp.ng_dif_frac = -1.; | ||||||
| srpfp.ng_bkg_frac = -1.; | ||||||
| } | ||||||
|
|
||||||
| } | ||||||
|
|
||||||
| void FillHitVars(const recob::Hit& hit, | ||||||
| unsigned producer, | ||||||
| const recob::SpacePoint& spacepoint, | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the inclusion of
FillRecoprevents this from being an actual problem, but I am trying to recall if our style guidelines suggest thatshould still be included here too since
FeatureVectoris used directly in CAFMaker_module?