Migrate Work from release/SBN2025A to develop#893
Conversation
…s well the modules converting from Wire to/from ChannelROI. Removing from icaruscode
This reverts commit 4e90203.
add stage0 plugins
tools_maker + cpu->gpu
…ed-dnnroi-gpu.jsonnet
jas1005
requested review from
SFBayLaser,
Copilot and
gputnam
and removed request for
Copilot
|
@gputnam, @SFBayLaser, @mvicenzi, and @francescopoppi I added y'all as reviewers for this PR since you all made commits that required some sort of manual intervention to resolve conflicts. If you're unable to review this PR, please let me know! |
mvicenzi
left a comment
mvicenzi
left a comment
There was a problem hiding this comment.
Thanks for getting this done!
I've reviewed the merge conflicts of the code I was involved with: see below for the correct PMT database tag that needs to be used. The new PMT tag requires icarus_data >10_06_01, but I believe that is being correctly applied.
|
Approved, |
jas1005
dismissed
mvicenzi’s stale review
Requested change was made in commit 194e2b7.
mvicenzi
left a comment
mvicenzi
left a comment
There was a problem hiding this comment.
My concern was resolved. Formally approving now.
…on for ICARUS Runs 2+4 MC.
…icarus_data version.
SFBayLaser
left a comment
SFBayLaser
left a comment
There was a problem hiding this comment.
I noted the comments on the TPC decoder and I might have missed in in the large number of updated files but I did not see changes related to that in this PR. My eye caught the "PCA" in the comment and I was hoping to see exactly what that was referring to so I could simply exclude it from concern... also the other parameter should be false at this time.
|
PS I did see that Claude made comments including at least one where it is catching an error that should be corrected before merging. |
|
I don't personally know what PCA relates to. But you specifically won't see the changes you mentioned since they are already on 'develop' via commit 0c58175. For example, RemoveBadRMS is already set to false on 'develop' via this commit. So it is excluded from this PR. What errors are you referrring to? I cannot find them. I tried to look into the Copilot review sessions for such errors, but I don't see any. I would also take Copilot reviews with a grain of salt. I know that Copilot has historically suggested the opposite of changes we want to make for other PRs. I personally don't rely on Copilot beyond syntax corrections. |
|
I found this comment embedded in the list of changed files: |
This PR migrates work from release/SBN2025A to develop as part of the effort to return to develop being the source of truth for ICARUSCode. After this migration, continued development of ICARUSCode should/will be done against develop. Details about the migration process, including choices made as part of resolving merge conflicts, are given below.
Effort was made to prioritize development that took place on release/SBN2025A where applicable and still relevant. The only notable exception to this was keeping the commit (0c58175, Tracy Usher) from develop that, according to this commit's message, "Made updates aimed at protecting against bad channels."
Several 'settings-level' choices were additionally made in various files:
icaruscode/Decode/DecoderTools/decoderTools_icarus.fcl: The TPCNoiseFilter1DTool entries were kept as "Threshold: [20.0, 12., 12.] # --> for PCA: [5.0, 3.5, 3.5]" and "RemoveBadRMS: false". The Threshold can be changed back to use the second set of numbers if needed. RemoveBadRMS can also be set to 'true' if needed.
test/ci/icarus_ci_<intimecosmic, nucosmics, single>_g4_quick_test_icaruscode.fcl: The included fcl files used by Production were prioritized over the standard (i.e. non-LArG4) fcl files. Specifically, the intimecosmic fcl file includes larg4_icarus_intime_sci.fcl and both the nucosmics and single fcl files include the larg4_icarus.fcl file. The standard fcl files can be swapped back in if needed.
fcl/reco/Stage0/data/stage0_daqPMT_drop.fcl: The outputs > out1 > fileName entry was kept as "%ifb_%tc_%p.root". The alternative was to not keep "%tc". This choice was made since "%tc" carries additional information.
fcl/configurations/calibration_database_GlobalTags_icarus.fcl: Several commits made changes to this file's ICARUS_Calibration_GlobalTags entry. In one commit on release/SBN2025A, @table::<TPC, PMT>_CalibrationTags had the extensions _Jan2025 and Oct2025, respectively. These lines' extensions were later changed back to _Oct2025 and _Run3_Feb2025, respectively. We have kept the _Oct2025 and _Run3_Feb2025 extensions in the final versions of these files.
It was also noticed in this migration process that release/SBN2025A received an icarus_data dependency upgrade to v10_06_03. At the time of this PR, develop used icarus_data v09_93_06. This change will be incorporated into future releases of ICARUSCode.