Add StreamId+callback Geant4 interfaces#2332
Draft
sethrj wants to merge 39 commits into
Draft
Conversation
sethrj
added
documentation
pcanal
reviewed
Mar 25, 2026
Test summary 5 669 files 9 191 suites 7m 48s ⏱️ Results for commit fc7b139. ♻️ This comment has been updated with latest results. |
pcanal
reviewed
Mar 25, 2026
sethrj
changed the title
Add StreamId as a first parameter to make_sens_det, make_tracking_action, and make_stepping_action in IntegrationTestBase and its mixins. The stream ID is supplied by g4_worker_stream() at the call sites inside ActionInitialization::Build and TestDetectorConstruction, making the thread-local nature of these calls explicit. All mixin overrides and external test-class overrides (DistOffloadMixin, TrackingManagerIntegration, UserActionIntegration) updated accordingly. Prompt: "Update the make_sens_det, make_tracking_action, and make_stepping_action functions to take a StreamId as a first parameter to indicate they're thread-local calls. This parameter will be provided using g4_worker_stream and dispatched inside IntegrationTestBase." Assisted-by: GitHub Copilot (Claude Sonnet 4.6)
Update ShimSensitiveDetector to: - Accept StreamId::size_type as a constructor argument (not stored) indicating thread capacity, making the per-stream intent explicit at call sites - Change HitProcessor signature to void(StreamId, G4Step const*) so the stream ID is available to hit processing code - Call g4_worker_stream() in ProcessHits to supply the stream ID Update LarSphereIntegrationMixin and WaterSphereIntegrationMixin: - make_sens_det now passes num_threads() and a (StreamId, G4Step const*) lambda to ShimSensitiveDetector - process_hit(G4Step const*) becomes process_hit(StreamId, G4Step const*) Update all overrides in TrackingManagerIntegration.test.cc and FastSimulationIntegration.test.cc to match the new process_hit signature. Prompt: "Update ShimSensitiveDetector and process_hit to take StreamId as call arguments (alongside the G4Step), and have ShimSensitiveDetector take StreamId::size_type num_streams as a construction argument." Assisted-by: GitHub Copilot (Claude Sonnet 4.6)
Remove null-pointer check from LarSphereIntegrationMixin::process_hit since the step is now a reference. Dereference in ProcessHits when calling the stored function. Prompt: "Change the HitProcessor to take G4Step const& instead of a pointer." Assisted-by: GitHub Copilot (Claude Sonnet 4.6)
Instead of returning std::unique_ptr<G4VSensitiveDetector> from make_sens_det, the method now returns std::function<void(StreamId, G4Step&)> (aliased as HitFunction). The ShimSensitiveDetector class is replaced by an anonymous-namespace SensitiveDetector in IntegrationTestBase.cc that wraps the HitFunction and calls g4_worker_stream() at hit time. TestDetectorConstruction wraps the returned function in a SensitiveDetector, returning nullptr when the function is empty. TestEm3IntegrationMixin::make_sens_det returns an empty HitFunction (no SD needed since integration tests do not check hit data). The gutted ShimSensitiveDetector.hh is kept as an empty header for include-path compatibility. Prompt: "Instead of make_sens_det returning a UPSensDet, return a std::function<void(StreamId,G4Step&)>. The ShimSensitiveDetector should be an anonymous::SensitiveDetector in IntegrationTestBase.cc. Simplify or wrap existing uses of make_sens_det accordingly." Assisted-by: GitHub Copilot (claude-sonnet-4-5)
Replace DistOffloadSteppingAction (which held a shared_ptr to a single StepCounters and was created once per thread) with DistOffloadCounter, a function-like class that owns a vector<StepCounters> sized to num_streams and is shared across all threads. An anonymous SteppingAction in the .cc delegates to DistOffloadCounter::operator()(StreamId, G4Step const&), which indexes into the per-stream slot with no locking. The geant_geo_ shared pointer is lazily initialized once via std::call_once. The merged() method replaces DistOffloadMixin::merge_step_counters(), which is removed. Prompt: "Now refactor DistOffloadSteppingAction as a function-like class DistOffloadCounter. It should store a vector (size num_streams) and also have a function `merged` that sums over those, replacing merge_step_counters." Assisted-by: GitHub Copilot (claude-sonnet-4-5)
Replace the virtual make_stepping_action(StreamId) -> UPStepAction pattern with make_step_callback() -> StepCallback, where StepCallback is std::function<void(StreamId, G4Step const&)>. An anonymous SteppingAction class in IntegrationTestBase.cc wraps the callback and calls g4_worker_stream() at step time, mirroring how SensitiveDetector wraps LocalHitFunc for hit callbacks. The UPStepAction alias and the G4UserSteppingAction forward declaration are removed from the header since they are no longer part of the public API. DistOffloadMixin:: make_step_callback() initializes the shared DistOffloadCounter once (under a static mutex) and returns a capturing lambda. Prompt: "Replace make_stepping_action with make_step_callback(); the callback should be function<void(StreamId, G4Step const&)>." Assisted-by: GitHub Copilot (claude-sonnet-4-5)
Move the make_step_callback() call from ActionInitialization::Build() (called per worker thread) into the constructor (called once on the master thread). The resulting StepCallback is stored as step_cb_ and copied into each worker's SteppingAction — std::function is copyable, so all threads share the same underlying callable. Remove the static mutex from DistOffloadMixin::make_step_callback() since it is now guaranteed to be called exactly once. Prompt: "I'd like make_step_callback to be called only once, and the resulting callback shared among the streams." Assisted-by: GitHub Copilot (claude-sonnet-4-5)
Move all function definitions from headers to .cc files for SensitiveDetector, SteppingAction, and StateDependent. Add Doxygen documentation to class and function definitions. Add missing StreamId include to StateDependent.hh, missing constructor definition to StateDependent.cc, and required includes (G4Step, corecel/Assert) to SensitiveDetector.cc and SteppingAction.cc. Remove redundant Geant4 headers from IntegrationTestBase.cc that are now transitively included via the new wrapper headers. Fix type alias block structure in SensitiveDetector.hh and update Threading.hh comment phrasing. Prompt: "In the last commit, I created some new files and moved code from IntegrationTestBase.cc into them, but the work is incomplete. In those files, move all function definitions to the .cc, add documentation and formatting to the class and functions, and ensure the code compiles. Update IntegrationTestBase.cc accordingly." Assisted-by: GitHub Copilot (claude-sonnet-4-5)
Add StateDependent::GeantStateChange enum and change callback signature to accept the enum. Map (previous, requested) Geant4 states to semantic enum values in StateDependent::Notify and update documentation table. Prompt: "Define a new enum GeantStateChange with begin_run etc., change the LocalStateChangeFunc signature to have a GeantStateChagne instead of two AppState, and update the documentation with a table that encodes (before, after) -> change..." Assisted-by: agentic-tool (GPT-5 mini)
sethrj
force-pushed
the
accel-test-changes
branch
from
8d1bdfd to
d9ce78c
Compare
sethrj
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2332 +/- ##
===========================================
- Coverage 87.25% 85.80% -1.45%
===========================================
Files 1386 1355 -31
Lines 43972 42368 -1604
Branches 13469 10947 -2522
===========================================
- Hits 38367 36354 -2013
- Misses 4377 4384 +7
- Partials 1228 1630 +402
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This refactors the
IntegrationTestBaseto rely onStreamIdinstead of thread-local data, partly as an example to @drbenmorgan of what I'm suggesting as a transition away from thread-local data:This formulation removes some ambiguity about what master vs worker means:
StreamId{0}will always run events;StreamId{}never will. Comparing againstgeant_main_stream()allows a single thread to initialize shared data.Instead of overriding
G4Vand instantiating it separately for each thread, we definewhich replaces both
G4UserSteppingActionandG4VSensitiveDetectoroverrides with a single function call, which in most of theacceltests directly reference the test harness itself. This removes some of the complexity of managing thread-local data because it allows the target class/function to decide how to allocate/merge data.