feat: Add JSON detector element #3851
Conversation
WalkthroughSignificant, the changes are. Modified, the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 8
🧹 Outside diff range and nitpick comments (10)
Plugins/Json/include/Acts/Plugins/Json/JsonDetectorElement.hpp (2)
16-21: Enhance the documentation with examples, we should.Good documentation, you have written. But stronger with examples, it would be. Add a code block showing the expected JSON structure, you should. Help future Padawans understand the way, this will.
Add this to the documentation:
/// importing whole tracking geometries from JSON files. In some parts of /// the codebase, the existence of a detector element associated to a surface /// has a specific meaning (e.g., flags surfaces as sensitive). +/// +/// @code{.json} +/// { +/// "surface": { +/// "transform": [...], // transformation matrix +/// "bounds": {...} // surface bounds +/// } +/// } +/// @endcode
22-30: Document the public interface, we must.Missing, the documentation for public methods is. Help others understand the way, proper documentation does.
Add documentation like this:
public: + /// Constructs a detector element from JSON surface description + /// @param jSurface The JSON object containing surface parameters + /// @param thickness The thickness of the detector element JsonDetectorElement(const nlohmann::json &jSurface, double thickness); + /// @copydoc DetectorElementBase::surface Surface &surface() override; const Surface &surface() const override; + /// @copydoc DetectorElementBase::thickness double thickness() const override; + /// @copydoc DetectorElementBase::transform const Transform3 &transform(const GeometryContext &gctx) const override;Plugins/Json/CMakeLists.txt (1)
Line range hint
1-48: Strong with the Force, this architecture is!Wise decision it was, to move JsonSurfacesReader to core plugin library. Together, JSON components now reside, promoting clarity and maintainability. Clear separation of concerns, achieved it is.
Plugins/Json/include/Acts/Plugins/Json/JsonSurfacesReader.hpp (4)
22-22: Moved to Acts namespace, the JsonSurfacesReader has.A significant change in code organization, this is. From ActsExamples to core Acts namespace, the migration shows maturity of the feature.
Verify all dependent code has been updated, you must:
#!/bin/bash # Search for any remaining references to the old namespace rg "ActsExamples::JsonSurfacesReader"
25-26: Documentation clarity achieved, it has.Clear explanation of file format expectations, provided now it is. Yet, the previous review comment by asalzburger about GeometryHierarchy map, unaddressed it remains.
Add more specific documentation about the output format, you should:
/// @brief Options specification for surface reading -/// The file should contain an array of json surfaces -/// as produced by the SurfaceJsonConverter tools +/// The file should contain an array of json surfaces +/// as produced by the SurfaceJsonConverter tools. +/// The output can be either a flat vector of surfaces +/// or a GeometryHierarchyMap, depending on the reader +/// method used.
51-60: New power to create detector elements, gained we have.Strong in the Force, this new method is. But validation of thickness parameter, missing it appears.
Add parameter validation, you should:
std::vector<std::shared_ptr<Acts::JsonDetectorElement>> readDetectorElements( - const Options& options, double thickness); + const Options& options, double thickness) { + if (thickness <= 0) { + throw std::invalid_argument( + "Detector element thickness must be positive"); + } + // ... implementation +}
Line range hint
35-48: Parameter documentation improved, it has.Clear and consistent with the Options struct, the changes are. But a small enhancement, suggest I do.
Add return value documentation, you should:
/// @param options specifies which file and what to read /// -/// @return a vector of surfaces +/// @return A vector of shared pointers to Surface objects, +/// each representing a surface read from the input filePlugins/Json/src/JsonSurfacesReader.cpp (2)
77-78: In the implementation file, default arguments should not reside.Move the default argument to the header file, young padawan.
-JsonSurfacesReader::readDetectorElements(const Options& options, - double thickness = 0.0) { +JsonSurfacesReader::readDetectorElements(const Options& options, + double thickness) {
85-89: Duplicate code pattern, I observe.Similar JSON traversal logic appears in three methods. Extract to private helper function, we should.
+private: + nlohmann::json traverseToSurfaces(const nlohmann::json& j, + const std::vector<std::string>& path) { + nlohmann::json current = j; + for (const auto& jep : path) { + current = current[jep]; + } + return current; + }Examples/Python/src/Json.cpp (1)
177-178: Relocate this functionality to the plugin, we should!In examples directory, this code should not dwell. To the plugin itself, move these bindings we must. Better architecture and cleaner separation of concerns, this will bring.
Consider moving these Python bindings to the JSON plugin module, as they are not example-specific functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
Examples/Io/Json/CMakeLists.txt(0 hunks)Examples/Python/src/Detector.cpp(1 hunks)Examples/Python/src/Json.cpp(2 hunks)Plugins/Json/CMakeLists.txt(1 hunks)Plugins/Json/include/Acts/Plugins/Json/JsonDetectorElement.hpp(1 hunks)Plugins/Json/include/Acts/Plugins/Json/JsonSurfacesReader.hpp(4 hunks)Plugins/Json/src/JsonDetectorElement.cpp(1 hunks)Plugins/Json/src/JsonSurfacesReader.cpp(3 hunks)Tests/UnitTests/Plugins/Json/CMakeLists.txt(1 hunks)Tests/UnitTests/Plugins/Json/JsonSurfacesReaderTests.cpp(1 hunks)
💤 Files with no reviewable changes (1)
- Examples/Io/Json/CMakeLists.txt
🔇 Additional comments (17)
Tests/UnitTests/Plugins/Json/CMakeLists.txt (2)
19-19: Hmmmm, good this addition is!
Aligned with the Force, this new test for JsonSurfacesReader is. Follows the established pattern of test declarations, it does.
19-19: Verify the test file and dependencies, we must.
Important it is, to ensure existence of test file and proper linkage with ActsPluginJson library.
✅ Verification successful
Verified and proper, everything is. Proceed with confidence, we shall.
In balance, all components are. The test file exists in its proper place, includes the necessary headers it does, and proper test cases it contains. The Force reveals:
- Present and correct, the JsonSurfacesReader implementation files are
- Properly included, the JsonSurfacesReader header is
- Configured correctly in CMake, the test target is
- Boost test framework, properly integrated it is
- Two test cases, written and ready they are
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test file existence and dependencies
# Check if the test file exists
echo "Searching for test file..."
fd JsonSurfacesReaderTests.cpp
# Check for JsonSurfacesReader related files
echo "Searching for related implementation files..."
fd JsonSurfacesReader
# Check if the test includes the necessary headers
echo "Checking test file dependencies..."
if [ -f Tests/UnitTests/Plugins/Json/JsonSurfacesReaderTests.cpp ]; then
rg "include.*JsonSurfacesReader" Tests/UnitTests/Plugins/Json/JsonSurfacesReaderTests.cpp
fi
Length of output: 729
Script:
#!/bin/bash
# Verify CMake configuration and dependencies
# Check CMake configuration for JsonSurfacesReader test
echo "Checking CMake configuration for test dependencies..."
rg "add_unittest.*JsonSurfacesReader" -A 5 Tests/UnitTests/Plugins/Json/CMakeLists.txt
# Check if the test file includes necessary testing framework headers
echo "Checking test framework includes..."
rg "include.*test" Tests/UnitTests/Plugins/Json/JsonSurfacesReaderTests.cpp
# Check for target linking in CMake
echo "Checking target linking..."
rg "target_link_libraries.*Json" Tests/UnitTests/Plugins/Json/CMakeLists.txt
# Examine test content to ensure it's properly structured
echo "Examining test structure..."
rg "TEST|BOOST_AUTO_TEST" Tests/UnitTests/Plugins/Json/JsonSurfacesReaderTests.cpp
Length of output: 893
Plugins/Json/src/JsonDetectorElement.cpp (2)
37-39: Simple and clear, this method is. Approve it, I do.
Clean implementation of thickness accessor, this is.
32-35:
Ignored, the GeometryContext is. Disturbing, this is.
Strong opinions in previous reviews, I sense. Default-constructed GeometryContext, forbidden it is in core code. Two paths forward, I see:
- Use the GeometryContext properly:
const Transform3 &JsonDetectorElement::transform(
- const GeometryContext & /*gctx*/) const {
+ const GeometryContext &gctx) const {
+ // Apply any context-dependent transformations here
return m_transform;
}- Or document why GeometryContext unused it is:
const Transform3 &JsonDetectorElement::transform(
+ // GeometryContext unused as transform is fixed at construction
const GeometryContext & /*gctx*/) const {
return m_transform;
}Choose wisely, you must.
✅ Verification successful
Document the GeometryContext's non-use, you must
Hmm, searched through the Force, I have. Pure virtual method from DetectorElementBase, this transform() is. When unused parameters we have, document the reason we should. Choice 2 from original review, the right path it is:
const Transform3 &JsonDetectorElement::transform(
+ // GeometryContext unused as transform is fixed at construction time and does not depend on alignment conditions
const GeometryContext & /*gctx*/) const {
return m_transform;
}Wisdom in documenting design decisions, there is. Future Padawans, understand the code better, they will.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other implementations of transform() method
ast-grep --pattern 'transform(const GeometryContext &$_) const { $$$}'
Length of output: 72
Script:
#!/bin/bash
# Search for transform method implementations with ripgrep for better context
rg "transform.*GeometryContext.*const" -A 3
# Also search for base class or interface that might define this method
rg "class.*DetectorElement" -A 5
Length of output: 37837
Script:
#!/bin/bash
# Check if there are any guidelines or documentation about GeometryContext usage
fd "README|CONTRIBUTING" --type f --exec grep -l "GeometryContext"
# Search for any comments or documentation about GeometryContext usage
rg "GeometryContext" --type cpp --type hpp -C 2
Length of output: 142
Script:
#!/bin/bash
# Look for similar detector element implementations
fd "DetectorElement" --type f --extension cpp --extension hpp --exec grep -l "transform.*GeometryContext"
Length of output: 507
Script:
#!/bin/bash
# Search for base class implementation to understand the contract
rg -A 10 "virtual.*transform.*GeometryContext.*const.*=.*0" Core/include/Acts/Geometry/DetectorElementBase.hpp
# Look at the implementation of transform in other detector elements
ast-grep --pattern 'transform(const GeometryContext &$_) const { $$$}' Plugins/TGeo/src/TGeoDetectorElement.cpp Plugins/Geant4/src/Geant4DetectorElement.cpp Plugins/GeoModel/src/GeoModelDetectorElement.cpp
Length of output: 717
Plugins/Json/include/Acts/Plugins/Json/JsonDetectorElement.hpp (3)
1-13: Correct, the header organization is, hmmmm.
Clean and minimal, the includes are. Present and proper, the license header is.
32-36: Wise placement of private members, this is.
Follow the ACTS convention of private members at bottom, you do. Properly initialized, the members are. Strong with the Force, this implementation is.
21-21: Verify similar implementations in the codebase, we must.
Ensure consistency with other detector element implementations, we should.
✅ Verification successful
Consistent with other detector elements, this implementation is.
Examined the codebase thoroughly, I have. Similar patterns in other detector element implementations, I observe:
- Like TGeoDetectorElement, DD4hepDetectorElement, and others, JsonDetectorElement properly inherits from DetectorElementBase
- Standard detector element interface it implements, with surface(), transform(), and thickness() methods
- Used in JsonSurfacesReader for JSON-based detector configuration, it is
No issues to report, there are. Follow the ways of the Force, this implementation does.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other detector element implementations
echo "=== Other detector element implementations ==="
ast-grep --pattern 'class $_ : public DetectorElementBase {
$$$
}'
# Search for similar JSON-based implementations
echo "=== Other JSON-based implementations ==="
rg "nlohmann::json" --type cpp
Length of output: 51483
Script:
#!/bin/bash
# Search for detector element implementations
echo "=== Detector element implementations ==="
rg "class.*DetectorElement" -A 5 --type cpp
# Search for JsonDetectorElement usage
echo "=== JsonDetectorElement usage ==="
rg "JsonDetectorElement" --type cpp -A 5
Length of output: 38385
Plugins/Json/CMakeLists.txt (2)
25-26: Approve these changes, I do!
Wise placement of new source files in ActsPluginJson library, it is. Architectural coherence maintained, as JSON-related functionality together stays.
Line range hint 34-37: Dependencies sufficient they are, but verify we must!
Correct dependencies present they are, with nlohmann_json already linked. Yet, verify additional requirements for new files, we should.
Run this script, we must:
✅ Verification successful
Sufficient dependencies they are, young padawan!
Through the Force, seen I have that nlohmann_json correctly linked it is. All source files, the necessary includes they have. No additional third-party dependencies required they are, as only Acts namespaces and nlohmann_json used they are.
In the JsonDetectorElement.hpp and JsonSurfacesReader.hpp, nlohmann_json properly included it is:
#include <nlohmann/json.hpp>The CMakeLists.txt, correct it remains:
target_link_libraries(
ActsPluginJson
PUBLIC ActsCore nlohmann_json::nlohmann_json
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for additional include dependencies in new source files
# Search for includes in new files
echo "=== Scanning JsonDetectorElement.cpp for includes ==="
rg -A 1 "^#include" src/JsonDetectorElement.cpp || echo "File not found in current path"
echo "=== Scanning JsonSurfacesReader.cpp for includes ==="
rg -A 1 "^#include" src/JsonSurfacesReader.cpp || echo "File not found in current path"
# Check if any new third-party dependencies are used
echo "=== Checking for potential new third-party dependencies ==="
fd -e cpp -e hpp . src/ --exec grep -l "namespace.*::" {} \; || echo "No files found"
Length of output: 933
Script:
#!/bin/bash
# Hmmmm... In the Plugin/Json directory, the source files must be
echo "=== Location of source files, we shall find ==="
fd -e cpp -e hpp . Plugins/Json/
echo "=== Includes in these files, we shall examine ==="
rg -A 1 "^#include" Plugins/Json/
echo "=== Third-party dependencies, we must detect ==="
rg "namespace.*::" Plugins/Json/
Length of output: 49579
Plugins/Json/include/Acts/Plugins/Json/JsonSurfacesReader.hpp (1)
12-12: Wise inclusion of JsonDetectorElement.hpp, it is.
Proper dependency management, this shows. Clear intention to use detector elements from JSON, I see.
Plugins/Json/src/JsonSurfacesReader.cpp (1)
Line range hint 9-20: Verify the namespace change impact, we must.
From ActsExamples to Acts, the namespace has moved. Impact on existing code, this change may have.
✅ Verification successful
Safe, the namespace change is. Proceed, we may.
In the Force, I sense no disturbance. The test files and Python bindings, properly updated they are. No references to ActsExamples::JsonSurfacesReader remain, and all includes point to the new path in the Acts namespace, they do. The change, contained and complete it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to ActsExamples::JsonSurfacesReader
rg "ActsExamples::JsonSurfacesReader"
# Search for includes of this file to verify they've been updated
rg -l "JsonSurfacesReader\.hpp"
Length of output: 197
Script:
#!/bin/bash
# Check the content of test file for any remaining ActsExamples references
cat Tests/UnitTests/Plugins/Json/JsonSurfacesReaderTests.cpp
# Check Python bindings for any remaining ActsExamples references
cat Examples/Python/src/Json.cpp
Length of output: 12196
Tests/UnitTests/Plugins/Json/JsonSurfacesReaderTests.cpp (2)
1-27: Proper organization of includes and license header, I sense.
Well-structured the includes are, grouped logically they are. License header, present and correct it is.
28-46: Careful with random test data, we must be.
Random transformations in tests, unpredictable results they may cause. Consider these improvements, you should:
- Use fixed, predetermined transformations for reliable test outcomes
- Add validation for generated surfaces
- Test edge cases with varying bounds sizes
Run this script to check for other random test data usage:
Examples/Python/src/Detector.cpp (1)
226-230: Correct, the basic binding structure is. Hmmmm.
A good foundation for the JsonDetectorElement, this binding provides. The use of std::shared_ptr, wise it is.
Examples/Python/src/Json.cpp (3)
14-14: Correct, the include placement is!
In harmony with other JSON headers, this addition stands. Well-ordered, the includes remain.
156-163: Wise, the namespace change is!
From ActsExamples to Acts namespace, moved the options have been. Clear and consistent with the C++ side, this change is.
170-175: Strong with the Force, this implementation is!
Correctly bound to Python, the JsonDetectorElement class is. Wise choice, returning shared_ptr for surface was, preventing Python side lifetime issues it does. Previous concerns, addressed they have been.
|
@benjaminhuth can you check the doc CI failure? |
Add a json detector element, which is explicitly constructed from a
nlohmann::jsonobject in order to ensure consistency.--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
Summary by CodeRabbit
JsonDetectorElementclass for creating detector elements from JSON descriptions.JsonSurfacesReader.JsonSurfacesReaderfor reading detector elements with specified thickness.JsonSurfacesReaderto ensure proper functionality and data integrity.