Migrate Work from release/SBN2025A to develop#893
Conversation
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.
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: |
|
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 |
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.
|
The HitMerger_module.cc issues do not need fixing. After talking with Gray, they confirmed that a hard crash is fine if there are no inputs. The NormalizeTPCPerPlane_tool.cc issues raised by Copilot do need fixing. Gray has informed me they will push a bugfix for this soon. |
SFBayLaser
left a comment
SFBayLaser
left a comment
There was a problem hiding this comment.
Pending the last update from Gray I am happy to approve. Thanks Jacob for all the hard work on this!
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
…r were left uninitialized.
…d added TriggerTimestamp variable to DAQTree.
…e CRTT0Tagging for 2D was using Cluster3D instead of combineCluster3D. This was due to a typo HitLabel vs HitLabels. b) Now for 1d also the pandora hit producers are based on Cluster3D.
…configuration for Run4 detsim.
…s overriden for Run 4.
…on for ICARUS Runs 2+4 MC.
Bug fixe caught by copilot
…ormalizeTPCPerPlane_tool.cc file.
jas1005
force-pushed
the
migrate/SBN2025A
branch
from
209f09e to
dbb8aa0
Compare
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.