Handle CAF making when Pandora runs after the NuGraph2 filter#616
Handle CAF making when Pandora runs after the NuGraph2 filter#616
Conversation
cerati
left a comment
There was a problem hiding this comment.
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(whenUsePandoraAfterNuGraph=true)
|
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. |
|
Thanks! @cerati do the changes look good to you? |
sbncode/CAFMaker/FillReco.cxx
Outdated
| nHIPHits += 1; | ||
|
|
||
| // shower hits | ||
| if (highestScoreIdx == 2) { |
There was a problem hiding this comment.
Please use the NuGraphCategory enum here (and in similar places I might have missed): https://github.com/SBNSoftware/sbnanaobj/blob/develop/sbnanaobj/StandardRecord/SRNuGraphScore.h#L23
There was a problem hiding this comment.
Right, thanks! Fixed (and sneaked in some more variables)!
|
Looks great! I only left a single comment above |
cerati
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments!
|
@PetrilloAtWork sorry to add more to your plate, just bumping this 🙂 -- could you take a look please? Thanks! |
|
@PetrilloAtWork , @acampani -- could you please take a look at this? Thanks. |
3017213 to
6aa4a4c
Compare
There was a problem hiding this comment.
I think the changes made here are overall consistent with my expectations and I didn't find anything specific explicitly wrong. I have just a couple of clarifications to ask:
-if the option to run twice Pandora reconstruction (one as input to nu-graph and one after nu-graph, profiting of the semantic classification) is enabled, then nu-graph slices will coincide with the final set of slices produced in the reconstruction so this case is clear;
- if the option to run Pandora reconstruction twice is not selected then nu-graph slices will basically be identical to those Pandora generated in its first (and unique) run with additional features saved at hit-level, correct?
Then I think the rest of the changes are meant to ensure all the semantic classification output from nu-graph is properly assigned to each slice and later on to each hit (and I guess here the same loop over nu-graph slices is repeated at pfp level because that's where info are stored, i.e. at slice level - I wonder if the second loop over nu-graph slices can be avoided, but perhaps this is not the case and I haven't add a chance to look at the details of this part of the code - could this be quickly checked? since I believe the loop is nested in a way that could allow this).
There was a problem hiding this comment.
Also I wonder why slice vertex-related info are highlighted as a change with respect to the previous code version but I hope it's an accident (this info should not have been changed based on nu-graph correct?)
There was a problem hiding this comment.
Yes, the interpretation is correct. The flag was needed because the slices in the two paths change a bit, but we want NuGraph2 variables in the CAF regardless of the chosen reconstruction path. The NuGraph2 PID is very strong, and can be helpful for selections even if the reconstruction is the classic Pandora one.
Some of the variables I added rely on the vertex information (e.g., the HIP tagger on the three planes), so I had to move some things here and there to make sure, e.g., that the vertex information was retrieved before I needed it. Effectively, there are no changes for other CAF variables.
There was a problem hiding this comment.
I guess I have only a curiosity on how the TPC wire (tick) distance default is decided (a presentation to look at/a short explanation would suffice for me thanks!)
There was a problem hiding this comment.
The default values sort of hint at a "3 cm" range around the reconstructed vertex. How such a distance translates onto the different wire planes is not obvious, so I just set default numbers that kind of make sense. They can be tuned at the FHiCL level, if anyone wants to play with the variables. An application of this is to reject HIP activity at the vertex, for example for selecting 1e0p0π final states; you can find something along these lines here (+ data/MC comparisons): https://sbn-docdb.fnal.gov/cgi-bin/sso/RetrieveFile?docid=45736&filename=EventSelection_20260220.pdf&version=1.
This PR adds a mode governed by the
UsePandoraAfterNuGraphFHiCL 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.