Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow bare Surface pointers to be written to JSON #3493

Closed
wants to merge 4 commits into from
Closed

feat: allow bare Surface pointers to be written to JSON #3493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5fcc70
adding json writing functionality
asalzburger Aug 7, 2024
d4e3748
Merge branch 'main' into feat-gen2-geomodel-json
asalzburger Aug 7, 2024
6be3bb4
add new test
asalzburger Aug 8, 2024
4007268
Merge branch 'main' into feat-gen2-geomodel-json
asalzburger Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions Examples/Python/src/Json.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,22 @@ void addJson(Context& ctx) {
}));
}

{
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();
});
}
Comment on lines +155 to +169
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
{
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());
}
});
}


{
auto sjOptions = py::class_<ActsExamples::JsonSurfacesReader::Options>(
mex, "SurfaceJsonOptions")
Expand Down
39 changes: 37 additions & 2 deletions Examples/Scripts/Python/geomodel.py → Examples/Scripts/Python/geomodel_geometry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,21 @@ def main():

p.add_argument(
"--output-json",
help="Write the surfaces to OBJ files",
help="Write the geometry to JSON files",
action="store_true",
default=False,
)

p.add_argument(
"--output-json-detray",
help="Write the geometry in detray JSON format",
action="store_true",
default=False,
)

p.add_argument(
"--output-json-sensitives-map",
help="Write the surfaces to a dedicated JSON file",
action="store_true",
default=False,
)
Expand Down Expand Up @@ -177,6 +191,17 @@ def main():
args.output + "_detector",
)

acts.svg.viewDetector(
gContext,
detector,
args.top_node,
[[ivol, volumeOptions] for ivol in range(detector.numberVolumes())],
[
["zr", ["materials"], zrRange],
],
args.output + "_material_surfaces",
)

# Output the internal navigation to SVG
if args.output_internals_svg:
for vol in detector.volumes():
Expand All @@ -197,7 +222,17 @@ def main():
)
# Output to a JSON file
if args.output_json:
acts.examples.writeDetectorToJsonDetray(gContext, detector, args.output)
# Gen2 detector json format
if args.output_json_detray:
acts.examples.writeDetectorToJsonDetray(gContext, detector, args.output)
else:
acts.examples.writeDetectorToJson(gContext, detector, args.output)

# Create a surface hierarchy map (Gen1 / Gen2)
if args.output_json_sensitives_map:
acts.examples.writeSensitivesMapToJson(
detector, args.output + "_sensitives.json", 4
)

return

Expand Down
5 changes: 5 additions & 0 deletions Plugins/Json/include/Acts/Plugins/Json/SurfaceJsonConverter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ void to_json(nlohmann::json& j, const Surface& surface);
/// @note it will take the default context
void to_json(nlohmann::json& j, const std::shared_ptr<const Surface>& surface);

/// Non-contextual conversion of a surface
///
/// @note it will take the default context
void to_json(nlohmann::json& j, const Surface* surface);
Comment on lines +49 to +52
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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


/// Contextual conversion of a surface
///
/// @param j the json to be filled
Expand Down
5 changes: 5 additions & 0 deletions Plugins/Json/src/SurfaceJsonConverter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to check if the surface does not have an associated detector element, otherwise, this is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@paulgessinger paulgessinger Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

void Acts::to_json(nlohmann::json& j,
const std::shared_ptr<const Acts::Surface>& surface) {
Acts::GeometryContext gctx;
Expand Down
29 changes: 29 additions & 0 deletions Tests/UnitTests/Plugins/Json/SurfaceJsonConverterTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,35 @@ BOOST_AUTO_TEST_CASE(PerigeeRoundTripTests) {
BOOST_CHECK_EQUAL(perigeeTest->geometryId(), perigeeRef->geometryId());
}

BOOST_AUTO_TEST_CASE(CylinderToJsonPlainPointerReference) {
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));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
cylinderRef->assignGeometryId(GeometryIdentifier(11u));
cylinderRef->assignGeometryId(GeometryIdentifier(14u));


const Surface* surf = cylinderRef.get();

// Test a cylinder
nlohmann::json cylinderOut = surf;
out.open("CylinderSurfacePlainPointer.json");
out << cylinderOut.dump(2);
out.close();

auto in = std::ifstream("CylinderSurfacePlainPointer.json",
std::ifstream::in | std::ifstream::binary);
BOOST_CHECK(in.good());
nlohmann::json cylinderIn;
in >> cylinderIn;
in.close();

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());
Comment on lines +227 to +232
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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);

}

BOOST_AUTO_TEST_CASE(SurfacesDetrayTests) {
Transform3 trf(Transform3::Identity() * Translation3(0., 0., -7.));
auto trapezoid = std::make_shared<TrapezoidBounds>(2., 3., 4.);
Expand Down
Loading