feat: introduction obj simhit writer #3180
Conversation
WalkthroughA new source file, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@github-actions[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 29 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 7
🧹 Outside diff range and nitpick comments (5)
Examples/Python/python/acts/examples/simulation.py (1)
591-603: Proper implementation of OBJ writer configuration, I sense!Follows the same pattern as CSV and ROOT writers, this implementation does. Directory creation and writer configuration, handled correctly they are.
A suggestion for defensive programming, I have:
if outputDirObj is not None: outputDirObj = Path(outputDirObj) if not outputDirObj.exists(): outputDirObj.mkdir() + if simHits is None: + return s.addWriter( acts.examples.ObjSimHitWriter( level=customLogLevel(),Examples/Io/Obj/src/ObjSimHitWriter.cpp (2)
84-84: Typo in comment, there is: 'immplementation' should be 'implementation'.Correct the spelling, you must.
Apply this diff to fix the typo:
- // Write data from internal immplementation + // Write data from internal implementation
119-119: Use 'try_emplace', you should, to improve performance.Unnecessary searching before insertion, there is. Replace with
try_emplace, we can.Apply this diff to enhance the code:
- particleHits[simHit.particleId().value()] = {}; + particleHits.try_emplace(simHit.particleId().value(), {});🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 119-119: Searching before insertion is not necessary. Instead of 'particleHits[simHit.particleId().value()]={}' consider using 'particleHits.try_emplace(simHit.particleId().value(), {});'.
(stlFindInsert)
Examples/Scripts/Python/full_chain_odd.py (2)
82-87: Input validation for '--gun-eta-range', add you should.Assumption that two values provided are, dangerous it is. Error handling for incorrect number of arguments, implement you must. Users' mistakes, gracefully handle you should.
Consider adding a check after parsing arguments:
if len(args.gun_eta_range) != 2: parser.error("--gun-eta-range requires exactly two values")
89-94: Input validation for '--gun-pt-range', necessary it is.Ensure that two values for pt range provided are. Validation, add to prevent errors from incorrect inputs.
Add this check:
if len(args.gun_pt_range) != 2: parser.error("--gun-pt-range requires exactly two values")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Examples/Io/Obj/CMakeLists.txt(1 hunks)Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjSimHitWriter.hpp(1 hunks)Examples/Io/Obj/src/ObjSimHitWriter.cpp(1 hunks)Examples/Io/Obj/src/ObjTrackingGeometryWriter.cpp(1 hunks)Examples/Python/python/acts/examples/simulation.py(8 hunks)Examples/Python/src/Output.cpp(2 hunks)Examples/Scripts/Python/full_chain_odd.py(7 hunks)Examples/Scripts/Python/sim_digi_odd.py(0 hunks)
💤 Files with no reviewable changes (1)
- Examples/Scripts/Python/sim_digi_odd.py
✅ Files skipped from review due to trivial changes (1)
- Examples/Io/Obj/src/ObjTrackingGeometryWriter.cpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Examples/Io/Obj/src/ObjSimHitWriter.cpp
[performance] 119-119: Searching before insertion is not necessary. Instead of 'particleHits[simHit.particleId().value()]={}' consider using 'particleHits.try_emplace(simHit.particleId().value(), {});'.
(stlFindInsert)
🔇 Additional comments (11)
Examples/Io/Obj/CMakeLists.txt (1)
3-3: Hmmmm, proper integration of the new source file, I sense.
Correctly added to the shared library, the new ObjSimHitWriter.cpp is. With the Force of proper CMake practices, aligned this change is.
Verify the source file's existence and structure, we must:
✅ Verification successful
Verified the source files and their structure are, hmmmm.
Found both the source and header files in their proper places, I have. Well-structured the ObjSimHitWriter class is, inheriting from WriterT<SimHitContainer> with proper configuration and implementation patterns, it does. Matches the CMake integration perfectly, this does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and basic structure of the new source file and its header
# Check if the source and header files exist
echo "Checking for source and header files..."
fd "ObjSimHitWriter" --type f
# Verify basic structure of the files
echo "Verifying class definition..."
ast-grep --pattern 'class ObjSimHitWriter'
Length of output: 5120
Examples/Python/src/Output.cpp (2)
28-30: LGTM! Well organized, the headers are, hmm.
In proper order and consistent style, these new includes follow. A good addition to our Jedi archives, they are.
113-115: LGTM! Balanced and harmonious, this writer declaration is.
Like other writers in our temple, this one follows the sacred patterns. The Force flows strong through its parameters, it does.
Verify the usage of these parameters elsewhere, we must.
✅ Verification successful
Verified, the parameters are. Balanced in the Force, they remain.
Through the Force, I sense the parameters flow naturally:
inputSimHits: Used consistently across writers and algorithms for simulation hit inputoutputDir: Common path for writing files, used throughout the codebaseoutputStem: Base name for output files, properly utilizedoutputPrecision: Controls floating-point precision in outputdrawConnections: Specific to ObjSimHitWriter for visualization control
Strong with the Force, these parameters are. In harmony with the rest of the codebase, they exist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ObjSimHitWriter usage in Python files
rg -t py "ObjSimHitWriter"
# Search for parameter usage
rg "inputSimHits|outputDir|outputStem|outputPrecision|drawConnections" --type cpp
Length of output: 22841
Examples/Python/python/acts/examples/simulation.py (3)
425-425: Consistent parameter addition, hmm, yes!
Well-documented and properly typed, the new parameter is. Follows the established pattern of other output parameters, it does.
Also applies to: 450-451
537-537: Consistent propagation of parameter through function chain, observe I do!
Through the chain of functions, the new parameter flows smoothly. From addGeant4 to addSimWriters, properly passed it is. Documentation matches established patterns, yes.
Also applies to: 653-653, 681-682, 782-782
Line range hint 1-900: Verify the integration with ObjSimHitWriter class, we must!
The force guides me to verify the proper integration of the new writer class.
✅ Verification successful
Properly integrated, the ObjSimHitWriter is, hmmmm.
Verified the implementation have I, and strong with the Force it is! The writer class:
- Complete Python bindings it has, in Examples/Python/src/Output.cpp
- Proper class definition with configuration options it shows
- Robust implementation with hit visualization and trajectory interpolation it possesses
- Thread-safe writing with mutex protection it maintains
- Clear error handling and proper file management it demonstrates
In the simulation.py file, correctly used the writer is, through the addSimWriters function. Matches the class interface perfectly, it does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ObjSimHitWriter class definition and usage
echo "=== Searching for ObjSimHitWriter class definition ==="
rg -l "class ObjSimHitWriter"
echo "=== Searching for ObjSimHitWriter includes ==="
rg -l "#include.*ObjSimHitWriter"
Length of output: 424
Script:
#!/bin/bash
# Check ObjSimHitWriter class definition and implementation
echo "=== ObjSimHitWriter Class Definition ==="
cat Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjSimHitWriter.hpp
echo "=== ObjSimHitWriter Python Bindings ==="
cat Examples/Python/src/Output.cpp | grep -A 10 -B 10 "ObjSimHitWriter"
echo "=== ObjSimHitWriter Implementation ==="
cat Examples/Io/Obj/src/ObjSimHitWriter.cpp
Length of output: 11915
Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjSimHitWriter.hpp (2)
37-84: Well-structured your class is, approve it I do.
The ObjSimHitWriter class and its Config are well-defined. Conforms to best practices, it does.
36-36:
Misplaced code within comments, found it is.
At line 36, the class declaration appears within a comment block. This may cause parsing errors. Ensure code and comments are correctly separated.
Apply this diff to correct the placement:
-/// ...class ObjSimHitWriter final : public WriterT<SimHitContainer> {
+///
+class ObjSimHitWriter final : public WriterT<SimHitContainer> {Likely invalid or redundant comment.
Examples/Scripts/Python/full_chain_odd.py (3)
235-236: Consistency in particle configuration, verify you must.
'args.gun_particles' used for number of particles per vertex, it seems. 'args.gun_multiplicity' for number of vertices is. Confusion between the two, ensure there is not. Description clarity, improve you should.
Review the usage of 'args.gun_particles' and 'args.gun_multiplicity' to ensure they align with intended behavior.
228-229: Units application in 'MomentumConfig', double-check you should.
With defaults corrected, 'args.gun_pt_range' now in GeV they are. Multiplying by 'u.GeV', units correct this makes. Carefully ensure that units consistent they remain.
Confirm that 'args.gun_pt_range' values multiplied by 'u.GeV' result in correct units within 'MomentumConfig'.
228-231:
Duplicate unit multiplication in 'MomentumConfig', avoid you must.
Already in GeV, 'args.gun_pt_range' values are, after adjusting defaults. Multiplying by 'u.GeV' again, incorrect it is. Units, only once apply you should.
Apply this diff to correct the unit handling:
MomentumConfig(
- args.gun_pt_range[0] * u.GeV,
- args.gun_pt_range[1] * u.GeV,
+ args.gun_pt_range[0] * u.UnitConstants.GeV,
+ args.gun_pt_range[1] * u.UnitConstants.GeV,
transverse=True,
),Likely invalid or redundant comment.
Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjSimHitWriter.hpp
Outdated
Show resolved
Hide resolved
asalzburger
force-pushed
the
feat-obj-simhit-writer
branch
from
eccba9f to
44e59d3
Compare
Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjSimHitWriter.hpp
Outdated
Show resolved
Hide resolved
Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjSimHitWriter.hpp
Outdated
Show resolved
Hide resolved
Possible, but the IO code is so contained and almost not exposed that it would probably be an overkill. I would prefer to do it once as the PR is not very big. |
Co-authored-by: Alexander J. Pfleger <[email protected]>
|
* support `--output-obj` like #3180. Can also still use `-cc` to output all formats. * Fuse initial and final sim particles when `--edm4hep` input specified (sync with #3804) Next, we should remove the command-line options for `full_chain_odd.py`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new `--output-obj` argument for enabling object file output. - **Improvements** - Updated help text for the `--output-csv` argument to clarify output options. - Enhanced output directory handling to support multiple output formats. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This PR adds a wavefront
.objsim hit writer to the Examples.Added (optional) spline fit based on
Eigen::SplineIn addition it corrects the include path of Obj based writers to
Io/Obit still was legacyPlugins/Obj.Summary by CodeRabbit
New Features
ObjSimHitWriterclass for writing simulation hit data to OBJ files.outputDirObjto several simulation functions for enhanced output management.full_chain_odd.pyfor more granular control over particle gun configurations.Bug Fixes
ObjSimHitWriter.Chores
sim_digi_odd.pyscript as part of cleanup.