feat: allow bare Surface pointers to be written to JSON #3493
Conversation
github-actions
bot
added
Component - Examples
asalzburger
force-pushed
the
feat-gen2-geomodel-json
branch
from
89fc123 to
6be3bb4
Compare
|
|
@asalzburger can you update the PR title to describe the change? What does it add JSON writing functionality to? |
| @@ -42,6 +42,11 @@ void Acts::to_json(nlohmann::json& j, const Acts::Surface& surface) { | |||
| j = SurfaceJsonConverter::toJson(gctx, surface); | |||
| } | |||
|
|
|||
| void Acts::to_json(nlohmann::json& j, const Acts::Surface* surface) { | |||
| Acts::GeometryContext gctx; | |||
| j = SurfaceJsonConverter::toJson(gctx, *surface); | |||
There was a problem hiding this comment.
This needs to check if the surface does not have an associated detector element, otherwise, this is invalid.
There was a problem hiding this comment.
Mmm, we have the same functionality for std::shared_ptr<Surface> - this just mirrors it for plain pointers, shall I change that then as well?
I guess we could throw and exception? That the default context is used is already pointed out in the comments.
This is needed because the GeometryIdHierarchyMap converter relies on the nlohmann::json bare to_json implementation.
There was a problem hiding this comment.
I would argue in that case, the conversion for shared pointers is also not safe.
Didn't we add a way to serialize the surface with an explicit context? The problem with a default constructed context is that it's up to the detector element how it's handled.
Sort of fundamentally, surfaces with detector elements can't really be serialized by writing out the transform, since it's only a snapshot. For the podio Track edm, I explicitly use the identifier of the surface if it has one, so it can be tied back to the detector element.
Otherwise, adding a check and an exception here might be good enough.
Sure, sorry - this was the last commit message - which didn't make much sense either. |
asalzburger
changed the title
WalkthroughA new function Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (6)
Examples/Scripts/Python/geomodel_geometry.py (3)
87-92: Mysterious, the ways of detray format are!Clarify the help text, you must. Know what "detray JSON format" means, young padawans may not. Add more description about this special format's purpose and structure, I suggest.
p.add_argument( "--output-json-detray", - help="Write the geometry in detray JSON format", + help="Write the geometry in detray JSON format (specialized format for detector ray tracing)", action="store_true", default=False, )
194-203: Duplicate code, a path to the dark side it is!Extract common SVG view configuration into a separate function, we should. Reduce code duplication, this will.
+def create_detector_view(gContext, detector, top_node, volume_options, views, output_name): + acts.svg.viewDetector( + gContext, + detector, + top_node, + [[ivol, volume_options] for ivol in range(detector.numberVolumes())], + views, + output_name, + ) # In main(): -acts.svg.viewDetector( - gContext, - detector, - args.top_node, - [[ivol, volumeOptions] for ivol in range(detector.numberVolumes())], - [["zr", ["materials"], zrRange]], - args.output + "_material_surfaces", -) +create_detector_view( + gContext, + detector, + args.top_node, + volumeOptions, + [["zr", ["materials"], zrRange]], + args.output + "_material_surfaces", +)
225-235: Hardcoded values, lead to inflexibility they do!Make the JSON indent configurable through command line arguments, we should. Fixed at 4 spaces, it should not be.
p.add_argument( "--output-json-sensitives-map", help="Write detector's sensitive surfaces hierarchy map to a dedicated JSON file", action="store_true", default=False, ) +p.add_argument( + "--json-indent", + type=int, + default=4, + help="Number of spaces for JSON indentation", +) # Later in the code: if args.output_json_sensitives_map: acts.examples.writeSensitivesMapToJson( - detector, args.output + "_sensitives.json", 4 + detector, args.output + "_sensitives.json", args.json_indent )Tests/UnitTests/Plugins/Json/SurfaceJsonConverterTests.cpp (2)
215-218: Handle file operations with care, you must.Missing error handling for file operations, I sense. Check file write success, a wise Jedi would.
- out.open("CylinderSurfacePlainPointer.json"); - out << cylinderOut.dump(2); - out.close(); + out.open("CylinderSurfacePlainPointer.json"); + BOOST_CHECK(out.good()); + out << cylinderOut.dump(2); + out.close();
206-233: Additional test scenarios, consider you should.Strong in the Force this test is, but stronger it could become. Edge cases and error scenarios, we must also explore.
Consider adding tests for:
- Null pointer handling
- Invalid surface types
- Malformed JSON input
- Memory management scenarios
Help with implementing these additional test cases, would you like?
Examples/Python/src/Json.cpp (1)
155-169: Refactor this function, we should. Cleaner code leads to inner peace.A suggestion for better structure and reusability, I have.
Consider this path to enlightenment:
+ mex.def( + "writeSensitivesMapToJson", + [](const Acts::Experimental::Detector& detector, + const std::string& fileName, std::size_t outputPrecision) -> void { + using SurfaceConverter = + Acts::GeometryHierarchyMapJsonConverter<const Acts::Surface*>; + + // Convert to JSON first + auto j = SurfaceConverter("surfaces") + .toJson(detector.sensitiveHierarchyMap(), nullptr); + + // Write to file with proper formatting + writeJsonToFile(fileName, j, outputPrecision); + }); + + // Helper function to write JSON to file + auto writeJsonToFile = [](const std::string& fileName, + const nlohmann::json& j, + std::size_t outputPrecision) { + std::ofstream out; + out.exceptions(std::ofstream::failbit | std::ofstream::badbit); + try { + out.open(fileName); + out << std::setprecision(outputPrecision) << j.dump(2); + out.close(); + } catch (const std::exception& e) { + throw std::runtime_error("Failed to write JSON file '" + fileName + + "': " + e.what()); + } + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Examples/Python/src/Json.cpp(1 hunks)Examples/Scripts/Python/geomodel_geometry.py(3 hunks)Plugins/Json/include/Acts/Plugins/Json/SurfaceJsonConverter.hpp(1 hunks)Plugins/Json/src/SurfaceJsonConverter.cpp(1 hunks)Tests/UnitTests/Plugins/Json/SurfaceJsonConverterTests.cpp(1 hunks)
🔇 Additional comments (6)
Plugins/Json/include/Acts/Plugins/Json/SurfaceJsonConverter.hpp (2)
49-52: Fulfill the prophecy, this change does!
Align with the PR objectives to enable bare Surface pointer JSON writing, this addition does. A simple yet effective solution, it is.
49-52: Consistent with the Force, this addition is.
Match the existing patterns of conversion, this overload does. Documentation style and function signature, follow the established way they do.
✅ Verification successful
Harmony in the code, I sense. Verified this implementation is.
Consistent patterns across all Surface to_json overloads, there are. Each implementation, the same GeometryContext approach uses, and to the common SurfaceJsonConverter::toJson they defer. A balanced trinity of overloads - reference, pointer, and shared_ptr - working in harmony they are.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage patterns of the new overload
rg "to_json.*Surface.*" --type cpp
Length of output: 1044
Script:
#!/bin/bash
# Check implementation details of Surface* overload
ast-grep --pattern 'void to_json(nlohmann::json& j, const Surface* surface) {
$$$
}'
# Check implementation details of other Surface overloads
ast-grep --pattern 'void to_json(nlohmann::json& j, const Surface& surface) {
$$$
}'
ast-grep --pattern 'void to_json(nlohmann::json& j, const std::shared_ptr<const Surface>& surface) {
$$$
}'
Length of output: 289
Script:
#!/bin/bash
# Search for implementation files with context
rg "void Acts::to_json.*Surface.*" -A 10 --type cpp Plugins/Json/src/
Length of output: 2125
Plugins/Json/src/SurfaceJsonConverter.cpp (1)
45-48:
Validate pointer and add safety checks, young padawan must!
Concerns from the Force, I sense:
- Null pointer check, missing it is
- Detector element validation, needed it may be
- Default GeometryContext, dangerous it can be for surfaces with detector elements
Apply these changes, you should:
void Acts::to_json(nlohmann::json& j, const Acts::Surface* surface) {
+ if (surface == nullptr) {
+ throw std::invalid_argument("Null surface pointer provided");
+ }
+ if (surface->associatedDetectorElement() != nullptr) {
+ throw std::runtime_error("Cannot serialize surface with detector element using default context");
+ }
Acts::GeometryContext gctx;
j = SurfaceJsonConverter::toJson(gctx, *surface);
}Run this script to check for potential unsafe usage:
#!/bin/bash
# Search for direct calls to this overload that might need safety checks
rg "to_json.*Surface\*" --type cpp
# Look for surfaces with detector elements that might be affected
ast-grep --pattern 'surface->associatedDetectorElement()'Past wisdom from Master paulgessinger, this mirrors: "This needs to check if the surface does not have an associated detector element, otherwise, this is invalid."
Examples/Scripts/Python/geomodel_geometry.py (2)
82-85: Hmmmm, more accurate this description is!
Changed from OBJ files to JSON files, the help text has. Clarity and truth in documentation, bring this does.
94-98: Clear as mud, this help text is!
Explain the purpose of sensitives map and its structure, we must. Documentation for this feature, essential it is.
p.add_argument(
"--output-json-sensitives-map",
- help="Write the surfaces to a dedicated JSON file",
+ help="Write detector's sensitive surfaces hierarchy map to a dedicated JSON file",
action="store_true",
default=False,
)Examples/Python/src/Json.cpp (1)
155-169: Verify the impact on the codebase, we must.
Hmmmm. Changes to detector's sensitive hierarchy map serialization, I see. Check usage in tests and examples, we should.
✅ Verification successful
Safe, these changes are. Proceed, we may.
Examined the usage, I have. Only in example script geomodel_geometry.py, this function is used, hmm. Optional feature for JSON export of sensitives map, it is. No impact on core functionality or tests, there shall be. Local to Python bindings and example scripts, these changes remain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential test cases and example usage
echo "=== Searching for test files ==="
fd -e cpp -e py test
echo "=== Searching for example usage ==="
rg "writeSensitivesMapToJson"
Length of output: 20372
Script:
#!/bin/bash
# Check the content of the file that uses writeSensitivesMapToJson
echo "=== Checking geomodel_geometry.py usage ==="
cat Examples/Scripts/Python/geomodel_geometry.py | grep -A 5 -B 5 writeSensitivesMapToJson
# Check if there are any tests for this functionality
echo "=== Checking Python tests for Json functionality ==="
rg -A 5 -B 5 "writeSensitivesMapToJson" Examples/Python/tests/
Length of output: 701
| /// Non-contextual conversion of a surface | ||
| /// | ||
| /// @note it will take the default context | ||
| void to_json(nlohmann::json& j, const Surface* surface); |
There was a problem hiding this comment.
💡 Codebase verification
Unsafe, this implementation is! No null check, I see!
Found the implementation in SurfaceJsonConverter.cpp, I have. Dereference the raw pointer without null check, it does:
void Acts::to_json(nlohmann::json& j, const Acts::Surface* surface) {
Acts::GeometryContext gctx;
j = SurfaceJsonConverter::toJson(gctx, *surface); // Unsafe dereference!
}Fix this, you must:
- Add nullptr check before dereferencing, you should
- Document the nullptr behavior in the header, essential it is
- Consider using std::shared_ptr instead, safer it would be, hmmmm
Already a std::shared_ptr overload exists, unnecessary this raw pointer version may be.
🔗 Analysis chain
Hmmmm, careful with raw pointers, we must be!
A raw pointer conversion function, you have added. But handle null pointers safely, we must. Verify the implementation, I shall.
Consider these points of wisdom, you should:
- Prefer smart pointers over raw pointers, as safer they are
- Document nullptr behavior explicitly, you must
- Consistent with other overloads, stay you should
The first attempt failed because the implementation might be in a cpp file or have a different pattern. Let me try these alternative approaches to find and verify the null pointer handling in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation to verify null pointer handling
ast-grep --pattern 'void Acts::to_json(nlohmann::json& j, const Acts::Surface* surface)'
Length of output: 90
Script:
#!/bin/bash
# Search for implementation in cpp files
rg -l "void\s+to_json.*Surface\*\s*surface" --type cpp
# Look for the implementation pattern without namespace
ast-grep --pattern 'to_json($j, $surface) {
$$$
}'
# Check existing implementations for comparison
rg "to_json.*Surface" -A 5
Length of output: 4129
| Transform3 trf(Transform3::Identity() * Translation3(0., 0., -7.)); | ||
| auto tube = std::make_shared<CylinderBounds>(5., 20.); | ||
| auto cylinderRef = Surface::makeShared<CylinderSurface>(trf, tube); | ||
| cylinderRef->assignGeometryId(GeometryIdentifier(11u)); |
There was a problem hiding this comment.
Duplicate GeometryId detected, hmmmm.
Same GeometryId (11u) as CylinderSurfaceRoundTripTests, this has. Confusion in test results, it may cause. Unique identifier, you must assign.
- cylinderRef->assignGeometryId(GeometryIdentifier(11u));
+ cylinderRef->assignGeometryId(GeometryIdentifier(14u));📝 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.
| cylinderRef->assignGeometryId(GeometryIdentifier(11u)); | |
| cylinderRef->assignGeometryId(GeometryIdentifier(14u)); |
| auto cylinderTest = SurfaceJsonConverter::fromJson(cylinderIn); | ||
|
|
||
| BOOST_CHECK( | ||
| cylinderTest->transform(gctx).isApprox(cylinderRef->transform(gctx))); | ||
| BOOST_CHECK_EQUAL(cylinderTest->geometryId(), cylinderRef->geometryId()); | ||
| BOOST_CHECK_EQUAL(cylinderTest->bounds(), cylinderRef->bounds()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Additional verification for pointer behavior, suggest I do.
Well-structured the basic tests are, but pointer-specific scenarios, we must also verify. Type information preservation and proper pointer handling, essential they are.
auto cylinderTest = SurfaceJsonConverter::fromJson(cylinderIn);
BOOST_CHECK(
cylinderTest->transform(gctx).isApprox(cylinderRef->transform(gctx)));
BOOST_CHECK_EQUAL(cylinderTest->geometryId(), cylinderRef->geometryId());
BOOST_CHECK_EQUAL(cylinderTest->bounds(), cylinderRef->bounds());
+ // Verify type information is preserved
+ BOOST_CHECK(dynamic_cast<const CylinderSurface*>(cylinderTest.get()) != nullptr);
+ // Verify the surface type matches
+ BOOST_CHECK_EQUAL(cylinderTest->type(), Surface::Type::Cylinder);📝 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.
| auto cylinderTest = SurfaceJsonConverter::fromJson(cylinderIn); | |
| BOOST_CHECK( | |
| cylinderTest->transform(gctx).isApprox(cylinderRef->transform(gctx))); | |
| BOOST_CHECK_EQUAL(cylinderTest->geometryId(), cylinderRef->geometryId()); | |
| BOOST_CHECK_EQUAL(cylinderTest->bounds(), cylinderRef->bounds()); | |
| auto cylinderTest = SurfaceJsonConverter::fromJson(cylinderIn); | |
| BOOST_CHECK( | |
| cylinderTest->transform(gctx).isApprox(cylinderRef->transform(gctx))); | |
| BOOST_CHECK_EQUAL(cylinderTest->geometryId(), cylinderRef->geometryId()); | |
| BOOST_CHECK_EQUAL(cylinderTest->bounds(), cylinderRef->bounds()); | |
| // Verify type information is preserved | |
| BOOST_CHECK(dynamic_cast<const CylinderSurface*>(cylinderTest.get()) != nullptr); | |
| // Verify the surface type matches | |
| BOOST_CHECK_EQUAL(cylinderTest->type(), Surface::Type::Cylinder); |
| { | ||
| mex.def( | ||
| "writeSensitivesMapToJson", | ||
| [](const Acts::Experimental::Detector& detector, | ||
| const std::string& fileName, std::size_t outputPrecision) -> void { | ||
| using SurfaceConverter = | ||
| Acts::GeometryHierarchyMapJsonConverter<const Acts::Surface*>; | ||
| std::ofstream out; | ||
| out.open(fileName); | ||
| auto j = SurfaceConverter("surfaces") | ||
| .toJson(detector.sensitiveHierarchyMap(), nullptr); | ||
| out << std::setprecision(outputPrecision) << j.dump(2); | ||
| out.close(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Handle file operations with care, young Padawan must.
Hmmmm. Missing error handling for file operations, I sense. A disturbance in the Force, this could cause.
Apply this diff to strengthen your code with the Force of error handling:
mex.def(
"writeSensitivesMapToJson",
[](const Acts::Experimental::Detector& detector,
const std::string& fileName, std::size_t outputPrecision) -> void {
using SurfaceConverter =
Acts::GeometryHierarchyMapJsonConverter<const Acts::Surface*>;
std::ofstream out;
+ out.exceptions(std::ofstream::failbit | std::ofstream::badbit);
+ try {
out.open(fileName);
auto j = SurfaceConverter("surfaces")
.toJson(detector.sensitiveHierarchyMap(), nullptr);
out << std::setprecision(outputPrecision) << j.dump(2);
out.close();
+ } catch (const std::exception& e) {
+ throw std::runtime_error("Failed to write JSON file '" + fileName +
+ "': " + e.what());
+ }
});📝 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.
| { | |
| mex.def( | |
| "writeSensitivesMapToJson", | |
| [](const Acts::Experimental::Detector& detector, | |
| const std::string& fileName, std::size_t outputPrecision) -> void { | |
| using SurfaceConverter = | |
| Acts::GeometryHierarchyMapJsonConverter<const Acts::Surface*>; | |
| std::ofstream out; | |
| out.open(fileName); | |
| auto j = SurfaceConverter("surfaces") | |
| .toJson(detector.sensitiveHierarchyMap(), nullptr); | |
| out << std::setprecision(outputPrecision) << j.dump(2); | |
| out.close(); | |
| }); | |
| } | |
| { | |
| mex.def( | |
| "writeSensitivesMapToJson", | |
| [](const Acts::Experimental::Detector& detector, | |
| const std::string& fileName, std::size_t outputPrecision) -> void { | |
| using SurfaceConverter = | |
| Acts::GeometryHierarchyMapJsonConverter<const Acts::Surface*>; | |
| std::ofstream out; | |
| out.exceptions(std::ofstream::failbit | std::ofstream::badbit); | |
| try { | |
| out.open(fileName); | |
| auto j = SurfaceConverter("surfaces") | |
| .toJson(detector.sensitiveHierarchyMap(), nullptr); | |
| out << std::setprecision(outputPrecision) << j.dump(2); | |
| out.close(); | |
| } catch (const std::exception& e) { | |
| throw std::runtime_error("Failed to write JSON file '" + fileName + | |
| "': " + e.what()); | |
| } | |
| }); | |
| } |
This PR rennames
geomodel.pytogeometry_geomodel.pyand adds the necessaryjsonupdate to write out the sensitive surfaces as a hierarchy map.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Surfaceobjects.Tests
CylinderSurfaceobjects using plain pointer references.