refactor!: Make nlohmann::json a public dependency #3954
Conversation
WalkthroughA significant enhancement to the ACTS project, this pull request introduces JSON serialization and deserialization capabilities across various components. Key changes include the addition of a new option for the Changes
Poem
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 (7)
Core/include/Acts/Utilities/BinningType.hpp (1)
50-59: Complete, the JSON serialization mapping is, hmm.All enum values covered properly, they are. But missing, the tests for JSON serialization are.
Help you create JSON serialization tests, I can. Create a new test file, shall we?
Tests/UnitTests/Examples/Algorithms/Digitization/ModuleClustersTests.cpp (1)
50-53: More explicit, the BinUtility construction now is.Improved readability, this change has. But align the formatting better, we should.
Apply this formatting, you should:
- binUtility += BinUtility{ - BinningData(BinningOption::open, BinningValue::binX, 20, -10.0f, 10.0f)}; - binUtility += BinUtility{ - BinningData(BinningOption::open, BinningValue::binY, 20, -10.0f, 10.0f)}; + binUtility += BinUtility{BinningData( + BinningOption::open, BinningValue::binX, 20, -10.0f, 10.0f)}; + binUtility += BinUtility{BinningData( + BinningOption::open, BinningValue::binY, 20, -10.0f, 10.0f)};Tests/UnitTests/Core/Geometry/ExtentTests.cpp (1)
189-209: Remove debug output and strengthen test coverage, we must!Good start with basic roundtrip testing, this is. But improvements, suggest I do:
- Remove debug output with std::cout, as temporary it seems
- Add more test cases with different binning values, comprehensive coverage to achieve
Apply this diff to improve the test:
BOOST_AUTO_TEST_CASE(JsonExtentRoundtripTests) { Extent e; e.set(BinningValue::binR, 0, 200); e.set(BinningValue::binZ, -50, 50); nlohmann::json j; j["extent"] = e; - std::cout << j.dump(2) << std::endl; - Extent eIn = j["extent"]; CHECK_CLOSE_ABS(eIn.min(BinningValue::binR), e.min(BinningValue::binR), 10e-5); CHECK_CLOSE_ABS(eIn.max(BinningValue::binR), e.max(BinningValue::binR), 10e-5); CHECK_CLOSE_ABS(eIn.min(BinningValue::binZ), e.min(BinningValue::binZ), 10e-5); CHECK_CLOSE_ABS(eIn.max(BinningValue::binZ), e.max(BinningValue::binZ), 10e-5); + + // Test additional binning values + e.set(BinningValue::binPhi, -M_PI, M_PI); + e.set(BinningValue::binEta, -4.0, 4.0); + + j["extent"] = e; + eIn = j["extent"]; + + CHECK_CLOSE_ABS(eIn.min(BinningValue::binPhi), e.min(BinningValue::binPhi), + 10e-5); + CHECK_CLOSE_ABS(eIn.max(BinningValue::binPhi), e.max(BinningValue::binPhi), + 10e-5); + CHECK_CLOSE_ABS(eIn.min(BinningValue::binEta), e.min(BinningValue::binEta), + 10e-5); + CHECK_CLOSE_ABS(eIn.max(BinningValue::binEta), e.max(BinningValue::binEta), + 10e-5); }Core/src/Geometry/Extent.cpp (1)
215-240: Simplify the JSON serialization logic, young Padawan.Unnecessary scope blocks, I sense. More elegant and concise, the code can be, hmm.
void Acts::to_json(nlohmann::json& j, const Acts::Extent& e) { - { nlohmann::json jrange; const auto& xrange = e.range(); for (auto ibv : allBinningValues()) { if (e.constrains(ibv)) { jrange[binningValueName(ibv)] = xrange[toUnderlying(ibv)]; } } j["range"] = jrange; - } - - { nlohmann::json jenvelope; const auto& envelope = e.envelope(); for (auto ibv : allBinningValues()) { if (envelope[ibv] != zeroEnvelope) { jenvelope[binningValueName(ibv)] = Range1D<double>(envelope[ibv][0], envelope[ibv][1]); } } if (!jenvelope.empty()) { j["envelope"] = jenvelope; } - } }Core/include/Acts/Geometry/Extent.hpp (1)
26-27: Forward declarations, a path to faster compilation they are.Wise choice using json_fwd.hpp, reducing compilation dependencies it does. Clean and consistent with the Force, these JSON function declarations are.
Consider documenting the JSON format in the header file, help future Padawans it will.
Also applies to: 331-334
Tests/UnitTests/Plugins/Json/ProtoDetectorJsonConverterTests.cpp (2)
36-67: Hmmmm, equality comparison function, well-structured it is.Thoroughly compare BinningData objects, this function does. But a small typo in variable name 'euqalStructure', I sense.
Fix the typo, we should:
- bool euqalStructure = + bool equalStructure = (ba.subBinningData != nullptr) ? isEqual(*ba.subBinningData, *bb.subBinningData, tolerance) : (bb.subBinningData == nullptr); - BOOST_CHECK(euqalStructure); + BOOST_CHECK(equalStructure); - return equalBool && equalRange && euqalStructure; + return equalBool && equalRange && equalStructure;
69-88: Wise comparison of Extent objects, this is.Handle floating-point comparisons with tolerance, you do. But early return on constraint mismatch, consider we should.
Optimize the flow, we can:
bool isEqual(const Acts::Extent& ea, const Acts::Extent& eb, double tolerance = 0.) { bool equalConstrains = true; bool equalRange = true; for (auto& bVal : allBinningValues()) { equalConstrains = equalConstrains && (ea.constrains(bVal) == eb.constrains(bVal)); BOOST_CHECK(equalConstrains); + if (!equalConstrains) { + return false; + } if (ea.constrains(bVal) && eb.constrains(bVal)) { equalRange = equalRange && std::abs(ea.min(bVal) - eb.min(bVal)) < tolerance; equalRange = equalRange && std::abs(ea.max(bVal) - eb.max(bVal)) < tolerance; BOOST_CHECK(equalRange); + if (!equalRange) { + return false; + } } } - BOOST_CHECK(equalConstrains); - BOOST_CHECK(equalRange); return equalRange && equalConstrains; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (40)
CMakeLists.txt(4 hunks)Core/CMakeLists.txt(1 hunks)Core/include/Acts/Definitions/Algebra.hpp(2 hunks)Core/include/Acts/Geometry/Extent.hpp(2 hunks)Core/include/Acts/Utilities/AxisFwd.hpp(3 hunks)Core/include/Acts/Utilities/BinUtility.hpp(2 hunks)Core/include/Acts/Utilities/BinningData.hpp(2 hunks)Core/include/Acts/Utilities/BinningType.hpp(2 hunks)Core/include/Acts/Utilities/RangeXD.hpp(2 hunks)Core/src/Definitions/Algebra.cpp(1 hunks)Core/src/Definitions/CMakeLists.txt(1 hunks)Core/src/Geometry/Extent.cpp(1 hunks)Core/src/Utilities/BinningData.cpp(0 hunks)Core/src/Utilities/BinningUtility.cpp(1 hunks)Core/src/Utilities/CMakeLists.txt(1 hunks)Examples/Io/Json/src/JsonDigitizationConfig.cpp(0 hunks)Plugins/Json/CMakeLists.txt(0 hunks)Plugins/Json/include/Acts/Plugins/Json/AlgebraJsonConverter.hpp(0 hunks)Plugins/Json/include/Acts/Plugins/Json/ExtentJsonConverter.hpp(0 hunks)Plugins/Json/include/Acts/Plugins/Json/GridJsonConverter.hpp(0 hunks)Plugins/Json/include/Acts/Plugins/Json/IndexedGridJsonHelper.hpp(0 hunks)Plugins/Json/include/Acts/Plugins/Json/ProtoDetectorJsonConverter.hpp(0 hunks)Plugins/Json/include/Acts/Plugins/Json/UtilitiesJsonConverter.hpp(0 hunks)Plugins/Json/src/AlgebraJsonConverter.cpp(0 hunks)Plugins/Json/src/DetectorVolumeFinderJsonConverter.cpp(0 hunks)Plugins/Json/src/ExtentJsonConverter.cpp(0 hunks)Plugins/Json/src/GridJsonConverter.cpp(0 hunks)Plugins/Json/src/IndexedSurfacesJsonConverter.cpp(0 hunks)Plugins/Json/src/MaterialJsonConverter.cpp(1 hunks)Plugins/Json/src/PortalJsonConverter.cpp(0 hunks)Plugins/Json/src/ProtoDetectorJsonConverter.cpp(0 hunks)Tests/UnitTests/Core/Geometry/ExtentTests.cpp(1 hunks)Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp(1 hunks)Tests/UnitTests/Core/Utilities/Range1DTests.cpp(2 hunks)Tests/UnitTests/Examples/Algorithms/Digitization/ModuleClustersTests.cpp(2 hunks)Tests/UnitTests/Plugins/Json/CMakeLists.txt(0 hunks)Tests/UnitTests/Plugins/Json/EqualityHelpers.hpp(0 hunks)Tests/UnitTests/Plugins/Json/ExtentJsonConverterTests.cpp(0 hunks)Tests/UnitTests/Plugins/Json/ProtoDetectorJsonConverterTests.cpp(1 hunks)Tests/UnitTests/Plugins/Json/UtilitiesJsonConverterTests.cpp(0 hunks)
💤 Files with no reviewable changes (20)
- Plugins/Json/include/Acts/Plugins/Json/ProtoDetectorJsonConverter.hpp
- Plugins/Json/include/Acts/Plugins/Json/ExtentJsonConverter.hpp
- Plugins/Json/CMakeLists.txt
- Tests/UnitTests/Plugins/Json/CMakeLists.txt
- Tests/UnitTests/Plugins/Json/ExtentJsonConverterTests.cpp
- Plugins/Json/include/Acts/Plugins/Json/IndexedGridJsonHelper.hpp
- Plugins/Json/include/Acts/Plugins/Json/GridJsonConverter.hpp
- Plugins/Json/src/DetectorVolumeFinderJsonConverter.cpp
- Tests/UnitTests/Plugins/Json/UtilitiesJsonConverterTests.cpp
- Plugins/Json/include/Acts/Plugins/Json/UtilitiesJsonConverter.hpp
- Plugins/Json/src/ExtentJsonConverter.cpp
- Plugins/Json/src/ProtoDetectorJsonConverter.cpp
- Examples/Io/Json/src/JsonDigitizationConfig.cpp
- Plugins/Json/src/AlgebraJsonConverter.cpp
- Plugins/Json/include/Acts/Plugins/Json/AlgebraJsonConverter.hpp
- Plugins/Json/src/GridJsonConverter.cpp
- Tests/UnitTests/Plugins/Json/EqualityHelpers.hpp
- Plugins/Json/src/PortalJsonConverter.cpp
- Plugins/Json/src/IndexedSurfacesJsonConverter.cpp
- Core/src/Utilities/BinningData.cpp
✅ Files skipped from review due to trivial changes (1)
- Core/src/Definitions/CMakeLists.txt
🔇 Additional comments (18)
Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp (1)
188-257: Good addition, the 'JsonBinUtilityRoundTripTests' is.
Ensures serialization and deserialization of 'BinUtility' objects are properly tested, this test does.
Core/src/Utilities/CMakeLists.txt (1)
9-10: Good additions, these source files are.
Including 'BinningData.cpp' and 'BinningUtility.cpp' ensures new implementations are compiled.
Core/src/Utilities/BinningUtility.cpp (1)
1-42: Well implemented, the JSON serialization functions are.
'to_json' and 'from_json' for 'BinUtility' correctly handle serialization, they do.
Core/src/Definitions/Algebra.cpp (1)
13-48: Properly implemented, the JSON serialization for 'Transform3' is.
Functions 'to_json' and 'from_json' correctly serialize and deserialize 'Transform3', they do.
Core/CMakeLists.txt (1)
21-24: Correct, the public dependency declaration is, hmm yes.
Properly declared as PUBLIC dependency alongside Boost and Eigen, nlohmann_json is. Align with the PR objective to make it a hard dependency, this does.
Verify the dependency setup, we must:
✅ Verification successful
Correct and well-justified, the PUBLIC dependency declaration is!
Multiple public headers in Core, using nlohmann_json they are. In Utilities, Geometry, and Definitions, json types exposed they become. Through the Force, I sense the proper setup:
find_package(nlohmann_json)in root CMakeLists.txt, present it is- Public headers like
AxisFwd.hpp,Algebra.hpp, and others, json they include - Both full and forward declarations (
json.hppandjson_fwd.hpp), properly used they are
The PUBLIC linkage in Core/CMakeLists.txt, essential it is, as downstream targets these headers they shall consume. Hmmmm, yes!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if nlohmann_json is properly set up in other CMake files
# Test: Search for nlohmann_json setup in parent CMake files
rg "find_package.*nlohmann_json" -g "CMakeLists.txt"
# Test: Check for any existing json-related options
rg "option.*json" -g "CMakeLists.txt"
Length of output: 509
Script:
#!/bin/bash
# Check if json is used in Core headers which would justify PUBLIC linkage
rg -l "nlohmann::json|nlohmann/json" --type hpp -g "Core/include/**"
# Check if json is only used in implementation files
rg -l "nlohmann::json|nlohmann/json" --type cpp -g "Core/src/**"
# Verify if json is properly set up in parent CMake before Core
fd "CMakeLists.txt" | xargs cat | awk '/add_subdirectory.*Core/{p=NR} /find_package.*nlohmann_json/{if(NR<p)print}'
Length of output: 416
Script:
#!/bin/bash
# Search for json usage in header files with correct extension
rg -l "nlohmann::json|nlohmann/json" -g "Core/include/**/*.h*"
# Check if any public headers include json headers
rg "#include.*json" -g "Core/include/**/*.h*"
# Check the order of find_package and add_subdirectory in root CMake
grep -E "find_package.*nlohmann_json|add_subdirectory.*Core" CMakeLists.txt
Length of output: 992
Core/include/Acts/Utilities/BinningType.hpp (1)
14-14: Include the json header at proper location, you have.
Placed before the Acts namespace declaration, maintaining proper include order, it is.
Core/include/Acts/Utilities/AxisFwd.hpp (2)
15-15: Correct, the JSON serialization for AxisBoundaryType is, hmm!
Match the enum values to their string representations perfectly, you do. Maintain consistency with the existing operator<< implementation, this does.
Also applies to: 32-35
70-73: Wise implementation of AxisType serialization, this is!
Consistent with operator<< implementation and enum values, the serialization remains. Good practices, you follow.
Core/include/Acts/Definitions/Algebra.hpp (2)
96-98: Declarations for Transform3 serialization, well-structured they are!
Follow the standard nlohmann/json patterns for custom type serialization, these declarations do.
102-113: Specialized ADL serializer, properly implemented it is!
Delegate to Acts namespace functions correctly, it does. Follow nlohmann/json best practices for ADL-based serialization, this implementation does.
Core/include/Acts/Utilities/BinUtility.hpp (1)
59-60: Wise changes these are, approve them I do.
The Force is strong with the explicit keyword. Prevent accidental conversions, it will. Clean and clear, the JSON function declarations are.
Also applies to: 331-334
Tests/UnitTests/Core/Utilities/Range1DTests.cpp (1)
613-623: Hmm, good test coverage for JSON serialization, I sense!
Proper validation of serialization round-trip, this test provides. Appropriate tolerance for floating-point comparisons, it uses.
Core/include/Acts/Utilities/BinningData.hpp (2)
25-25: Wise choice of forward declaration, this is!
Using json_fwd.hpp instead of full json.hpp, reduce compilation dependencies it does. The Force of clean architecture, strong with this one is!
559-561: Well-structured JSON interface functions, these are!
Follow the nlohmann::json conventions for serialization, these declarations do. In namespace Acts, properly placed they are.
CMakeLists.txt (2)
93-93: Clear and consistent option naming, I observe!
Follow project conventions for CMake options, this does. Default value of ON, reasonable it is.
313-320: Flexible configuration, provided it is!
Choice between system and bundled versions, users have. Clear message about system usage, logged it is. Version requirement of 3.10.5, sufficient it appears.
Core/include/Acts/Utilities/RangeXD.hpp (2)
18-18: Hmmmm, include the JSON header, you must.
Correctly placed, this include statement is. Essential for JSON serialization capabilities, it becomes.
640-644: Strong with the Force, this serialization function is.
Wisely implemented, the to_json function is. Properly handles the Range1D type, it does. Clear and concise, the implementation remains.
github-actions
bot
added
Component - Core
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CMakeLists.txt (1)
313-320: Well structured, this implementation is!But improve the error handling, we could. A message when falling back to bundled version, helpful it would be.
Apply this change, you should:
if(ACTS_SETUP_NLOHMANN_JSON) if(ACTS_USE_SYSTEM_NLOHMANN_JSON) message(STATUS "Using system installation of nlohmann::json") find_package(nlohmann_json ${_acts_nlohmanjson_version} REQUIRED CONFIG) else() + message(STATUS "Using bundled nlohmann::json") add_subdirectory(thirdparty/nlohmann_json) endif() endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CMakeLists.txt(5 hunks)docs/getting_started.md(1 hunks)
🔇 Additional comments (3)
CMakeLists.txt (2)
93-93: Strong with the Force, this configuration is!
Like ACTS_SETUP_BOOST and ACTS_SETUP_EIGEN3, this option follows. Consistency in the build system, maintain we must.
39-39: Hmmmm, wise choice this is!
Setting the default to ON, a good practice it is. System-provided versions, more maintainable they are.
Run this verification, we must:
✅ Verification successful
Let us see how the Force guides the system json integration, hmmmm.
Wise implementation, confirmed it is!
Through the Force, seen I have:
- All json includes use standard path
nlohmann/json.hpp, not bundled path they do - CMake logic properly handles system vs bundled version it does
- No direct dependencies on bundled path location exist
- Clean separation between system and bundled versions maintained it is
The path to system-provided json, clear and well-structured it is. Default value of ON, cause no disturbance in the Force it will.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any parts of the codebase assume bundled nlohmann_json
# Search for direct includes of bundled json
rg -l "thirdparty/nlohmann_json"
# Search for json-related CMake conditions that might need updates
rg "ACTS_BUILD_PLUGIN_JSON|nlohmann_json"
Length of output: 2926
Script:
#!/bin/bash
# Check for the new option and its usage
rg -A 5 "ACTS_USE_SYSTEM_NLOHMANN_JSON" CMakeLists.txt
# Check if there are any direct references to bundled json header paths
rg "thirdparty/nlohmann_json/.*\.hpp"
# Look for any potential json-related include paths
rg "#include.*json"
Length of output: 4841
docs/getting_started.md (1)
271-271: Documentation, clear and accurate it is!
Matches the implementation perfectly, this does. Help users understand the configuration options better, it will.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Core/src/Geometry/Extent.cpp (1)
215-240: Approve the serialization implementation, I do.Clean and efficient, the code is. Serialize only what is needed, it does. Hmmmm.
Suggest extracting range and envelope serialization into separate functions, I do. Clearer intent and better testability, it would provide:
+void serializeRange(nlohmann::json& j, const Acts::Extent& e) { + nlohmann::json jrange; + const auto& xrange = e.range(); + for (auto ibv : allBinningValues()) { + if (e.constrains(ibv)) { + jrange[binningValueName(ibv)] = xrange[toUnderlying(ibv)]; + } + } + j["range"] = jrange; +} + +void serializeEnvelope(nlohmann::json& j, const Acts::Extent& e) { + nlohmann::json jenvelope; + const auto& envelope = e.envelope(); + for (auto ibv : allBinningValues()) { + if (envelope[ibv] != zeroEnvelope) { + jenvelope[binningValueName(ibv)] = + Range1D<double>(envelope[ibv][0], envelope[ibv][1]); + } + } + if (!jenvelope.empty()) { + j["envelope"] = jenvelope; + } +} + void Acts::to_json(nlohmann::json& j, const Acts::Extent& e) { - { - nlohmann::json jrange; - // ... range serialization - } - { - nlohmann::json jenvelope; - // ... envelope serialization - } + serializeRange(j, e); + serializeEnvelope(j, e); }Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp (1)
173-173: A typo in the documentation, there is."bollean" spelled incorrectly it is. "boolean" the correct spelling should be.
Apply this diff to fix the typo:
-/// @return a bollean if equal +/// @return a boolean if equal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Core/include/Acts/Utilities/RangeXD.hpp(2 hunks)Core/src/Geometry/Extent.cpp(1 hunks)Plugins/Json/src/MaterialJsonConverter.cpp(1 hunks)Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Utilities/RangeXD.hpp
🔇 Additional comments (4)
Core/src/Geometry/Extent.cpp (1)
242-261:
Handle the dark side of invalid JSON, we must.
A disturbance in the Force, I sense. Protect against malformed JSON and invalid values, we should. Previous wisdom, still applies it does.
Additional protection against the dark side, suggest I do:
void Acts::from_json(const nlohmann::json& j, Acts::Extent& e) {
+ if (!j.contains("range")) {
+ throw std::invalid_argument("Missing required 'range' field in JSON");
+ }
const auto& jrange = j.at("range");
for (const auto& [key, value] : jrange.items()) {
+ if (!value.is_object()) {
+ throw std::invalid_argument("Range values must be objects");
+ }
BinningValue bval = binningValueFromName(key);
- e.set(bval, value.at("min"), value.at("max"));
+ try {
+ const double min = value.at("min").get<double>();
+ const double max = value.at("max").get<double>();
+ e.set(bval, min, max);
+ } catch (const nlohmann::json::exception& ex) {
+ throw std::invalid_argument("Invalid min/max values in range");
+ }
}
if (j.contains("envelope")) {
const auto& jenvelope = j.at("envelope");
+ if (!jenvelope.is_object()) {
+ throw std::invalid_argument("Envelope must be an object");
+ }
ExtentEnvelope envelope;
for (const auto& [key, value] : jenvelope.items()) {
+ if (!value.is_object()) {
+ throw std::invalid_argument("Envelope values must be objects");
+ }
BinningValue bval = binningValueFromName(key);
- envelope[bval] = {value["min"], value["max"]};
+ try {
+ const double min = value.at("min").get<double>();
+ const double max = value.at("max").get<double>();
+ envelope[bval] = {min, max};
+ } catch (const nlohmann::json::exception& ex) {
+ throw std::invalid_argument("Invalid min/max values in envelope");
+ }
}
e.setEnvelope(envelope);
}
}Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp (1)
188-257: Well-structured test case, this is. Approve it, I do.
Comprehensive testing of JSON serialization it provides:
- One-dimensional BinUtility testing
- Two-dimensional BinUtility testing
- Three-dimensional BinUtility testing
- Transformation scenario testing
Plugins/Json/src/MaterialJsonConverter.cpp (2)
700-700:
Throw the errors, you must.
Created runtime_error objects are, but thrown they are not. Add the throw keyword, we must.
Apply this diff to fix the error handling:
- std::runtime_error("Unsupported binning for Detray");
+ throw std::runtime_error("Unsupported binning for Detray");
- std::runtime_error("Unsupported binning for Detray");
+ throw std::runtime_error("Unsupported binning for Detray");Also applies to: 724-724
706-709:
A disturbance in the Force, I sense. Wrong variable used for grid linking, it is.
The code uses gridIndex for lookup when gridIndexType it should use. Lead to incorrect grid linking, this will.
Apply this diff to fix the grid linking:
if (gridLink.contains(gridIndexType)) {
- std::size_t& fGridIndex = gridLink[gridIndex];
+ std::size_t& fGridIndex = gridLink[gridIndexType];
gridIndex = fGridIndex;
fGridIndex++;Likely invalid or redundant comment.
|
@stephenswat, @asalzburger for this we'll need to get rid of the usage of removed headers in traccc: I'll look into it next week. We'll probably need a quick tag on traccc so we can make our last major version of the year in time. |
paulgessinger
force-pushed
the
hard-json-dependency
branch
from
d1dd044 to
c157df9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
thirdparty/traccc/CMakeLists.txt (2)
18-23: Handle patch failures gracefully, you should!Silent failures in the patch command, lead to the dark side they do. Consider adding error handling, wise it would be.
FetchContent_Declare( traccc ${ACTS_TRACCC_SOURCE} PATCH_COMMAND - patch -p1 -i ${CMAKE_CURRENT_SOURCE_DIR}/remove-json-include.diff + bash -c "if ! patch -p1 --forward -i ${CMAKE_CURRENT_SOURCE_DIR}/remove-json-include.diff; then echo 'Patch failed to apply cleanly' >&2; exit 1; fi" )
18-23: Document the patch's purpose, you must!Clear documentation, the path to understanding is. Add comments explaining the patch's purpose, help future maintainers it will.
+# Apply patch to remove JSON includes as part of making nlohmann::json a public dependency FetchContent_Declare( traccc ${ACTS_TRACCC_SOURCE} PATCH_COMMAND patch -p1 -i ${CMAKE_CURRENT_SOURCE_DIR}/remove-json-include.diff )Core/include/Acts/Definitions/Algebra.hpp (1)
102-113: Strong with the Force, this design pattern is.Wisely chosen, the ADL serializer pattern is. Through it:
- Automatic serialization in JSON containers, achieved it is
- Clean separation of concerns, maintained it remains
- Extension points for future types, provided they are
Yet, document these functions in your source files, you must. Clear documentation, the path to enlightenment is.
Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp (1)
175-184: Ensure equality checks for all binning data, we must.Although looping over binning data, the function is, not returning early if inequality found, it should. Consider adding a break or returning false immediately upon finding unequal binning data.
Apply this diff to improve the code:
if (equal) { for (std::size_t ib = 0; ib < ba.binningData().size(); ++ib) { equal = isEqual(ba.binningData()[ib], bb.binningData()[ib], tolerance); BOOST_CHECK(equal); + if (!equal) { + return false; + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (43)
CMakeLists.txt(5 hunks)Core/CMakeLists.txt(1 hunks)Core/include/Acts/Definitions/Algebra.hpp(2 hunks)Core/include/Acts/Geometry/Extent.hpp(2 hunks)Core/include/Acts/Utilities/AxisFwd.hpp(3 hunks)Core/include/Acts/Utilities/BinUtility.hpp(2 hunks)Core/include/Acts/Utilities/BinningData.hpp(2 hunks)Core/include/Acts/Utilities/BinningType.hpp(2 hunks)Core/include/Acts/Utilities/RangeXD.hpp(2 hunks)Core/src/Definitions/Algebra.cpp(1 hunks)Core/src/Definitions/CMakeLists.txt(1 hunks)Core/src/Geometry/Extent.cpp(1 hunks)Core/src/Utilities/BinningData.cpp(0 hunks)Core/src/Utilities/BinningUtility.cpp(1 hunks)Core/src/Utilities/CMakeLists.txt(1 hunks)Examples/Io/Json/src/JsonDigitizationConfig.cpp(0 hunks)Plugins/Detray/src/DetrayMaterialConverter.cpp(1 hunks)Plugins/Json/CMakeLists.txt(0 hunks)Plugins/Json/include/Acts/Plugins/Json/AlgebraJsonConverter.hpp(0 hunks)Plugins/Json/include/Acts/Plugins/Json/ExtentJsonConverter.hpp(0 hunks)Plugins/Json/include/Acts/Plugins/Json/GridJsonConverter.hpp(0 hunks)Plugins/Json/include/Acts/Plugins/Json/IndexedGridJsonHelper.hpp(0 hunks)Plugins/Json/include/Acts/Plugins/Json/ProtoDetectorJsonConverter.hpp(0 hunks)Plugins/Json/include/Acts/Plugins/Json/UtilitiesJsonConverter.hpp(0 hunks)Plugins/Json/src/AlgebraJsonConverter.cpp(0 hunks)Plugins/Json/src/DetectorVolumeFinderJsonConverter.cpp(0 hunks)Plugins/Json/src/ExtentJsonConverter.cpp(0 hunks)Plugins/Json/src/GridJsonConverter.cpp(0 hunks)Plugins/Json/src/IndexedSurfacesJsonConverter.cpp(0 hunks)Plugins/Json/src/MaterialJsonConverter.cpp(1 hunks)Plugins/Json/src/PortalJsonConverter.cpp(0 hunks)Plugins/Json/src/ProtoDetectorJsonConverter.cpp(0 hunks)Tests/UnitTests/Core/Geometry/ExtentTests.cpp(1 hunks)Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp(1 hunks)Tests/UnitTests/Core/Utilities/Range1DTests.cpp(2 hunks)Tests/UnitTests/Plugins/Json/CMakeLists.txt(0 hunks)Tests/UnitTests/Plugins/Json/EqualityHelpers.hpp(0 hunks)Tests/UnitTests/Plugins/Json/ExtentJsonConverterTests.cpp(0 hunks)Tests/UnitTests/Plugins/Json/ProtoDetectorJsonConverterTests.cpp(1 hunks)Tests/UnitTests/Plugins/Json/UtilitiesJsonConverterTests.cpp(0 hunks)docs/getting_started.md(1 hunks)thirdparty/traccc/CMakeLists.txt(1 hunks)thirdparty/traccc/remove-json-include.diff(1 hunks)
💤 Files with no reviewable changes (20)
- Plugins/Json/CMakeLists.txt
- Plugins/Json/include/Acts/Plugins/Json/ProtoDetectorJsonConverter.hpp
- Plugins/Json/include/Acts/Plugins/Json/ExtentJsonConverter.hpp
- Plugins/Json/src/IndexedSurfacesJsonConverter.cpp
- Tests/UnitTests/Plugins/Json/CMakeLists.txt
- Plugins/Json/src/DetectorVolumeFinderJsonConverter.cpp
- Plugins/Json/src/ProtoDetectorJsonConverter.cpp
- Tests/UnitTests/Plugins/Json/ExtentJsonConverterTests.cpp
- Core/src/Utilities/BinningData.cpp
- Plugins/Json/include/Acts/Plugins/Json/IndexedGridJsonHelper.hpp
- Plugins/Json/include/Acts/Plugins/Json/GridJsonConverter.hpp
- Examples/Io/Json/src/JsonDigitizationConfig.cpp
- Plugins/Json/src/GridJsonConverter.cpp
- Plugins/Json/src/AlgebraJsonConverter.cpp
- Tests/UnitTests/Plugins/Json/UtilitiesJsonConverterTests.cpp
- Plugins/Json/include/Acts/Plugins/Json/AlgebraJsonConverter.hpp
- Plugins/Json/src/ExtentJsonConverter.cpp
- Tests/UnitTests/Plugins/Json/EqualityHelpers.hpp
- Plugins/Json/src/PortalJsonConverter.cpp
- Plugins/Json/include/Acts/Plugins/Json/UtilitiesJsonConverter.hpp
✅ Files skipped from review due to trivial changes (1)
- thirdparty/traccc/remove-json-include.diff
🚧 Files skipped from review as they are similar to previous changes (16)
- Core/src/Utilities/CMakeLists.txt
- Tests/UnitTests/Core/Geometry/ExtentTests.cpp
- Core/CMakeLists.txt
- Core/src/Definitions/CMakeLists.txt
- Core/include/Acts/Utilities/BinningData.hpp
- Tests/UnitTests/Core/Utilities/Range1DTests.cpp
- Core/src/Definitions/Algebra.cpp
- Core/src/Utilities/BinningUtility.cpp
- Core/include/Acts/Utilities/BinningType.hpp
- Core/include/Acts/Utilities/RangeXD.hpp
- Core/include/Acts/Geometry/Extent.hpp
- Core/include/Acts/Utilities/AxisFwd.hpp
- Core/include/Acts/Utilities/BinUtility.hpp
- Plugins/Json/src/MaterialJsonConverter.cpp
- CMakeLists.txt
- docs/getting_started.md
🔇 Additional comments (9)
thirdparty/traccc/CMakeLists.txt (1)
18-23: Verify the patch file's existence and content, we must!
Hmmmm. A patch file, you reference, but its existence, we must confirm. The path to wisdom, through verification leads.
✅ Verification successful
Verified, the patch file is. Safe to proceed, we are!
Found the patch file at the correct location, I have. Simple but purposeful, its changes are - removing an unnecessary JSON utility include while preserving the essential ones. Aligned with the PR's objective of JSON dependency restructuring, this modification is.
Confident in this change, I am, for:
- Exists in the correct location, the patch file does
- Clean and focused, the changes are
- Removes only the
UtilitiesJsonConverter.hppinclude, while maintaining other necessary JSON functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify patch file existence and content
# Check if patch file exists
if [ -f "thirdparty/traccc/remove-json-include.diff" ]; then
echo "Found patch file"
# Show patch content
cat "thirdparty/traccc/remove-json-include.diff"
else
echo "Patch file not found!"
fi
Length of output: 603
Core/include/Acts/Definitions/Algebra.hpp (3)
26-26: Wise choice, this include statement is, hmmmm.
Properly placed after Eigen headers but before our namespace, it is. The path to JSON serialization, this header opens.
96-98: Verify the implementation of these functions, we must.
Declared these functions are, but their implementation, see we must. In the source files, their presence verify we shall.
✅ Verification successful
Implemented well, these functions are. The Force is strong with them.
Complete and proper, the implementation in Core/src/Definitions/Algebra.cpp is. Handle both rotation and translation components of Transform3, it does. Follows the ways of JSON serialization, it does:
- Properly handles translation with null case, it does
- Identity transform as default, it creates
- Rotation matrix and translation vector, it processes correctly
The balance in the code, I sense. No disturbance in the Force, there is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of json serialization functions
echo "Searching for Transform3 JSON serialization implementation..."
rg "void Acts::to_json.*Transform3" --type cpp
rg "void Acts::from_json.*Transform3" --type cpp
Length of output: 434
Script:
#!/bin/bash
# Let us see the implementation details, hmm
echo "Implementation details, reveal we shall..."
rg "void Acts::to_json.*Transform3" -A 10 --type cpp
rg "void Acts::from_json.*Transform3" -A 10 --type cpp
Length of output: 1835
Line range hint 26-113: Search your feelings, verify the impact we must.
Changed to public dependency, nlohmann::json has. Impact on dependent code, investigate we shall.
✅ Verification successful
Let us seek deeper understanding of the changes, hmm? More context we need before final judgment can be made.
Safe, this change is. Proceed, we may.
Clear as the waters of Dagobah, this matter has become. The JSON serialization of Transform3, moved to public interface it has been, but:
- Self-contained, the implementation remains
- In Core module only, the JSON functionality exists
- No breaking changes in the Force, I sense
- Other JSON converters in Plugins/Json untouched they stay
The path to public dependency, well-structured it is. Minimal impact on the codebase, this change brings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing Transform3 JSON usage
echo "Searching for existing Transform3 JSON usage..."
rg "to_json.*Transform3|from_json.*Transform3" --type cpp
# Search for potential impact in traccc component
echo "Checking traccc component impact..."
rg "Acts/Plugins/Json/UtilitiesJsonConverter\.hpp"
Length of output: 1000
Script:
#!/bin/bash
# Search for other JSON-related includes that might be affected
echo "Searching for JSON includes in the codebase..."
rg "include.*json" --type cpp
# Check for any other Transform3 serialization patterns
echo "Checking Transform3 serialization patterns..."
rg "Transform3.*serialize|serialize.*Transform3" --type cpp
# Look for implementation details
echo "Examining Transform3 JSON implementation..."
cat Core/src/Definitions/Algebra.cpp
Length of output: 6280
Plugins/Detray/src/DetrayMaterialConverter.cpp (1)
Line range hint 176-182: Handles bin utility swapping correctly, this code does.
Properly swaps bin utilities when dimensions are Z and Phi, ensuring correct grid configuration, it does.
Core/src/Geometry/Extent.cpp (1)
242-261:
Handle missing JSON fields, you must.
Assumes the presence of required fields, the from_json method does. To prevent exceptions and handle malformed JSON gracefully, check for required keys, you should.
Apply this diff to add error handling:
void Acts::from_json(const nlohmann::json& j, Acts::Extent& e) {
+ if (!j.contains("range")) {
+ throw std::invalid_argument("JSON object must contain 'range' field");
+ }
const auto& jrange = j.at("range");
for (const auto& [key, value] : jrange.items()) {
+ if (!value.contains("min") || !value.contains("max")) {
+ throw std::invalid_argument("Range must contain 'min' and 'max' fields");
+ }
BinningValue bval = binningValueFromName(key);
e.set(bval, value.at("min"), value.at("max"));
}
+ if (j.contains("envelope")) {
const auto& jenvelope = j.at("envelope");
ExtentEnvelope envelope;
for (const auto& [key, value] : jenvelope.items()) {
+ if (!value.contains("min") || !value.contains("max")) {
+ throw std::invalid_argument("Envelope must contain 'min' and 'max' fields");
+ }
BinningValue bval = binningValueFromName(key);
envelope[bval] = {value["min"], value["max"]};
}
e.setEnvelope(envelope);
}
}Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp (1)
164-165:
Include all checks in the return value, you must.
Performing equalBoundaries check, the function is, but not including it in the final return statement, it does. Modify the return statement to include equalBoundaries, you should.
Apply this diff to fix the issue:
BOOST_CHECK(equalBoundaries);
- return equalBool && equalRange && equalStructure;
+ return equalBool && equalRange && equalStructure && equalBoundaries;
}Tests/UnitTests/Plugins/Json/ProtoDetectorJsonConverterTests.cpp (2)
47-52:
Typo in variable name 'euqalStructure', correct it you must.
Misspelled 'equalStructure' as 'euqalStructure', it is. Consistency and clarity, we must maintain.
Apply this diff to fix the typo:
BOOST_CHECK(equalRange);
- bool euqalStructure =
+ bool equalStructure =
(ba.subBinningData != nullptr)
? isEqual(*ba.subBinningData, *bb.subBinningData, tolerance)
: (bb.subBinningData == nullptr);
- BOOST_CHECK(euqalStructure);
+ BOOST_CHECK(equalStructure);66-67:
Include all checks in the return value, you must.
Performing equalBoundaries check, the function is, but not including it in the final return statement, it does. Include equalBoundaries in the return value, you should.
Apply this diff to fix the issue:
BOOST_CHECK(equalBoundaries);
- return equalBool && equalRange && euqalStructure;
+ return equalBool && equalRange && equalStructure && equalBoundaries;
}
With this PR,
nlohmann::jsonbecomes a hard dependency of ACTS.Blocked by:
Summary by CodeRabbit
New Features
Transform3,Extent,BinUtility, andBinningData.nlohmann_jsonlibrary in the project setup.Extent,BinUtility, andRange1Dclasses.Bug Fixes
Documentation