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
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: |
…icarus_data version.
|
I have resolved the incorrect file name issue. @SFBayLaser, was this your only concern in giving your approval for this PR? I see there are some other Copilot comments. But I'm unfamiliar with those snippets of code and how to fix them. I'm looking into who recently touched the relevant files and pinging them here for guidance on how to fix these Copilot issues. |
|
@gputnam, Copilot has made some comments related to files you've most recently touched. I'm unfamiliar with these snippets of code. Do you know how to resolve these issues? If not, do you know who I could contact to get some guidance on resolving these Copilot issues? You can navigate to the files changed section of this page to see the details of the Copilot comments. The two offending files are linked below: icaruscode/TPC/Calorimetry/NormalizeTPCPerPlane_tool.cc icaruscode/TPC/SignalProcessing/HitFinder/HitMerger_module.cc |
Bug fixe caught by copilot
francescopoppi
left a comment
francescopoppi
left a comment
There was a problem hiding this comment.
Hi all, I found one bug in the CRT Data Analysis module, I pushed a commit with the fix. While scrolling in the changes I also found one possible error here the tool NormalizeTPCPerPlane might not be properly defined. I also think that the updated gains should have a reference docdb file for future reference.
|
|
||
| tpcgain: { | ||
| tool_type: NormalizeTPC | ||
| tool_type: NormalizeTPCPerPlane |
There was a problem hiding this comment.
Is it intended?
I believe so, but I also believe https://github.com/SBNSoftware/icaruscode/pull/893/changes#diff-c62b2ed7f7a3a67b12804ece957666e51726e048c9080ae78937961e2220778c
should be modified in NormalizeTPCPerPlane
There was a problem hiding this comment.
There are other NormalizeTPC*_tool.cc files in this directory that clarify the intended syntax for such files. I'm pushing a commit that fixes the modifications in NormalizeTPCPerPlane_tool.cc as Francesco mentions. Regardless of which tool we wind up using, instances of "NormalizeTPC" in the NormalizeTPCPerPlane_tool.cc file are bad since a NormalizeTPC_tool.cc file aready exists.
I cannot weigh in from a physics perspective on whether we should use NormalizeTPCPerPlane over NormalizeTPC in this context. But other lines in this normtools_icarus.fcl file all use the PerPlane variants of NormalizeTPC when applicable.
There was a problem hiding this comment.
@gputnam, I have made the same NormalizeTPC --> NormalizeTPCPerPlane changes in the NormalizeTPCPerPlane_tool.cc file for release/SBN2025A. Should I also pull in the commit Francesco made (98e88a3) to CRTDataAnalysis_module.cc? Or is this change not relevant for production? If it's relevant, I'll cut a new patch for release/SBN2025A.
| icarus_data_calconst: [0.0133333, 0.0133333, 0.0133333] | ||
| # GP 8/4/25 -- Update gain to re-measurement with Run 2 stopping muons and | ||
| # fixed recombination. | ||
| icarus_data_calconst: [0.016751, 0.012755, 0.012513] |
There was a problem hiding this comment.
I really feel like this change should have a docdb link with a presentation/note.
There was a problem hiding this comment.
This change was made by @gputnam. Can you point to any presentation links that address this change, Gray?
…ormalizeTPCPerPlane_tool.cc file.
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.