Cerati/feature nugraph2 #815

cerati · 2025-04-10T22:18:22Z

This PR adds the code to run NuGraph2 in ICARUS, as presented on docdb 40585.

NuGraph is not yet added to the reconstruction sequence. It will be done after this PR is integrated and fully tested in grid jobs.

PRs for sbncode and sbnanaobj will follow to add NuGraph information to the CAFs.

Note: this PR has a breaking change, since it splits the sequence icarus_reco_pandoraGausCryoE/W into icarus_reco_pandoraGausCryoE/W+icarus_reco_postPandoraGausCryoE/W. NuGraph is run in the latter, and the split is required since NuGraph needs both pandora and icarus_tpcpmtbarycentermatch to be run upfront. The standard reco fcl files are updated accordingly, but non centrally-maintained fcls may have issues.

cerati · 2025-04-10T22:20:12Z

Looks like this is on top of PR #809 . The two are independent, but hopefully #809 will soon be merged. Only files in icaruscode/NuGraphIcarus need to be reviewed as part of this PR.

cerati · 2025-05-02T18:03:59Z

This PR needs also needs icarus_data branch feature/cerati_NuGprah

Tag for integration release

PetrilloAtWork · 2025-05-07T01:03:09Z

icaruscode/NuGraphIcarus/scripts/setup_tritonserver-nugraph-v0_grid.sh

+    export METRPORT=$((BASEPORT+2))
+done
+#
+echo "physics.producers.NuGraphCryoE.TritonConfig.serverURL: \"localhost:"$GRPCPORT"\"" >> $FCL


I suggest to create a copy of the FHiCL file instead of changing the one specified, to prevent errors where the original FHiCL file is read-only or in storage that does not support appending.

PetrilloAtWork · 2025-05-07T01:10:44Z

icaruscode/NuGraphIcarus/testinference_slice_icarus.fcl

+  TimeTracker:           {}
+  MemoryTracker:         {}
+  RandomNumberGenerator: {}
+  @table::icarus_geometry_services


Geometry services are included in the "common" bundle.

Suggested change

@table::icarus_geometry_services

PetrilloAtWork · 2025-05-07T01:11:36Z

icaruscode/NuGraphIcarus/testinference_slice_icarus.fcl

+ reco: [nuslhitsCryoE, nuslhitsCryoW, NuGraphCryoE, NuGraphCryoW]
+ #reco: [ nuslhitsCryoE, NuGraphCryoE]
+ trigger_paths:       [ reco ]
+
+ ana: [NuGraphAnaCryoE,NuGraphAnaCryoW]
+ #ana: [NuGraphAnaCryoE]
+ streamROOT:         [ rootOutput ]
+ end_paths:          [ ana, streamROOT ]


Please fix the indentation.

PetrilloAtWork · 2025-05-07T01:13:50Z

icaruscode/NuGraphIcarus/nugraph_icarus.fcl

+NuGraphCryoE.avgs_u: [168.04594 , 178.3245  , 266.6149 , 3.857218 ]
+NuGraphCryoE.avgs_v: [1245.3547 , 176.54117 , 323.52786, 4.3267984]
+NuGraphCryoE.avgs_y: [1225.5012 , 183.58075 , 310.83493, 4.3409133]
+NuGraphCryoE.devs_u: [ 82.80644 ,  67.60649 , 274.32666, 1.2912455]
+NuGraphCryoE.devs_v: [ 293.06314,  66.8194  , 322.11386, 1.4249923]
+NuGraphCryoE.devs_y: [ 307.1943 ,  67.063324, 312.461  , 1.4532351]


Where are these numbers from? do they need to be changed if we change geometry?

These are training-specific. With a new geometry, a new training will likely be useful but it would depend on the type of geometry changes.

PetrilloAtWork · 2025-05-07T01:14:28Z

icaruscode/NuGraphIcarus/hdf5maker_icarus_slice.fcl

+  MemoryTracker:         {}
+  RandomNumberGenerator: {}
+  FileCatalogMetadata:   @local::art_file_catalog_mc
+  @table::icarus_geometry_services


The "common" bundle includes the geometry already.

Suggested change

@table::icarus_geometry_services

icaruscode/NuGraphIcarus/hdf5maker_icarus_slice.fcl

francescopoppi · 2025-05-07T18:40:16Z

Hi @cerati I have a request if we can include the following change in the icaruscode cafmaker fcl.
Recently I included CRT Tagged tracks in the carmaker (see: https://github.com/SBNSoftware/sbncode/blame/be46110722ff58957b030001069dda11bf57fb1c/sbncode/CAFMaker/CAFMaker_module.cc#L1954-L1961C48 ) and since I was exploiting a data product that could have also been used by SBND colleagues @PetrilloAtWork asked me to keep the default string for the product (https://github.com/SBNSoftware/sbncode/blob/be46110722ff58957b030001069dda11bf57fb1c/sbncode/CAFMaker/CAFMakerParams.h#L275C1-L286C1 ) namely: CRTHitMatchLabel = "pandoraTrackCRTHit"
can I ask you to include in the icarus cafmaker definitions the following lines:
cafmaker.CRTHitMatchLabel: "CRTT0Tagging"
cafmaker.CRTHitMatchInfoLabel: "CRTT0Tagging"

[https://github.com/SBNSoftware/icaruscode/blob/343bb9ee925a5b15653a8b183356e4e1f2fe8367/fcl/caf/cafmaker_defs.fcl#L321]

Thanks!

EDIT: actually... ignore this request. I realized my first PR is not yet merged, so I will add these changes there. Sorry for the confusion

fcl/g4/larg4_icarus.fcl

PetrilloAtWork

It looks like I have limited myself to 7 bits of comments.

I am less than enthusiastic about how much the ICARUSNuSliceHitsProducer module and his True twin depend on a specific type of PMT matching. There should be no reason for that, if my reading of the code is correct and the module blindly trusts the match from the input. At the point that I would advocate using the direct association between slice and flash for the matching, and the trigger time from raw::Trigger to determine if it's the triggering flash. This is easier said than done, since there is no such direct association, but I think this is important enough that we may want to add that one too (I can help with that).

Other general comment: please add in the FHiCL file a short introduction describing what they do and your name as author, so that we can track you down when flames reach the ceiling. 🪣

Also: this is a breaking change in that it changes the definitions in stage1 workflow; derived workflows that use e.g. icarus_reco_pandoraGausCryoW will not execute some of the modules any more. This needs to be advertised: please write a summary of the breaking changes in the PR description, and make sure the release managers are aware of them so that they can report them in the release notes.

Names and locations

Proposed changes:

The custom for module names is to spell ICARUS all uppercase.
No need to have Icarus in the directory name, NuGraph is enough, and I would argue that it's better under TPC directory.
jobs dealing with a single cryostat should bear the name of that cryostat

icaruscode/NuGraphIcarus/IcarusNuSliceHitsProducer_module.cc

icaruscode/NuGraphIcarus/scripts/setup_tritonserver-nugraph-v0_grid.sh

icaruscode/NuGraphIcarus/testinference_slice_icarus.fcl

icaruscode/NuGraphIcarus/nugraph_icarus.fcl

icaruscode/NuGraphIcarus/CMakeLists.txt

fcl/reco/Definitions/stage1_icarus_defs.fcl

cerati · 2025-06-10T03:43:23Z

@PetrilloAtWork I addressed all your comments (implementing most of your suggestions, although not all). Please let me know if there is any follow up.

…lModuleLabel

PetrilloAtWork · 2025-06-11T22:21:50Z

I have marked as resolved the comments I consider addressed.
I have left unresolved the comments that haven't been implemented without any message from you, and the ones where I wrote a reply (including a few that do not require follow up).

cerati · 2025-06-11T22:48:52Z

@PetrilloAtWork I addressed all 2nd round comments and posted a reply to all those that are not resolved

PetrilloAtWork

A typo is left to be fixed in a description.

All my comments have been discussed and resolution was achieved.

Still not convinced that leaving a change of format pending is a good idea... time will tell.

Approved, with thanks to the author for his patience.

acampani · 2025-06-26T08:59:45Z

icaruscode/TPC/NuGraph/StitchingUtils.h

If my understanding is correct the stitched plane basically exchanges IND2 and COLL in TPC 2 and 3 because of the different wires orientation on the two sides of the cathode, the remaining corrections are explained in detail, but perhaps I'd add also a comment on this for clarity.

Excellent suggestion, will do it. Edit: DONE.

acampani · 2025-06-26T09:23:42Z

icaruscode/TPC/NuGraph/ICARUSHDF5Maker_module.cc

I only have a comment on L451: why is the position included among features if it's left at a default? Is this changed at a later stage? Also I'd use a different default (in principle this is a physical position).

The reason is to keep the format consistent with other experiments that do fill this information. Eventually we may want to do it also for ICARUS.

acampani · 2025-06-26T11:56:46Z

fcl/caf/cafmaker_defs.fcl

The changes to caf files will have a dedicated PR, correct?

Yes. sbncode PR532 was merged in May

cerati added 3 commits March 31, 2025 14:58

first commit of NuGraphIcarus

94abed0

use IcarusNuSliceHitsProducer

d728cdb

add script to start local triton server

6f211ce

cerati requested a review from acampani April 10, 2025 22:20

cerati mentioned this pull request Apr 11, 2025

Feature/cerati ng2caf SBNSoftware/sbncode#532

Merged

cerati added 5 commits April 10, 2025 22:47

fix script name

dc9e706

take a quick nap to avoid starting a lar job before the server is ready

81247b4

fix script names, add version for grid processing

23d0dfd

no sleep better wait

17f1e7b

add NuGraph to reco fcl, switch to libtorch inference by default

d9bed1c

cerati added 4 commits May 2, 2025 14:25

make nugraph part of the pandora sequence

8deda1b

add icarus_postPandora_Gauss

e87380b

remove spurious character

0aa9cdc

Merge tag 'v10_04_08' into cerati/feature_nugraph2

be34e0b

Tag for integration release

PetrilloAtWork mentioned this pull request May 7, 2025

add first NuGraph2 info to CAF SBNSoftware/sbnanaobj#137

Merged

PetrilloAtWork reviewed May 7, 2025

View reviewed changes

icaruscode/NuGraphIcarus/hdf5maker_icarus_slice.fcl Outdated Show resolved Hide resolved

PetrilloAtWork reviewed May 8, 2025

View reviewed changes

fcl/g4/larg4_icarus.fcl Outdated Show resolved Hide resolved

cerati added 4 commits May 8, 2025 12:09

restore cluster3D

cb5276c

Merge branch 'develop' into cerati/feature_nugraph2

4eb87e5

Merge branch 'develop' into cerati/feature_nugraph2

6f1406d

add icarus_postPandora_Gauss to 1d fcl as well

70b8af6

PetrilloAtWork requested changes Jun 4, 2025

View reviewed changes

cerati added 6 commits June 5, 2025 19:17

address G.P.'s comments

6f1b8b7

moved NuGraph folder, improved fcl documentation

f2c7261

remove cafmaker_nugraph_test.fcl

ac05da7

rename Icarus to ICARUS

5297b0c

remove fEventInfo

a6a3c8f

fix assocSliceHitKeys usage

2f14f07

cerati added enhancement New feature or request breaking this change will break backward compatibility in some way labels Jun 10, 2025

restore correct behavior for allIDs, add PMT positions, fix SimChanne…

c3b784b

…lModuleLabel

cerati added 2 commits June 11, 2025 17:34

addressing comments

438a62d

addressing comments

55e0087

improve documentation

d789fe9

PetrilloAtWork self-requested a review June 12, 2025 01:35

PetrilloAtWork approved these changes Jun 12, 2025

View reviewed changes

fix typo in description

9e55de9

acampani reviewed Jun 26, 2025

View reviewed changes

add comment about plane id redefinition for stitching

5b87ad2

Cerati/feature nugraph2 #815

Are you sure you want to change the base?

Cerati/feature nugraph2 #815

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

cerati commented May 2, 2025

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!

francescopoppi commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Names and locations

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cerati commented Jun 10, 2025

Uh oh!

PetrilloAtWork commented Jun 11, 2025

Uh oh!

cerati commented Jun 11, 2025

Uh oh!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

acampani Jun 26, 2025 • 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!

cerati Jun 26, 2025 • 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!

Uh oh!

cerati commented Apr 10, 2025 •

edited

Loading

cerati commented Apr 10, 2025 •

edited

Loading

francescopoppi commented May 7, 2025 •

edited

Loading

acampani Jun 26, 2025 •

edited

Loading

cerati Jun 26, 2025 •

edited

Loading