Skip to content

Version of GausHitFinder with 2 unfolds #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wddgit wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Version of GausHitFinder with 2 unfolds #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions migration/gauss_hit_finder/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
add_library(find_hits_with_gaussians SHARED
find_hits_with_gaussians.cpp
find_hits_with_gaussians_design1.cpp
find_hits_with_gaussians_design2.cpp
copied_from_larsoft_minor_edits/Hit.cxx
copied_from_larsoft_minor_edits/geo_types.cxx
copied_from_larsoft_minor_edits/CandHitStandard.cxx
Expand Down Expand Up @@ -40,6 +41,11 @@ add_library(find_hits_with_gaussians_design1_hof MODULE
target_link_libraries(find_hits_with_gaussians_design1_hof PRIVATE
phlex::module find_hits_with_gaussians)

add_library(find_hits_with_gaussians_design2_hof MODULE
register_find_hits_with_gaussians_design2.cpp)
target_link_libraries(find_hits_with_gaussians_design2_hof PRIVATE
phlex::module find_hits_with_gaussians)

file(GLOB test_files ${CMAKE_CURRENT_SOURCE_DIR}/*.dat)
file(COPY ${test_files} DESTINATION ${CMAKE_CURRENT_BINARY_DIR})

Expand Down Expand Up @@ -142,6 +148,54 @@ else()
endforeach()
endif()

# 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")
Comment on lines +151 to +158

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.txt

Repository: 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.txt

Repository: 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.


# The reference files were generated on a Linux machine
# so we expect identical output on Linux
if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
foreach(n RANGE 4)
add_test(
NAME GausHitFinderCompareDesign2_${n}
COMMAND diff -q -s
${CMAKE_CURRENT_SOURCE_DIR}/art_hits_${n}.txt
${CMAKE_CURRENT_BINARY_DIR}/hits_${n}.txt
)
set_tests_properties(GausHitFinderCompareDesign2_${n} PROPERTIES DEPENDS GausHitFinderTestDesign2)
endforeach()
# On a Mac, we expect the output to be slightly
# different because the reference files were
# generated on a Linux machine.
elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
foreach(n RANGE 4)
add_test(
NAME GausHitFinderCompareDesign2_${n}
COMMAND ${Python3_EXECUTABLE}
${CMAKE_CURRENT_SOURCE_DIR}/compare_hits.py
${CMAKE_CURRENT_SOURCE_DIR}/art_hits_${n}.txt
${CMAKE_CURRENT_BINARY_DIR}/hits_${n}.txt
)
set_tests_properties(GausHitFinderCompareDesign2_${n} PROPERTIES DEPENDS GausHitFinderTestDesign2)
endforeach()
# Always fail on other platforms
# This will warn us to add a new comparison
# method if we ever run on a new platform.
else()
foreach(n RANGE 4)
add_test(
NAME GausHitFinderCompareDesign2_${n}
COMMAND ${CMAKE_COMMAND} -E false
)
set_tests_properties(GausHitFinderCompareDesign2_${n} PROPERTIES DEPENDS GausHitFinderTestDesign2)
endforeach()
endif()

# We need to include the library directory for 'generate_layers' on the PHLEX_PLUGIN_PATH.
# Unfortunately, phlex does not have a target that easily does this (plugins are not intended
# to be accessible downstream), so we have to work around that. Right now, the library directory
Expand All @@ -151,6 +205,7 @@ endif()
set_tests_properties(
GausHitFinderTest
GausHitFinderTestWithUnfold
GausHitFinderTestDesign2
PROPERTIES
ENVIRONMENT_MODIFICATION
"PHLEX_PLUGIN_PATH=path_list_prepend:${CMAKE_CURRENT_BINARY_DIR}:$<TARGET_FILE_DIR:phlex::core>"
Expand Down
23 changes: 18 additions & 5 deletions migration/gauss_hit_finder/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,18 @@ iteration is replaced by that sequence of algorithms.
3. register_find_hits_with_gaussians_design1.cpp
4. test_find_hits_with_gaussians_design1.jsonnet

These files extend design1 by also replacing the inner
parallel_for (over ROIs) with a second unfold-transform-fold.
The resulting layer hierarchy is:
spill -> wire -> roi. A `wire_roi_data` struct bundles
a single ROI (`datarange_t`) with the wire-level context
(channel, view) needed by the transform.

1. find_hits_with_gaussians_design2.hpp
2. find_hits_with_gaussians_design2.cpp
3. register_find_hits_with_gaussians_design2.cpp
4. test_find_hits_with_gaussians_design2.jsonnet

We plan to implement more prototype migrations
of GausHitFinder in the future to explore the
possibilities and execute tests.
Expand Down Expand Up @@ -114,10 +126,11 @@ Getting the single transform to work was a good first step.

The next thing we implemented was replacing the
outer `parallel_for` with an `unfold`, `transform`, and `fold`
sequence of algorithms. In the future, we plan to create other
versions pushing the division into separate algorithms to lower
levels. Then we plan to run tests and compare the different
versions.
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).
Comment on lines +129 to +132

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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

We plan to run tests and compare the different versions.

`Phlex` will not support the `Tools` feature that existed in
`art`. One possibility is that algorithm nodes scheduled
Expand Down Expand Up @@ -223,7 +236,7 @@ Two output data members of `Hit` were ignored because they depend on the `Geomet
geo::WireID fWireID; ///< WireID for the hit (Cryostat, TPC, Plane, Wire)
```

This was done for both `phlex` versions of GausHitFinder. The output of both are identical with each other and with the `art` version.
This was done for all `phlex` versions of GausHitFinder. The output of all versions are identical with each other and with the `art` version.

The `phlex` process was run with multithreading and that causes the order of `Hit` objects to vary from one execution to the next and also the order of events to vary. In the comparison the `Hit` objects were sorted and we had to be careful to compare matching events.

Expand Down
5 changes: 4 additions & 1 deletion migration/gauss_hit_finder/find_hits_with_gaussians_design1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ namespace examples {
{
// Probably eventually delete the following line
// (or convert to logging utility)
std::cout << "Finding hits with Gaussians." << std::endl;
std::cout << "Finding hits with Gaussians (design 1)." << std::endl;
}

unfold_wire_vector_design1::const_iterator unfold_wire_vector_design1::initial_value() const
Expand All @@ -59,6 +59,9 @@ namespace examples {
unfold_wire_vector_design1::unfold(const_iterator current) const
{
recob::Wire const& wire = *current;
// Note this copies the Wire object.
// We discussed this in May and did it intentionally.
// We may revisit this decision later.
return std::make_pair(++current, wire);
};

Expand Down
Loading