Cerati/feature nugraph2 #815
Conversation
|
This PR needs also needs |
| export METRPORT=$((BASEPORT+2)) | ||
| done | ||
| # | ||
| echo "physics.producers.NuGraphCryoE.TritonConfig.serverURL: \"localhost:"$GRPCPORT"\"" >> $FCL |
There was a problem hiding this comment.
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.
| TimeTracker: {} | ||
| MemoryTracker: {} | ||
| RandomNumberGenerator: {} | ||
| @table::icarus_geometry_services |
There was a problem hiding this comment.
Geometry services are included in the "common" bundle.
| @table::icarus_geometry_services |
| reco: [nuslhitsCryoE, nuslhitsCryoW, NuGraphCryoE, NuGraphCryoW] | ||
| #reco: [ nuslhitsCryoE, NuGraphCryoE] | ||
| trigger_paths: [ reco ] | ||
|
|
||
| ana: [NuGraphAnaCryoE,NuGraphAnaCryoW] | ||
| #ana: [NuGraphAnaCryoE] | ||
| streamROOT: [ rootOutput ] | ||
| end_paths: [ ana, streamROOT ] |
There was a problem hiding this comment.
Please fix the indentation.
| 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] |
There was a problem hiding this comment.
Where are these numbers from? do they need to be changed if we change geometry?
There was a problem hiding this comment.
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.
| MemoryTracker: {} | ||
| RandomNumberGenerator: {} | ||
| FileCatalogMetadata: @local::art_file_catalog_mc | ||
| @table::icarus_geometry_services |
There was a problem hiding this comment.
The "common" bundle includes the geometry already.
| @table::icarus_geometry_services |
|
Hi @cerati I have a request if we can include the following change in the icaruscode cafmaker fcl. [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 |
There was a problem hiding this comment.
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
ICARUSall uppercase. - No need to have
Icarusin the directory name,NuGraphis enough, and I would argue that it's better underTPCdirectory. - jobs dealing with a single cryostat should bear the name of that cryostat
icaruscode/NuGraphIcarus/scripts/setup_tritonserver-nugraph-v0_grid.sh
Outdated
Show resolved
Hide resolved
cerati
added
enhancement
|
@PetrilloAtWork I addressed all your comments (implementing most of your suggestions, although not all). Please let me know if there is any follow up. |
|
I have marked as resolved the comments I consider addressed. |
|
@PetrilloAtWork I addressed all 2nd round comments and posted a reply to all those that are not resolved |
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/Wintoicarus_reco_pandoraGausCryoE/W+icarus_reco_postPandoraGausCryoE/W. NuGraph is run in the latter, and the split is required since NuGraph needs bothpandoraandicarus_tpcpmtbarycentermatchto be run upfront. The standard reco fcl files are updated accordingly, but non centrally-maintained fcls may have issues.