Version of GausHitFinder with 2 unfolds#21
Conversation
📝 WalkthroughWalkthroughAdds design2 Gaussian hit finding with separate wire and ROI unfold stages, configurable ROI-level hit construction, PHLEX plugin registration, output comparison tests, and migration documentation. ChangesDesign2 Gaussian hit finder
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WiresSource
participant WireUnfolder
participant ROIUnfolder
participant HitFinder
participant ROIAccumulator
participant SpillAccumulator
WiresSource->>WireUnfolder: wire vector
WireUnfolder->>ROIUnfolder: individual wire
ROIUnfolder->>HitFinder: wire_roi_data
HitFinder->>ROIAccumulator: ROI hits
ROIAccumulator->>SpillAccumulator: per-wire hits
SpillAccumulator-->>WiresSource: final hit vector
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@migration/gauss_hit_finder/CMakeLists.txt`:
- Around line 151-158: Replace the hard-coded DEPENDS chain on
GausHitFinderTestDesign2 with the shared RESOURCE_LOCK used for the hit output
files. Apply the lock property to this test so all designs serialize safely,
including filtered ctest runs, without referencing previous comparison test
names.
In `@migration/gauss_hit_finder/find_hits_with_gaussians_design2.hpp`:
- Line 7: Correct the misspelled README reference in the banner comments: update
migration/gauss_hit_finder/find_hits_with_gaussians_design2.hpp lines 7-7,
migration/gauss_hit_finder/find_hits_with_gaussians_design2.cpp lines 1-1, and
migration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cpp lines
1-1 from REAMDME.md to README.md.
- Around line 33-42: Add a direct include for RawTypes.h in the header’s include
list so wire_roi_data::channel can resolve raw::ChannelID_t without relying on
Wire.h’s transitive includes. Keep the existing includes and declarations
unchanged.
In `@migration/gauss_hit_finder/README.md`:
- Around line 129-132: Update the prose in the README to use the spaced names
“design 1” and “design 2” instead of “design1” and “design2”; keep unspaced
forms only where they refer to filenames or identifiers.
In `@migration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cpp`:
- Around line 3-7: Add direct standard-library includes for the symbols used in
this translation unit: include <iostream> for std::cerr and std::endl, and
<stdexcept> for std::runtime_error. Update the include block in
register_find_hits_with_gaussians_design2.cpp without relying on transitive
includes.
In `@migration/gauss_hit_finder/test_find_hits_with_gaussians_design2.jsonnet`:
- Around line 24-30: Update the provenance comment in
test_find_hits_with_gaussians_design2.jsonnet to remove personal names, Slack
references, email references, and dates; retain only a generic note that the
configuration parameters were copied from the expanded FCL configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85c9df68-22c2-40c1-8a9a-09592f4c9edb
📒 Files selected for processing (7)
migration/gauss_hit_finder/CMakeLists.txtmigration/gauss_hit_finder/README.mdmigration/gauss_hit_finder/find_hits_with_gaussians_design1.cppmigration/gauss_hit_finder/find_hits_with_gaussians_design2.cppmigration/gauss_hit_finder/find_hits_with_gaussians_design2.hppmigration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cppmigration/gauss_hit_finder/test_find_hits_with_gaussians_design2.jsonnet
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-07-08T20:31:37.310Z
Learnt from: wddgit
Repo: Framework-R-D/phlex-examples PR: 18
File: migration/gauss_hit_finder/register_find_hits_with_gaussians_design1.cpp:49-63
Timestamp: 2026-07-08T20:31:37.310Z
Learning: In Framework-R-D/phlex-examples, files under `migration/` that are LArSoft-derived copies should be kept as close as possible to the original LArSoft implementation. During code review, avoid suggesting behavioral changes or extra “hardening” (e.g., new validation, altered logic, different edge-case handling) beyond what is strictly needed to support the phlex migration/demo or to make the code compile/integrate. If a deviation from upstream is required, keep it minimal and document the reason.
Applied to files:
migration/gauss_hit_finder/find_hits_with_gaussians_design1.cppmigration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cppmigration/gauss_hit_finder/find_hits_with_gaussians_design2.cpp
🪛 Clang (14.0.6)
migration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cpp
[warning] 22-22: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 22-22: all parameters should be named in a function
(readability-named-parameter)
[warning] 64-64: variable 'cand_hit_standard_vec' is not initialized
(cppcoreguidelines-init-variables)
migration/gauss_hit_finder/find_hits_with_gaussians_design2.cpp
[warning] 22-22: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 22-22: parameter name 'x' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 24-24: statement should be inside braces
(readability-braces-around-statements)
[warning] 28-28: statement should be inside braces
(readability-braces-around-statements)
[warning] 31-31: statement should be inside braces
(readability-braces-around-statements)
[warning] 52-52: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 57-57: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 60-60: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 60-60: method 'unfold' can be made static
(readability-convert-member-functions-to-static)
[warning] 77-77: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 77-77: method 'initial_value' can be made static
(readability-convert-member-functions-to-static)
[warning] 82-82: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 88-88: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 99-99: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 99-99: function 'find_hits_with_gaussians_design2' has cognitive complexity of 102 (threshold 25)
(readability-function-cognitive-complexity)
[note] 114-114: nesting level increased to 1
(clang)
[note] 124-124: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 125-125: nesting level increased to 2
(clang)
[note] 132-132: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 178-178: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 185-185: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 208-208: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 213-213: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 225-225: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 225-225: +1
(clang)
[note] 229-229: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 234-234: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 243-243: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 245-245: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 287-287: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 296-296: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 304-304: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 309-309: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 316-316: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 344-344: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 345-345: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 346-346: +5, including nesting penalty of 4, nesting level increased to 5
(clang)
[note] 354-354: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 355-355: +5, including nesting penalty of 4, nesting level increased to 5
(clang)
[note] 365-365: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 369-369: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 372-372: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 375-375: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 409-409: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 419-419: +2, including nesting penalty of 1, nesting level increased to 2
(clang)
[note] 419-419: +1
(clang)
[note] 432-432: nesting level increased to 3
(clang)
[note] 437-437: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 437-437: +1
(clang)
[note] 442-442: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 452-452: nesting level increased to 4
(clang)
[note] 453-453: +1
(clang)
[note] 457-457: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 463-463: nesting level increased to 4
(clang)
[note] 469-469: +3, including nesting penalty of 2, nesting level increased to 3
(clang)
[note] 470-470: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 470-470: +1
(clang)
[note] 477-477: +1, including nesting penalty of 0, nesting level increased to 1
(clang)
[note] 479-479: +1, nesting level increased to 1
(clang)
[error] 113-113: no viable conversion from '(lambda at /home/jailuser/git/migration/gauss_hit_finder/find_hits_with_gaussians_design2.cpp:114:7)' to 'std::function<double (double, double, double, double, int, int)>'
(clang-diagnostic-error)
[note] 114-114: candidate function
(clang)
[warning] 115-115: 3 adjacent parameters of 'operator()' of similar type ('double') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 115-115: the first parameter in the range is 'peakAmp'
(clang)
[note] 117-117: the last parameter in the range is 'areaNorm'
(clang)
[error] 119-119: no member named 'numbers' in namespace 'std'
(clang-diagnostic-error)
[warning] 124-124: statement should be inside braces
(readability-braces-around-statements)
[warning] 129-129: 2 adjacent parameters of 'operator()' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 129-129: the first parameter in the range is 'low'
(clang)
[note] 130-130: the last parameter in the range is 'hi'
(clang)
[warning] 130-130: parameter name 'hi' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 132-132: statement should be inside braces
(readability-braces-around-statements)
[warning] 161-161: narrowing conversion from 'lar::range_t::size_type' (aka 'unsigned long') to signed type 'raw::TDCtick_t' (aka 'int') is implementation-defined
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 179-179: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 180-180: narrowing conversion from 'size_t' (aka 'unsigned long') to signed type 'int' is implementation-defined
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 185-185: statement should be inside braces
(readability-braces-around-statements)
[warning] 193-193: narrowing conversion from 'std::vector::size_type' (aka 'unsigned long') to signed type 'int' is implementation-defined
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 234-234: statement should be inside braces
(readability-braces-around-statements)
[warning] 254-254: uninitialized record type: 'peakParams'
(cppcoreguidelines-pro-type-member-init)
[warning] 256-256: narrowing conversion from 'double' to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 257-257: narrowing conversion from 'double' to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 258-258: narrowing conversion from 'double' to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 259-259: narrowing conversion from 'double' to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 260-260: narrowing conversion from 'double' to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 261-261: narrowing conversion from 'double' to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 329-329: narrowing conversion from 'double' to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 331-331: variable 'chargeErr' is not initialized
(cppcoreguidelines-init-variables)
[error] 332-332: no member named 'numbers' in namespace 'std'
(clang-diagnostic-error)
[warning] 335-335: use auto when declaring iterators
(modernize-use-auto)
[warning] 336-336: use auto when declaring iterators
(modernize-use-auto)
[warning] 339-339: use auto when declaring iterators
(modernize-use-auto)
[warning] 340-340: narrowing conversion from 'float' to '__gnu_cxx::__normal_iterator<const float *, std::vector>::difference_type' (aka 'long')
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 340-340: narrowing conversion from 'float' to '__gnu_cxx::__normal_iterator<const float *, std::vector>::difference_type' (aka 'long')
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 341-341: use auto when declaring iterators
(modernize-use-auto)
[warning] 342-342: narrowing conversion from 'float' to '__gnu_cxx::__normal_iterator<const float *, std::vector>::difference_type' (aka 'long')
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 342-342: narrowing conversion from 'float' to '__gnu_cxx::__normal_iterator<const float *, std::vector>::difference_type' (aka 'long')
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 349-349: narrowing conversion from 'float' to '__gnu_cxx::__normal_iterator<const float *, std::vector>::difference_type' (aka 'long')
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 349-349: narrowing conversion from 'float' to '__gnu_cxx::__normal_iterator<const float *, std::vector>::difference_type' (aka 'long')
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 358-358: narrowing conversion from 'float' to '__gnu_cxx::__normal_iterator<const float *, std::vector>::difference_type' (aka 'long')
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 358-358: narrowing conversion from 'float' to '__gnu_cxx::__normal_iterator<const float *, std::vector>::difference_type' (aka 'long')
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 365-365: statement should be inside braces
(readability-braces-around-statements)
[warning] 369-369: statement should be inside braces
(readability-braces-around-statements)
[warning] 372-372: statement should be inside braces
(readability-braces-around-statements)
[warning] 375-375: statement should be inside braces
(readability-braces-around-statements)
[warning] 386-386: narrowing conversion from 'raw::TDCtick_t' (aka 'int') to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 391-391: narrowing conversion from 'double' to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 392-392: narrowing conversion from 'double' to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 395-395: narrowing conversion from 'int' to signed type 'short' is implementation-defined
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 396-396: narrowing conversion from 'int' to signed type 'short' is implementation-defined
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 397-397: narrowing conversion from 'double' to 'float'
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 409-409: statement should be inside braces
(readability-braces-around-statements)
[warning] 438-438: statement should be inside braces
(readability-braces-around-statements)
[warning] 449-449: use auto when declaring iterators
(modernize-use-auto)
[warning] 457-457: statement should be inside braces
(readability-braces-around-statements)
🪛 Cppcheck (2.21.0)
migration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cpp
[style] 35-35: The function 'isValidChannelID' is never used.
(unusedFunction)
[style] 124-124: The function 'to_int' is never used.
(unusedFunction)
migration/gauss_hit_finder/find_hits_with_gaussians_design2.cpp
[style] 218-218: The function 'StartTick' is never used.
(unusedFunction)
[style] 222-222: The function 'EndTick' is never used.
(unusedFunction)
[style] 230-230: The function 'SigmaPeakTime' is never used.
(unusedFunction)
[style] 242-242: The function 'SigmaPeakAmplitude' is never used.
(unusedFunction)
[style] 246-246: The function 'ROISummedADC' is never used.
(unusedFunction)
[style] 250-250: The function 'HitSummedADC' is never used.
(unusedFunction)
[style] 254-254: The function 'Integral' is never used.
(unusedFunction)
[style] 258-258: The function 'SigmaIntegral' is never used.
(unusedFunction)
[style] 262-262: The function 'Multiplicity' is never used.
(unusedFunction)
[style] 266-266: The function 'LocalIndex' is never used.
(unusedFunction)
[style] 270-270: The function 'GoodnessOfFit' is never used.
(unusedFunction)
[style] 274-274: The function 'DegreesOfFreedom' is never used.
(unusedFunction)
[style] 282-282: The function 'SignalType' is never used.
(unusedFunction)
[style] 300-300: The function 'PeakTimeMinusRMS' is never used.
(unusedFunction)
[style] 305-305: The function 'TimeDistanceAsRMS' is never used.
(unusedFunction)
[style] 35-35: The function 'isValidChannelID' is never used.
(unusedFunction)
[style] 215-215: The function 'NSignal' is never used.
(unusedFunction)
[style] 124-124: The function 'to_int' is never used.
(unusedFunction)
[style] 217-217: The function 'markValid' is never used.
(unusedFunction)
[style] 220-220: The function 'markInvalid' is never used.
(unusedFunction)
[style] 244-244: The function 'getInvalidID' is never used.
(unusedFunction)
[style] 294-294: The function 'asCryostatID' is never used.
(unusedFunction)
[style] 326-326: The function 'next' is never used.
(unusedFunction)
[style] 409-409: The function 'asTPCID' is never used.
(unusedFunction)
[style] 466-466: The function 'asPlaneID' is never used.
(unusedFunction)
[style] 471-471: The function 'planeID' is never used.
(unusedFunction)
[style] 220-220: The function 'set' is never used.
(unusedFunction)
[style] 52-52: The function 'initial_value' is never used.
(unusedFunction)
[style] 57-57: The function 'predicate' is never used.
(unusedFunction)
[style] 60-60: The function 'unfold' is never used.
(unusedFunction)
[style] 99-99: The function 'find_hits_with_gaussians_design2' is never used.
(unusedFunction)
[style] 488-488: The function 'fold_roi_hits_design2' is never used.
(unusedFunction)
[style] 498-498: The function 'fold_hits_into_vector_design2' is never used.
(unusedFunction)
🪛 LanguageTool
migration/gauss_hit_finder/README.md
[grammar] ~129-~129: Ensure spelling is correct
Context: ...rm, and fold` sequence of algorithms (design1). After that, we created design2 which ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (11)
migration/gauss_hit_finder/test_find_hits_with_gaussians_design2.jsonnet (2)
1-23: LGTM!
31-73: LGTM!migration/gauss_hit_finder/README.md (2)
65-76: LGTM!
239-239: LGTM!migration/gauss_hit_finder/find_hits_with_gaussians_design1.cpp (1)
48-48: LGTM!Also applies to: 62-65
migration/gauss_hit_finder/find_hits_with_gaussians_design2.hpp (2)
50-54: LGTM!Also applies to: 56-73, 94-135
79-92: 🩺 Stability & AvailabilityCheck phlex’s inner unfold lifetime before relying on
wire_.
unfold_wire_design2borrows the Wire from the upstreamm.unfold<...>pipeline via a reference member and reads it later inunfold(). If phlex does not guarantee the intermediate Wire lives as long as this product object, this reads a dangling reference; storing the Wire by value here avoids that handoff risk.migration/gauss_hit_finder/find_hits_with_gaussians_design2.cpp (2)
19-35: LGTM!Also applies to: 43-93, 137-269, 271-502
113-119: 📐 Maintainability & Code QualityNo target standard issue here.
CMAKE_CXX_STANDARD 23is set, and both design files already use<numbers>/std::numbers::pi, so theclang-diagnostic-erroris an analysis-environment-stdfallback rather than a patch defect.migration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cpp (1)
20-62: LGTM!Also applies to: 100-129
migration/gauss_hit_finder/CMakeLists.txt (1)
6-6: LGTM!Also applies to: 44-48, 160-197, 208-208
| # Repeat for design2 version with unfold-transform-fold. | ||
| add_test(NAME GausHitFinderTestDesign2 | ||
| COMMAND phlex -c ${CMAKE_CURRENT_SOURCE_DIR}/test_find_hits_with_gaussians_design2.jsonnet | ||
| WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}) | ||
|
|
||
| # The tests must not be run concurrently because they read and write to the same file. | ||
| set_tests_properties(GausHitFinderTestDesign2 PROPERTIES DEPENDS | ||
| "GausHitFinderCompareWithUnfold_0;GausHitFinderCompareWithUnfold_1;GausHitFinderCompareWithUnfold_2;GausHitFinderCompareWithUnfold_3;GausHitFinderCompareWithUnfold_4") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the DEPENDS test names actually exist, and see the full ordering chain.
rg -n 'add_test|NAME Gaus|PROPERTIES DEPENDS|RESOURCE_LOCK|Python3' migration/gauss_hit_finder/CMakeLists.txtRepository: Framework-R-D/phlex-examples
Length of output: 2121
🏁 Script executed:
#!/bin/bash
# Inspect the shared output path and full ordering chain around the relevant tests.
sed -n '55,200p' migration/gauss_hit_finder/CMakeLists.txtRepository: Framework-R-D/phlex-examples
Length of output: 6145
Use a resource lock for shared hit outputs.
The DEPENDS chain works, but every new design still has to hard-code the previous design’s comparison test names. Those tests all read/write the same ${CMAKE_CURRENT_BINARY_DIR}/hits_${n}.txt files under the lock named in the diff, so a shared RESOURCE_LOCK expresses the real constraint and keeps filtered ctest runs safe without adding test-name dependency chains.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migration/gauss_hit_finder/CMakeLists.txt` around lines 151 - 158, Replace
the hard-coded DEPENDS chain on GausHitFinderTestDesign2 with the shared
RESOURCE_LOCK used for the hit output files. Apply the lock property to this
test so all designs serialize safely, including filtered ctest runs, without
referencing previous comparison test names.
| // Design2 extends design1 by also replacing the inner parallel_for | ||
| // (over ROIs) with a second unfold-transform-fold. | ||
|
|
||
| // See REAMDME.md for some general comments about this example. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
REAMDME.md typo copy-pasted into all three new design2 files. The banner comment points readers at a file that doesn't exist; the letters are transposed in every copy. Root cause is one copy-paste, so fix it in all three (and maybe grep the design1 files while you're in there — the typo looks inherited).
migration/gauss_hit_finder/find_hits_with_gaussians_design2.hpp#L7-L7: changeREAMDME.mdtoREADME.md.migration/gauss_hit_finder/find_hits_with_gaussians_design2.cpp#L1-L1: changeREAMDME.mdtoREADME.md.migration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cpp#L1-L1: changeREAMDME.mdtoREADME.md.
📍 Affects 3 files
migration/gauss_hit_finder/find_hits_with_gaussians_design2.hpp#L7-L7(this comment)migration/gauss_hit_finder/find_hits_with_gaussians_design2.cpp#L1-L1migration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cpp#L1-L1
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migration/gauss_hit_finder/find_hits_with_gaussians_design2.hpp` at line 7,
Correct the misspelled README reference in the banner comments: update
migration/gauss_hit_finder/find_hits_with_gaussians_design2.hpp lines 7-7,
migration/gauss_hit_finder/find_hits_with_gaussians_design2.cpp lines 1-1, and
migration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cpp lines
1-1 from REAMDME.md to README.md.
| #include <cstddef> | ||
| #include <memory> | ||
| #include <utility> | ||
| #include <vector> | ||
|
|
||
| #include "copied_from_larsoft_minor_edits/CandHitStandard.h" | ||
| #include "copied_from_larsoft_minor_edits/Hit.h" | ||
| #include "copied_from_larsoft_minor_edits/HitFilterAlg.h" | ||
| #include "copied_from_larsoft_minor_edits/PeakFitterMrqdt.h" | ||
| #include "copied_from_larsoft_minor_edits/Wire.h" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Include RawTypes.h directly for raw::ChannelID_t.
wire_roi_data::channel (Line 52) uses raw::ChannelID_t, which this header only gets transitively (via Wire.h). The .cpp already includes RawTypes.h explicitly — do the same here so the header stands on its own.
♻️ Proposed include fix
`#include` "copied_from_larsoft_minor_edits/PeakFitterMrqdt.h"
+#include "copied_from_larsoft_minor_edits/RawTypes.h" // raw::ChannelID_t
`#include` "copied_from_larsoft_minor_edits/Wire.h"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <cstddef> | |
| #include <memory> | |
| #include <utility> | |
| #include <vector> | |
| #include "copied_from_larsoft_minor_edits/CandHitStandard.h" | |
| #include "copied_from_larsoft_minor_edits/Hit.h" | |
| #include "copied_from_larsoft_minor_edits/HitFilterAlg.h" | |
| #include "copied_from_larsoft_minor_edits/PeakFitterMrqdt.h" | |
| #include "copied_from_larsoft_minor_edits/Wire.h" | |
| `#include` <cstddef> | |
| `#include` <memory> | |
| `#include` <utility> | |
| `#include` <vector> | |
| `#include` "copied_from_larsoft_minor_edits/CandHitStandard.h" | |
| `#include` "copied_from_larsoft_minor_edits/Hit.h" | |
| `#include` "copied_from_larsoft_minor_edits/HitFilterAlg.h" | |
| `#include` "copied_from_larsoft_minor_edits/PeakFitterMrqdt.h" | |
| `#include` "copied_from_larsoft_minor_edits/RawTypes.h" // raw::ChannelID_t | |
| `#include` "copied_from_larsoft_minor_edits/Wire.h" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migration/gauss_hit_finder/find_hits_with_gaussians_design2.hpp` around lines
33 - 42, Add a direct include for RawTypes.h in the header’s include list so
wire_roi_data::channel can resolve raw::ChannelID_t without relying on Wire.h’s
transitive includes. Keep the existing includes and declarations unchanged.
| sequence of algorithms (design1). After that, we created | ||
| design2 which also replaces the inner `parallel_for` (over ROIs) | ||
| with a second `unfold`, `transform`, and `fold` sequence, | ||
| resulting in a three-layer hierarchy (spill -> wire -> roi). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use spaced design names in prose.
Write “design 1” and “design 2” here; the unspaced forms should remain reserved for filenames and identifiers.
Suggested wording
- sequence of algorithms (design1). After that, we created
- design2 which also replaces the inner `parallel_for` (over ROIs)
+ sequence of algorithms (design 1). After that, we created
+ design 2 which also replaces the inner `parallel_for` (over ROIs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sequence of algorithms (design1). After that, we created | |
| design2 which also replaces the inner `parallel_for` (over ROIs) | |
| with a second `unfold`, `transform`, and `fold` sequence, | |
| resulting in a three-layer hierarchy (spill -> wire -> roi). | |
| sequence of algorithms (design 1). After that, we created | |
| design 2 which also replaces the inner `parallel_for` (over ROIs) | |
| with a second `unfold`, `transform`, and `fold` sequence, | |
| resulting in a three-layer hierarchy (spill -> wire -> roi). |
🧰 Tools
🪛 LanguageTool
[grammar] ~129-~129: Ensure spelling is correct
Context: ...rm, and fold` sequence of algorithms (design1). After that, we created design2 which ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migration/gauss_hit_finder/README.md` around lines 129 - 132, Update the
prose in the README to use the spaced names “design 1” and “design 2” instead of
“design1” and “design2”; keep unspaced forms only where they refer to filenames
or identifiers.
Source: Linters/SAST tools
| #include <exception> | ||
| #include <memory> | ||
| #include <string> | ||
| #include <utility> | ||
| #include <vector> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing <iostream> and <stdexcept> — this TU is riding on transitive includes.
Line 76 uses std::cerr/std::endl and Line 77 throws std::runtime_error. <exception> gives you std::exception, not std::runtime_error (that's <stdexcept>), and nothing here pulls in <iostream>. It builds today purely because some phlex or larsoft header leaks them — one upstream include cleanup and this breaks in a way that looks totally unrelated to your change.
🔧 Proposed include fix
`#include` <exception>
+#include <iostream>
`#include` <memory>
+#include <stdexcept>
`#include` <string>
`#include` <utility>
`#include` <vector>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include <exception> | |
| #include <memory> | |
| #include <string> | |
| #include <utility> | |
| #include <vector> | |
| `#include` <exception> | |
| `#include` <iostream> | |
| `#include` <memory> | |
| `#include` <stdexcept> | |
| `#include` <string> | |
| `#include` <utility> | |
| `#include` <vector> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migration/gauss_hit_finder/register_find_hits_with_gaussians_design2.cpp`
around lines 3 - 7, Add direct standard-library includes for the symbols used in
this translation unit: include <iostream> for std::cerr and std::endl, and
<stdexcept> for std::runtime_error. Update the include block in
register_find_hits_with_gaussians_design2.cpp without relying on transitive
includes.
| // Copied in the configuration parameters that | ||
| // I obtained from "Far Detector Simulation/Reconstruction conveners": | ||
| // Dominic Brailsford and Laura Paulucci (in cc). See Dom and Kyle | ||
| // 2/17/2026 on SLACK and also email around the same time. These were | ||
| // copied out of the expanded fcl configuration. Note that the ones | ||
| // that have different values for different planes are not used when | ||
| // filter_hits is false. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Remove internal communication details from the checked-in configuration.
The names and Slack/email references are not required to run the test and unnecessarily expose internal provenance. Keep a generic configuration-source comment instead.
Suggested cleanup
- // Copied in the configuration parameters that
- // I obtained from "Far Detector Simulation/Reconstruction conveners":
- // Dominic Brailsford and Laura Paulucci (in cc). See Dom and Kyle
- // 2/17/2026 on SLACK and also email around the same time. These were
- // copied out of the expanded fcl configuration. Note that the ones
- // that have different values for different planes are not used when
- // filter_hits is false.
+ // Values copied from the expanded detector configuration.
+ // Plane-specific values are unused when filter_hits is false.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Copied in the configuration parameters that | |
| // I obtained from "Far Detector Simulation/Reconstruction conveners": | |
| // Dominic Brailsford and Laura Paulucci (in cc). See Dom and Kyle | |
| // 2/17/2026 on SLACK and also email around the same time. These were | |
| // copied out of the expanded fcl configuration. Note that the ones | |
| // that have different values for different planes are not used when | |
| // filter_hits is false. | |
| // Values copied from the expanded detector configuration. | |
| // Plane-specific values are unused when filter_hits is false. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@migration/gauss_hit_finder/test_find_hits_with_gaussians_design2.jsonnet`
around lines 24 - 30, Update the provenance comment in
test_find_hits_with_gaussians_design2.jsonnet to remove personal names, Slack
references, email references, and dates; retain only a generic note that the
configuration parameters were copied from the expanded FCL configuration.
1 participant
Version of GausHitFinder with 2 unfolds. An earlier PR added a version of GausHitFinder where the outer parallel_for call had been replaced by an unfold then a transform then a fold. This PR extends that by replacing the inner parallel_for call with the same unfold-transform-fold sequence.
Tests are added that compare the results of this version to the art version. The tests are similar to those for the earlier two phlex versions of this module. The new tests pass.
parallel_forloops.artresults across five events and supported platforms.