feat: move MaterialTrackWriting infrastructure to Core + first use of RNTuple #4043
Conversation
WalkthroughEnhancements to the ROOT material track handling in the ACTS project, this pull request introduces. A new Changes
Suggested Labels
Suggested Reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 (
|
github-actions
bot
added
Component - Examples
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
Plugins/Root/include/Acts/Plugins/Root/RootMaterialTrack.hpp (5)
35-37: Typographical error in comment, there is.In the comment on line 37, "aut" should be corrected to "out" or "both". Clearer, it will be.
37-39: "Correspond", the correct spelling is.On lines 39 and 40, "correpond" should be "correspond". Improve clarity, this will.
179-179: Misspelled "method", you have.In the comments on lines 179 and 184, "mehtod" should be "method". Attention to detail, important it is.
Also applies to: 184-184
181-181: Typo in parameter description, there is.On line 181, "wread" should be "read". Clear communication, ensure you must.
210-210: Correct the spelling of "Payload", you should.In the comment on line 210, "Paylod" should be "Payload". Precision, value highly we do.
Examples/Io/Root/src/RootMaterialTrackWriter.cpp (1)
61-61: Update the comment to reflect RNTupleModel initialization, you must.The comment mentions
RDataFrame, but initializingRNTupleModel, the code is.Apply this diff to correct the comment:
- /// Initialize the RDataFrame + /// Initialize the RNTupleModel
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)Examples/Io/Root/CMakeLists.txt(1 hunks)Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackReader.hpp(3 hunks)Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackWriter.hpp(5 hunks)Examples/Io/Root/src/RootMaterialTrackReader.cpp(4 hunks)Examples/Io/Root/src/RootMaterialTrackWriter.cpp(4 hunks)Plugins/CMakeLists.txt(1 hunks)Plugins/Root/CMakeLists.txt(1 hunks)Plugins/Root/include/Acts/Plugins/Root/RootMaterialTrack.hpp(1 hunks)Plugins/Root/src/RootMaterialTrack.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: missing_includes
- GitHub Check: build_debug
🔇 Additional comments (19)
Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackReader.hpp (3)
13-13: IncludeRootMaterialTrack.hpp, wisely you have.Necessary for utilizing
Acts::RootMaterialTrack, it is.
41-41: Inheritance fromActs::RootMaterialTrack::Config, beneficial it is.Streamlines configuration, this change does.
101-102: Memberm_rootMaterialTrack, introduced you have.Ensure proper initialization and usage, you must. Verify that all methods utilize this member correctly.
Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackWriter.hpp (5)
11-16: Essential includes, added they are.Dependencies for
Acts::Definitions::AlgebraandWriterT, required they are.
28-32: Forward declaration ofRNTupleWriter, cautiously proceed you should.Ensure that forward declaration suffices and no incomplete type issues arise.
60-60: Config inherits fromActs::RootMaterialTrack::Config, harmonized it is.Enhances code reuse and consistency, this change does.
69-70: Configuration optionrnTuple, introduced you have.Controls RNTuple writing, this addition does. Default value set to
false, it is.
108-111: Member variables for RNTuple writing and material track, added they are.Ensure proper initialization and cleanup, you must. Manage resources correctly, important it is.
Examples/Io/Root/src/RootMaterialTrackReader.cpp (4)
32-33: Initializem_rootMaterialTrackwithconfig, correctly you have.Configures material track reader appropriately, this does.
51-51: Reading initialized withinitializeRead, well done this is.Prepares input chain for reading, this method does.
74-74: Destructor simplified, but mindful you must be.Ensure all resources and memory are properly released. Verify that no memory leaks occur.
108-109: Material track read withm_rootMaterialTrack.read(), simplified it is.Streamlines the reading process, this change does.
Plugins/Root/include/Acts/Plugins/Root/RootMaterialTrack.hpp (1)
1-212: Code structure and implementation, commendable it is.Well-designed and consistent the class appears. Good work, you have done.
Plugins/Root/src/RootMaterialTrack.cpp (1)
1-471: Implementation, strong it is.Thoroughly reviewed, I have. Functionality correct and efficient, it seems.
Plugins/Root/CMakeLists.txt (1)
1-20: CMake configuration, proper it appears.No issues found in your build setup. Proceed, you may.
Plugins/CMakeLists.txt (1)
18-18: Correct, the placement is, but verify dependencies we must!Before TGeo plugin, the Root plugin stands. Wise this ordering seems, but confirm the dependency chain we should.
Run this command to verify plugin dependencies:
Examples/Io/Root/CMakeLists.txt (1)
42-42: Wise addition to dependencies, this is!Public linking of ActsPluginRoot, proper it is. Harmony with the new Root plugin architecture, it brings.
CMakeLists.txt (1)
414-414: Additional ROOT components, required they are. But verify their presence in minimum version, we must!RIO and ROOTDataFrame components, added they were. Essential for material track handling, they are. But ensure their availability in minimum ROOT version ${_acts_root_version}, we should.
Run this command to verify component availability:
Examples/Io/Root/src/RootMaterialTrackWriter.cpp (1)
77-77: Ensure destructor handles resource cleanup, you should.Empty the destructor is. If resources need explicit release, handle them here, you must. Confirm that automatic cleanup sufficient is.
github-actions
bot
added
the
Infrastructure
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
Examples/Io/Root/src/RootMaterialTrackWriter.cpp (2)
66-73:⚠️ Potential issueHandle RNTuple case in finalize(), you must.
The finalize method, handle both TTree and RNTuple cases it should.
64-64:⚠️ Potential issueHandle cleanup in destructor, you must.
Empty destructor, I see. Resources properly released, they must be. In destructor or finalize method, cleanup should occur.
Apply this diff to ensure proper cleanup:
-RootMaterialTrackWriter::~RootMaterialTrackWriter() {} +RootMaterialTrackWriter::~RootMaterialTrackWriter() { + if (m_outputFile != nullptr) { + m_outputFile->Close(); + m_outputFile = nullptr; + } +}
🧹 Nitpick comments (7)
Plugins/Root/src/RootMaterialTrack.cpp (5)
363-366: Replace dynamic_cast with type() check, you should.Performance impact, dynamic_cast has. More efficient approach using surface->type(), available it is.
Apply this diff for better performance:
- auto radialBounds = dynamic_cast<const RadialBounds*>(&surfaceBounds); - auto cylinderBounds = - dynamic_cast<const CylinderBounds*>(&surfaceBounds); - if (radialBounds != nullptr) { + if (surface->type() == Surface::RadialBounds) { + const auto& radialBounds = static_cast<const RadialBounds&>(surfaceBounds);
379-379: Magic number -1, replace with named constant you must.Clear meaning to magic number -1, give we should. Understanding and maintenance, easier it becomes.
Add this constant at the top of the file:
+ static constexpr int UNKNOWN_SURFACE_TYPE = -1; - m_payload.surfaceType->emplace_back(-1); + m_payload.surfaceType->emplace_back(UNKNOWN_SURFACE_TYPE);
432-435: Early continue with zero step length, document you should.Reason for skipping zero step length, clear it must be. Documentation helps understanding.
Add comment explaining the skip:
double s = m_payload.stepLength->at(is); + // Skip material interactions with zero thickness as they don't contribute if (s == 0) { continue; }
363-377: Safer handling of surface bounds, implement you should.Dynamic casting for bounds checking, risky it is. Visitor pattern, a better approach would be.
Consider implementing a visitor pattern:
-auto radialBounds = dynamic_cast<const RadialBounds*>(&surfaceBounds); -auto cylinderBounds = dynamic_cast<const CylinderBounds*>(&surfaceBounds); -if (radialBounds != nullptr) { - m_payload.surfaceRangeMin->emplace_back(radialBounds->rMin()); - m_payload.surfaceRangeMax->emplace_back(radialBounds->rMax()); -} else if (cylinderBounds != nullptr) { +class BoundsVisitor { +public: + void visit(const RadialBounds& bounds) { + min = bounds.rMin(); + max = bounds.rMax(); + } + void visit(const CylinderBounds& bounds) { + min = -bounds.get(CylinderBounds::eHalfLengthZ); + max = bounds.get(CylinderBounds::eHalfLengthZ); + } + double min = 0; + double max = 0; +};
363-366: Use type() instead of dynamic_cast, you should.Performance impact, dynamic_cast has. Better approach using surface->type() and switch statement would be.
Consider this alternative approach:
- auto radialBounds = dynamic_cast<const RadialBounds*>(&surfaceBounds); - auto cylinderBounds = - dynamic_cast<const CylinderBounds*>(&surfaceBounds); - if (radialBounds != nullptr) { + switch(surface->type()) { + case Surface::Disc:CMakeLists.txt (1)
174-177: Dependency alignment between ROOT and TGeo, prudent it is!
Usingset_option_ifto tieACTS_BUILD_PLUGIN_ROOTtoACTS_BUILD_PLUGIN_TGEOensures that your configuration flows smoothly. Verify that this dependency meets your design intentions, you should.Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackWriter.hpp (1)
56-65: Document configuration options better, you should.Clear documentation for configuration options, missing it is. Help future Padawans understand the code better, it would.
Add documentation for each configuration option:
struct Config : public Acts::RootMaterialTrack::Config { + /// @brief Collection of material tracks to write + /// @note Used as input for the writer std::string inputMaterialTracks = "material_tracks"; + /// @brief Path where the output file will be written std::string filePath = ""; + /// @brief Mode for opening the file (e.g., "RECREATE", "UPDATE") std::string fileMode = "RECREATE"; + /// @brief Name of the tree within the ROOT file std::string treeName = "material_tracks"; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CI/codespell_ignore.txt(1 hunks)CMakeLists.txt(3 hunks)CMakePresets.json(1 hunks)Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackReader.hpp(3 hunks)Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackWriter.hpp(4 hunks)Examples/Io/Root/src/RootMaterialTrackWriter.cpp(3 hunks)Plugins/CMakeLists.txt(1 hunks)Plugins/Root/CMakeLists.txt(1 hunks)Plugins/Root/include/Acts/Plugins/Root/RootMaterialTrack.hpp(1 hunks)Plugins/Root/src/RootMaterialTrack.cpp(1 hunks)docs/getting_started.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Plugins/CMakeLists.txt
- Plugins/Root/CMakeLists.txt
- Plugins/Root/include/Acts/Plugins/Root/RootMaterialTrack.hpp
👮 Files not reviewed due to content moderation or server errors (4)
- Examples/Io/Root/src/RootMaterialTrackWriter.cpp
- Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackReader.hpp
- Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackWriter.hpp
- Plugins/Root/src/RootMaterialTrack.cpp
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: macos
- GitHub Check: build_debug
🔇 Additional comments (15)
Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackReader.hpp (3)
41-48: Well structured, this code is.Clean inheritance from RootMaterialTrack::Config and clear configuration parameters, I see. Good practices, followed they are.
41-48: Wise changes in naming convention, these are.From hyphen to underscore, the names have changed. Consistency with modern C++ practices, this brings.
41-48: Well structured the code is, hmmmm.Thread-safe and memory-safe, this implementation is. Good practices it follows.
Examples/Io/Root/include/ActsExamples/Io/Root/RootMaterialTrackWriter.hpp (2)
56-65: Consistent with reader implementation, this is.Good symmetry between reader and writer implementations, I observe. Clear configuration structure, maintained it is.
56-65: Symmetry with Reader changes, maintained it is.Mirror the Reader's changes, this does. Consistency across codebase, it brings.
CI/codespell_ignore.txt (1)
22-22: Addition of 'mata', wise decision it is!
The new word "mata" now ignored by codespell, ensuring false alerts vanish from your path. Approved, this change is.CMakePresets.json (1)
69-69: New cache variable for the ROOT plugin, you have enabled!
Setting "ACTS_BUILD_PLUGIN_ROOT" to "ON" in the ci-common preset supports the new functionality well. Consistency with the broader configuration, it maintains.CMakeLists.txt (4)
89-89: New ROOT plugin option introduced, it is!
Declaring "ACTS_BUILD_PLUGIN_ROOT" with a default of OFF wisely allows toggling the new plugin functionality. Trust in this configuration, you may.
178-181: Linking ROOT plugin with Analysis Apps, smart the dependency chain is!
The clause makingACTS_BUILD_PLUGIN_ROOTdepend onACTS_BUILD_ANALYSIS_APPSis a logical move. Confirm that this relation reflects the intended behavior in your project design, you must.
417-424: Updated find_package for the ROOT plugin, correct it appears!
By requiring the ROOT componentsRIOandROOTDataFrame, this block now supports the new backend effectively. Ensure that your tests confirm compatibility with the designated ROOT version, you will.
427-436: Legacy TGeo plugin block remains, it does.
While the new ROOT plugin uses updated components, the TGeo block still usesGeomandGraf. Advise users to choose the proper plugin flag, for clarity in configuration, you must.docs/getting_started.md (1)
289-289: Documentation of the new ROOT plugin option, clear it is!
Adding theACTS_BUILD_PLUGIN_ROOTentry to the build options table enlightens users about this new feature nicely. Approved this documentation update, I have.Examples/Io/Root/src/RootMaterialTrackWriter.cpp (2)
75-93: Well implemented, the writeT method is.Thread safety with mutex, it maintains. Material track data, correctly it fills. Good practices, I observe.
66-73: Close all resources in finalize(), you must.In finalize() method, RNTuple resources not properly closed they are. Memory leaks this may cause.
Plugins/Root/src/RootMaterialTrack.cpp (1)
246-280: Wise memory management, implemented it is.Reserve vectors before filling, good practice this is. Memory allocations reduced, performance improved.
| m_outputFile->Close(); | ||
| } | ||
| } | ||
| RootMaterialTrackWriter::~RootMaterialTrackWriter() {} |
There was a problem hiding this comment.
Handle resource cleanup in destructor, you must.
Raw pointers m_outputFile and m_outputTree, managed properly they must be. Memory leaks, prevent we should.
Apply this diff to ensure proper cleanup:
-RootMaterialTrackWriter::~RootMaterialTrackWriter() {}
+RootMaterialTrackWriter::~RootMaterialTrackWriter() {
+ if (m_outputFile) {
+ m_outputFile->Close();
+ m_outputFile = nullptr;
+ }
+ m_outputTree = nullptr; // TTree owned by TFile, delete not needed
+}📝 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.
| RootMaterialTrackWriter::~RootMaterialTrackWriter() {} | |
| RootMaterialTrackWriter::~RootMaterialTrackWriter() { | |
| if (m_outputFile) { | |
| m_outputFile->Close(); | |
| m_outputFile = nullptr; | |
| } | |
| m_outputTree = nullptr; // TTree owned by TFile, delete not needed | |
| } |
Handle cleanup in destructor, you must.
Empty destructor, resource leaks it may cause. Clean up resources properly in destructor, you should.
Apply this diff to ensure proper cleanup:
-RootMaterialTrackWriter::~RootMaterialTrackWriter() {}
+RootMaterialTrackWriter::~RootMaterialTrackWriter() {
+ if (m_outputFile != nullptr) {
+ m_outputFile->Close();
+ m_outputFile = nullptr;
+ }
+}📝 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.
| RootMaterialTrackWriter::~RootMaterialTrackWriter() {} | |
| RootMaterialTrackWriter::~RootMaterialTrackWriter() { | |
| if (m_outputFile != nullptr) { | |
| m_outputFile->Close(); | |
| m_outputFile = nullptr; | |
| } | |
| } |
|
|
||
| // Record pre/post step | ||
| if (m_cfg.prePostStep) { | ||
| Vector3 prePos = mint.position - 0.5 * mint.pathCorrection * direction; | ||
| Vector3 posPos = mint.position + 0.5 * mint.pathCorrection * direction; | ||
|
|
||
| m_payload.stepXs->emplace_back(prePos.x()); | ||
| m_payload.stepYs->emplace_back(prePos.y()); | ||
| m_payload.stepZs->emplace_back(prePos.z()); | ||
| m_payload.stepXe->emplace_back(posPos.x()); | ||
| m_payload.stepYe->emplace_back(posPos.y()); | ||
| m_payload.stepZe->emplace_back(posPos.z()); | ||
| } | ||
|
|
||
| // Store surface information | ||
| if (m_cfg.storeSurface) { | ||
| const Surface* surface = mint.surface; | ||
| if (mint.intersectionID.value() != 0) { | ||
| m_payload.surfaceId->emplace_back(mint.intersectionID.value()); | ||
| m_payload.surfacePathCorrection->emplace_back(mint.pathCorrection); | ||
| m_payload.surfaceX->emplace_back(mint.intersection.x()); | ||
| m_payload.surfaceY->emplace_back(mint.intersection.y()); | ||
| m_payload.surfaceZ->emplace_back(mint.intersection.z()); | ||
| m_payload.surfaceR->emplace_back( | ||
| VectorHelpers::perp(mint.intersection)); | ||
| m_payload.surfaceDistance->emplace_back( | ||
| (mint.position - mint.intersection).norm()); | ||
| } else if (surface != nullptr) { | ||
| auto sfIntersection = | ||
| surface | ||
| ->intersect(gctx, mint.position, mint.direction, | ||
| BoundaryTolerance::None()) | ||
| .closest(); | ||
| m_payload.surfaceId->emplace_back(surface->geometryId().value()); | ||
| m_payload.surfacePathCorrection->emplace_back(1.0); | ||
| m_payload.surfaceX->emplace_back(sfIntersection.position().x()); | ||
| m_payload.surfaceY->emplace_back(sfIntersection.position().y()); | ||
| m_payload.surfaceZ->emplace_back(sfIntersection.position().z()); | ||
| } else { | ||
| m_payload.surfaceId->emplace_back(GeometryIdentifier().value()); | ||
| m_payload.surfaceX->emplace_back(0); | ||
| m_payload.surfaceY->emplace_back(0); | ||
| m_payload.surfaceZ->emplace_back(0); | ||
| m_payload.surfacePathCorrection->emplace_back(1.0); | ||
| } | ||
| if (surface != nullptr) { | ||
| m_payload.surfaceType->emplace_back(surface->type()); | ||
| const SurfaceBounds& surfaceBounds = surface->bounds(); | ||
| auto radialBounds = dynamic_cast<const RadialBounds*>(&surfaceBounds); | ||
| auto cylinderBounds = | ||
| dynamic_cast<const CylinderBounds*>(&surfaceBounds); | ||
| if (radialBounds != nullptr) { | ||
| m_payload.surfaceRangeMin->emplace_back(radialBounds->rMin()); | ||
| m_payload.surfaceRangeMax->emplace_back(radialBounds->rMax()); | ||
| } else if (cylinderBounds != nullptr) { | ||
| m_payload.surfaceRangeMin->emplace_back( | ||
| -cylinderBounds->get(CylinderBounds::eHalfLengthZ)); | ||
| m_payload.surfaceRangeMax->emplace_back( | ||
| cylinderBounds->get(CylinderBounds::eHalfLengthZ)); | ||
| } else { | ||
| m_payload.surfaceRangeMin->emplace_back(0); | ||
| m_payload.surfaceRangeMax->emplace_back(0); | ||
| } | ||
| } else { | ||
| m_payload.surfaceType->emplace_back(-1); | ||
| m_payload.surfaceRangeMin->emplace_back(0); | ||
| m_payload.surfaceRangeMax->emplace_back(0); | ||
| } | ||
| } | ||
|
|
||
| // store volume information | ||
| if (m_cfg.storeVolume) { | ||
| GeometryIdentifier vlayerID; | ||
| if (!mint.volume.empty()) { | ||
| vlayerID = mint.volume.geometryId(); | ||
| m_payload.volumeId->emplace_back(vlayerID.value()); | ||
| } else { | ||
| vlayerID.setVolume(0); | ||
| vlayerID.setBoundary(0); | ||
| vlayerID.setLayer(0); | ||
| vlayerID.setApproach(0); | ||
| vlayerID.setSensitive(0); | ||
| m_payload.volumeId->emplace_back(vlayerID.value()); | ||
| } | ||
| } | ||
|
|
||
| // the material information | ||
| const auto& mprops = mint.materialSlab; | ||
| m_payload.stepLength->emplace_back(mprops.thickness()); | ||
| m_payload.matX0->emplace_back(mprops.material().X0()); | ||
| m_payload.matL0->emplace_back(mprops.material().L0()); | ||
| m_payload.matA->emplace_back(mprops.material().Ar()); | ||
| m_payload.matZ->emplace_back(mprops.material().Z()); | ||
| m_payload.matRho->emplace_back(mprops.material().massDensity()); | ||
| // re-calculate if defined to do so | ||
| if (m_cfg.recalculateTotals) { | ||
| (*m_payload.tX0) += mprops.thicknessInX0(); | ||
| (*m_payload.tL0) += mprops.thicknessInL0(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Break down large fill method, you must.
Too many responsibilities the fill method has. Harder to maintain and understand it becomes. Split into smaller, focused methods you should.
Consider extracting these methods:
+void fillTrackInfo(const RecordedMaterialTrack& rmTrack, const Auxiliaries& aux);
+void fillMaterialProperties(const MaterialInteraction& mint);
+void fillSurfaceInfo(const MaterialInteraction& mint, const GeometryContext& gctx);
+void fillVolumeInfo(const MaterialInteraction& mint);Committable suggestion skipped: line range outside the PR's diff.
|
This PR makes the Root based recording of Geant4 material (as input to the material mapping) to
Coreand puts reading and writing onto the same internalpayload.Summary: The new
RNTuplebackend results in a file size reduction of 35% and a per event runtime reduction (G4 recording and writing) of 17.5%.Update: Unforunately I had to remove the RNTuple writing infrastructure for the moment as it is not yet stable thoughout our different ROOT versions, the code can be updated to it very quickly though.
Details of recording a million material tracks in the ODD are as follows:
(CURRENT MAIN) TTree backend:
(THIS PR) RNTuple backend:
(THIS PR) TTree backend:
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates
RootMaterialTrackclass for more efficient material track management.