feat: adding G4 stepping #3812
feat: adding G4 stepping #3812
Conversation
WalkthroughThe changes in this pull request introduce several enhancements to the Geant4 simulation framework within the ACTS library. Key modifications include the addition of new member variables for tracking propagation summaries and step logging, updates to configuration structures, and enhancements to the handling of simulation data. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
Examples/Algorithms/Geant4/include/ActsExamples/Geant4/SensitiveSteppingAction.hpp (1)
44-46: LGTM! Consider adding documentation for the new parameter.The addition of the
stepLoggingparameter is well-placed and follows the existing configuration pattern. However, it would be helpful to add a brief documentation comment explaining:
- The purpose of step logging
- What information gets logged when enabled
- Where the logs are output
Add documentation like this:
bool secondary = true; + /// Controls whether to log detailed information about each simulation step. + /// When enabled, [describe what gets logged and where] bool stepLogging = false;Examples/Algorithms/Geant4/include/ActsExamples/Geant4/Geant4Simulation.hpp (1)
128-130: LGTM: Configuration options are well-structured.The new configuration options are logically organized and have appropriate default values.
Consider adding documentation comments for these new options to clarify their purpose and impact:
+ /// Name of the output collection for propagation records. + /// Only used when recordPropagationSummaries is true. std::string outputPropagationSummaries = "propagation_summaries"; + /// Whether to record hits for charged particles bool recordHitsOfCharged = true; + /// Whether to record hits for primary particles bool recordHitsOfPrimaries = true; + /// Whether to record propagation summaries for debugging bool recordPropagationSummaries = false;Also applies to: 145-146, 149-150, 154-155
Examples/Algorithms/Geant4/src/Geant4Simulation.cpp (1)
338-347: Consider minor improvements to propagation summary handlingThe implementation of propagation summary output is well-structured and efficient. Consider these minor improvements:
- Add a brief comment explaining the purpose and structure of PropagationSummaries
- Make the trackId const in the range-based for loop since it's not modified:
- for (auto& [trackId, summary] : eventStore().propagationRecords) { + for (const auto& [trackId, summary] : eventStore().propagationRecords) {Examples/Python/src/Geant4Component.cpp (1)
218-222: Document performance implications of hit recording options.The new granular hit recording options (
recordHitsOfCharged,recordHitsOfNeutrals,recordHitsOfPrimaries,recordHitsOfSecondaries) provide excellent control but may impact simulation performance when enabled. Consider:
- Adding documentation about the performance implications of enabling each option
- Providing recommended configurations for common use cases
Examples/Algorithms/Geant4/src/SensitiveSteppingAction.cpp (2)
61-61: Ensure consistent unit conversion definitionsConsider moving the unit conversion constants (
convertLength,convertEnergy,convertTime) to a shared location if they are used in multiple places within the codebase to promote consistency and maintainability.
128-129: Redundant retrieval of step pointsSince
kinematicsOfStepretrievespreStepPointandpostStepPoint, consider using it here to avoid redundant code and enhance consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
Examples/Algorithms/Geant4/include/ActsExamples/Geant4/EventStore.hpp(2 hunks)Examples/Algorithms/Geant4/include/ActsExamples/Geant4/Geant4Simulation.hpp(4 hunks)Examples/Algorithms/Geant4/include/ActsExamples/Geant4/SensitiveSteppingAction.hpp(1 hunks)Examples/Algorithms/Geant4/src/Geant4Simulation.cpp(3 hunks)Examples/Algorithms/Geant4/src/SensitiveSteppingAction.cpp(11 hunks)Examples/Algorithms/Propagation/CMakeLists.txt(1 hunks)Examples/Algorithms/Propagation/include/ActsExamples/Propagation/SimHitToSummaryConversion.hpp(0 hunks)Examples/Algorithms/Propagation/src/SimHitToSummaryConversion.cpp(0 hunks)Examples/Python/python/acts/examples/simulation.py(1 hunks)Examples/Python/src/Geant4Component.cpp(1 hunks)Examples/Python/src/Propagation.cpp(0 hunks)
💤 Files with no reviewable changes (3)
- Examples/Algorithms/Propagation/include/ActsExamples/Propagation/SimHitToSummaryConversion.hpp
- Examples/Algorithms/Propagation/src/SimHitToSummaryConversion.cpp
- Examples/Python/src/Propagation.cpp
🔇 Additional comments (19)
Examples/Algorithms/Propagation/CMakeLists.txt (1)
1-1: LGTM! Verify downstream dependencies.
The removal of SimHitToSummaryConversion.cpp from the library sources aligns with the PR objective of eliminating the SimHitConversion algorithm.
Let's verify if there are any downstream dependencies that might be affected:
✅ Verification successful
Changes to ActsExamplesPropagation library are safe to proceed
The comprehensive search across the codebase shows no references to SimHitToSummaryConversion, indicating that:
- No other CMake targets depend on this component
- No source files include or use this functionality
- The class definition has been completely removed
This confirms that removing SimHitToSummaryConversion.cpp from the library sources is safe and won't impact any downstream dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential downstream dependencies on the removed SimHitToSummaryConversion functionality
# Search for any references to SimHitToSummaryConversion in CMake files
echo "Checking CMake files for dependencies..."
rg -l "SimHitToSummaryConversion" --type cmake
# Search for any C++ includes of the removed header
echo "Checking source files for includes..."
rg -l "#include.*SimHitToSummaryConversion" --type cpp --type hpp
Length of output: 318
Script:
#!/bin/bash
# Let's try a more comprehensive search without file type restrictions
# and broaden the search patterns
echo "Searching for any mentions of SimHitToSummaryConversion..."
rg -l "SimHitToSummaryConversion"
echo -e "\nSearching for any mentions of SimHit.*Conversion..."
rg -l "SimHit.*Conversion"
# Let's also check if there are any remaining references to this class pattern
echo -e "\nSearching for class definition or usage..."
ast-grep --pattern 'class SimHitToSummaryConversion'
Length of output: 464
Examples/Algorithms/Geant4/include/ActsExamples/Geant4/SensitiveSteppingAction.hpp (1)
44-46: Verify integration with propagation summary recording.
Since this change is part of a larger feature to use Geant4 as a propagation reference, let's verify the relationship between stepLogging and the propagation summary functionality mentioned in the AI summary.
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have enough information about how stepLogging integrates with propagation summaries:
stepLogging is correctly integrated with propagation summary recording
The code shows proper integration between stepLogging and propagation summaries:
stepLoggingis controlled byrecordPropagationSummariesinGeant4Simulation.cpp- When enabled, it properly initializes propagation summaries for each track and records step information
- The data flows correctly through the
EventStoreand gets written viaRootPropagationSummaryWriter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how stepLogging is used with propagation summaries
# Look for files that use stepLogging
rg "stepLogging" -l
# Check for propagation summary related code
rg "PropagationSummary|recordPropagationSummaries" -A 5
# Look for the implementation file
fd "SensitiveSteppingAction.cpp"
Length of output: 22215
Examples/Algorithms/Geant4/include/ActsExamples/Geant4/EventStore.hpp (2)
12-12: LGTM: Include statement for PropagationSummary is well-placed.
The addition of PropagationSummary.hpp aligns with the PR's objective of using Geant4 as a propagation reference and follows the existing include organization pattern.
56-58: LGTM: Well-structured addition of propagation records.
The new propagationRecords member is:
- Appropriately typed as an unordered_map for efficient track ID lookups
- Well-documented with its purpose
- Logically placed with other tracking-related members
- Consistent with the PR's goal of using Geant4 as a propagation reference
Let's verify the PropagationSummary type definition and usage:
✅ Verification successful
LGTM: PropagationSummary is well-defined and properly integrated
The verification confirms:
PropagationSummaryis properly defined inExamples/Framework/include/ActsExamples/EventData/PropagationSummary.hpp- It's consistently used across the codebase, including Geant4 integration, propagation algorithms, and IO operations
- No legacy
SimHitConversionreferences were found - The new member's type and usage aligns with the existing propagation infrastructure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify PropagationSummary definition and its usage in related files
echo "Checking PropagationSummary definition and usage..."
rg -t hpp -t cpp "PropagationSummary" --no-heading
# Check if there are any remaining references to SimHitConversion
echo "Verifying removal of SimHitConversion..."
rg -t hpp -t cpp "SimHitConversion" --no-heading
Length of output: 359
Script:
#!/bin/bash
# Let's try without file type filters since they weren't recognized
echo "Checking PropagationSummary definition and usage..."
rg "struct.*PropagationSummary|class.*PropagationSummary" -A 5
echo "Checking all PropagationSummary references..."
rg "PropagationSummary"
echo "Checking for any SimHitConversion references..."
rg "SimHitConversion"
Length of output: 6661
Examples/Algorithms/Geant4/include/ActsExamples/Geant4/Geant4Simulation.hpp (3)
13-13: LGTM: Include addition is appropriate.
The PropagationSummary header inclusion is properly placed and necessary for the new propagation summary functionality.
189-190: LGTM: Data handle addition follows established patterns.
The new WriteDataHandle for PropagationSummaries is properly implemented and consistent with the class's data handling approach.
13-13: Verify integration with related components.
The changes look good overall, but let's verify the integration with related components.
Also applies to: 128-130, 145-146, 149-150, 154-155, 189-190
✅ Verification successful
Integration verified successfully
The PropagationSummary integration is properly implemented across the codebase:
- PropagationSummary is well-defined in
Examples/Framework/include/ActsExamples/EventData/PropagationSummary.hpp - Implementation in Geant4Simulation correctly handles the configuration and data writing
- Python bindings are properly updated with the new configuration options
- Multiple components are properly using PropagationSummary, including writers, propagators, and algorithms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the new configuration options and data handle
# Test 1: Check if PropagationSummary is properly defined
echo "Checking PropagationSummary definition..."
rg -l "class PropagationSummary" "Examples/EventData"
# Test 2: Check for corresponding changes in implementation file
echo "Checking implementation file..."
rg -l "m_outputPropagationSummaries" "Examples/Algorithms/Geant4/src"
# Test 3: Check for Python binding updates
echo "Checking Python bindings..."
rg "outputPropagationSummaries|recordPropagationSummaries" "Examples/Python"
Length of output: 817
Script:
#!/bin/bash
# Let's verify the PropagationSummary definition in the correct location
echo "Checking PropagationSummary definition..."
fd PropagationSummary.hpp
# Check implementation details
echo -e "\nChecking implementation and usage..."
rg -A 2 "m_outputPropagationSummaries" Examples/Algorithms/Geant4/src/Geant4Simulation.cpp
# Check if there are any other related components using PropagationSummary
echo -e "\nChecking related usages..."
rg -l "PropagationSummary" --type cpp --type hpp
Length of output: 671
Script:
#!/bin/bash
# Check the content of PropagationSummary.hpp
echo "Checking PropagationSummary definition content..."
cat Examples/Framework/include/ActsExamples/EventData/PropagationSummary.hpp
# Check for related usages with correct file extensions
echo -e "\nChecking related usages..."
rg -l "PropagationSummary" -g "*.cpp" -g "*.hpp"
# Check if the implementation properly handles the configuration
echo -e "\nChecking configuration handling..."
rg -A 5 "cfg.outputPropagationSummaries" Examples/Algorithms/Geant4/src/Geant4Simulation.cpp
Length of output: 3078
Examples/Algorithms/Geant4/src/Geant4Simulation.cpp (2)
236-240: LGTM: Configuration changes enhance flexibility
The new configuration options provide fine-grained control over hit recording behavior, aligning well with the PR's objective of adding G4 stepping capabilities. The configuration is well-structured and follows good naming practices.
304-307: LGTM: Proper initialization of propagation summary output
The output handle initialization is correctly guarded by the configuration flag and follows the established pattern in the codebase.
Examples/Python/src/Geant4Component.cpp (1)
211-211: Clarify the relationship between propagation summary configuration options.
There appears to be potential redundancy between outputPropagationSummaries and recordPropagationSummaries. Please clarify:
- Is one for controlling recording and the other for output?
- Are both needed or can they be consolidated?
Also applies to: 223-223
Examples/Python/python/acts/examples/simulation.py (2)
732-736: LGTM: Well-structured parameter additions for hit recording control.
The new parameters provide granular control over hit recording for different particle types with sensible defaults:
recordHitsOfCharged: Controls hit recording for charged particlesrecordHitsOfNeutrals: Controls hit recording for neutral particlesrecordHitsOfPrimaries: Controls hit recording for primary particlesrecordPropagationSummaries: Controls propagation summary recording
732-736: LGTM: Configuration changes properly integrate with Geant4Simulation.
The new hit recording parameters are correctly passed to the Geant4Simulation constructor, maintaining a clean and logical configuration structure.
Examples/Algorithms/Geant4/src/SensitiveSteppingAction.cpp (7)
14-14: Approved inclusion of necessary header for stepping logger
The addition of #include "Acts/Propagator/detail/SteppingLogger.hpp" ensures that the stepping logger functionalities are available, which is necessary for the step logging features introduced.
23-23: Approved inclusion of <array> header
The inclusion of the <array> header is appropriate as it is required for the use of std::array in the new functions.
58-86: Well-structured function for kinematic data extraction
The new kinematicsOfStep function cleanly encapsulates the extraction and unit conversion of kinematic data from G4Step, improving code readability and reusability.
88-96: Simplified hitFromStep function with improved parameter handling
Updating hitFromStep to accept a G4Step* streamlines the function and leverages the new kinematicsOfStep function, enhancing code clarity and reducing redundancy.
99-109: Addition of stepFromG4Step function enhances code modularity
The new stepFromG4Step function provides a clean abstraction for converting a G4Step into an Acts::detail::Step, improving code organization and facilitating step logging.
242-256: Efficient logging of propagation steps
The use of stepFromG4Step within the step logging logic enhances the clarity and efficiency of the code, ensuring that step data is accurately recorded.
300-300: Ensure consistent usage of hit index
In lines 300, 313, and 341, hitFromStep is called with different index values. In line 300, the index is set to eventStore().particleHitCount.at(particleId) - 1, while in lines 313 and 341, the index is -1. Verify that using -1 as the index does not cause issues in downstream processing, such as hit identification or serialization.
Run the following script to check the usage of the index parameter in ActsFatras::Hit and any assumptions about its value:
Also applies to: 313-313, 341-341
✅ Verification successful
Let me gather more information about the hitFromStep function and its usage to better understand the index parameter handling.
Based on the code analysis, I can now generate the final response:
Using -1 as hit index is intentional and safe
The hit index parameter in hitFromStep is used differently in these scenarios:
- When storing final hits (
eventStore().hits), it usesparticleHitCount - 1to maintain sequential indexing - When creating temporary hits for buffering (
hitBuffer), it uses -1 as these are intermediate hits that will be combined into a final hit
The codebase shows no validation or assumptions about hit indices being non-negative. The index is primarily used for bookkeeping in the event store and doesn't affect hit identification or serialization, which rely on geometry and particle IDs instead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search the codebase for uses of `ActsFatras::Hit` constructor and check how the `index` parameter is utilized.
# Find all instances where `ActsFatras::Hit` is constructed
rg -A 5 'ActsFatras::Hit\s*\('
# Search for code that uses the `index` value of hits
rg -A 3 'hit.index\('
# Look for any assertions or conditions that `index` >= 0
rg 'index\s*<\s*0|index\s*>=\s*0'
Length of output: 3025
Script:
#!/bin/bash
# Search for the hitFromStep function implementation and its usages
ast-grep --pattern 'hitFromStep($$$)'
# Search for any validation or checks related to hit indices
rg -A 5 'particleHitCount|hitFromStep'
# Look for any hit processing or validation code
rg -A 5 'class.*Hit|struct.*Hit'
Length of output: 40211
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Examples/Algorithms/Geant4/src/SensitiveSteppingAction.cpp(11 hunks)
🔇 Additional comments (3)
Examples/Algorithms/Geant4/src/SensitiveSteppingAction.cpp (3)
58-86: LGTM! Well-structured kinematics extraction.
The function effectively extracts and converts kinematic information from G4Step, with proper unit handling and clear organization of pre/post step data.
88-97: LGTM! Clean refactoring of hit creation.
The function has been effectively simplified by using G4Step directly and leveraging the new kinematicsOfStep function, while maintaining the same functionality.
Line range hint 300-341: LGTM! Consistent application of refactored hit creation.
The changes effectively integrate the refactored hitFromStep function while maintaining the existing hit buffer logic and functionality.
|
A bit of follow-up: as one can see the mid-point implementation creates steps exactly at hypothetical cylinders, independent from the Runge-Kutta-Stepping |
|
I thought the In ACTS I think we don't store the previous position ( I wonder how After thinking about it I beliefe what G4 calls |
Yes, post-step would be more appropriate comparing to our |
|
I also thought about the Right the mapping from volume based to surface based is where we will ultimately clash with G4. I wonder if we can push these necessary conversion to the very end of the processing? Keeping the stepping info decoupled from any kind of geo/navigation info but later relate them if necessary. But I don't know yet what the use case is. |
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
I will make a PR for that, let me resolve the conflicts first here. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Good to go. |
|
Now should be fine. |
|
acts-project-service
added
the
Breaks Athena build
This adds the possibility to use Geant4 as a propagation reference without the need of the SimHitConversion algorithm
Summary by CodeRabbit
Release Notes
New Features
Geant4Simulationfor tracking particle interactions with new parameters for recording hits and propagation summaries.SensitiveSteppingActionto control logging behavior during simulations.Bug Fixes
Refactor
SensitiveSteppingActionfor better clarity and maintainability.SimHitToSummaryConversionalgorithm to focus on core propagation functionalities.Documentation