feat: Structured logging #3955
feat: Structured logging #3955
Conversation
…after removal of helpers
WalkthroughHmm, a transformation of JSON handling in the ACTS project, this pull request represents. Modifications to CMake configurations, JSON serialization capabilities, and removal of utility converters, we observe. A comprehensive refactoring of JSON-related functionality across multiple files and components, this change embodies. Streamlining JSON interactions and centralizing serialization logic, the goal appears to be. Changes
Sequence DiagramsequenceDiagram
participant Core
participant JSON
participant Plugins
Core->>JSON: Serialize Object
JSON-->>Core: JSON Representation
Core->>Plugins: Use Serialized Data
Plugins-->>Core: Process Complete
Possibly Related PRs
Suggested Labels
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Component - Core
| /// @brief handle output as json | ||
| /// | ||
| /// @param [in] lvl debug output level of message | ||
| /// @param [in] json json object representing the log record | ||
| virtual void flush(const Level& lvl, nlohmann::json&& json) = 0; |
There was a problem hiding this comment.
is this breading? users inheriting from this class will not be able to compile after the change
There was a problem hiding this comment.
I can default it or we can make it breaking.
![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 comments (1)
Core/include/Acts/Utilities/Logger.hpp (1)
Line range hint
87-87: Documentation parameters, incorrect they are. Align them with macro definitions, you must.In the macro documentation for
ACTS_VERBOSE,ACTS_DEBUG,ACTS_INFO,ACTS_WARNING,ACTS_ERROR, andACTS_FATAL, the parameterxis used, but in the macro definitions,__VA_ARGS__is used instead. Consistency in parameter names, maintain you should.Apply this diff to update the parameter names:
-/// @param x debug message +/// @param ... debug message argumentsAlso applies to: 99-99, 111-111, 123-123, 135-135, 147-147
🧰 Tools
🪛 GitHub Actions: Docs
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
🧹 Nitpick comments (14)
Core/include/Acts/Utilities/Logger.hpp (5)
601-609: Consistency in JSON logging, maintain you should.In
LevelOutputDecorator::flush, when handling JSON output, the methodtoStringis used to set the "level" field. Consistent naming withlevelNamefunction, consider you might, for clarity and maintainability.Apply this diff for consistency:
- json["level"] = toString(lvl); + json["level"] = levelName(lvl);🧰 Tools
🪛 GitHub Actions: Docs
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
941-942: Member initialization order, attend to it you must.In the
Loggerconstructor, the initialization ofm_modeoccurs afterm_filterPolicy, but in the class definition,m_modeis declared beforem_filterPolicy. Reorder the initialization list to match declaration order, you should, to prevent warnings.Apply this diff to reorder the initialization list:
: m_printPolicy(std::move(pPrint)), - m_filterPolicy(std::move(pFilter)), m_mode{mode}, + m_filterPolicy(std::move(pFilter))🧰 Tools
🪛 GitHub Actions: Docs
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
857-878: UnhandledOutputModecases, consider future-proofing you should.In the
switchstatement handlingOutputMode, nodefaultcase there is. If new modes are added in the future, ensure they are handled or an error is raised.Apply this diff to add a default case:
} + default: + // Handle unexpected output modes + m_printPolicy->flush(lvl, message); + break;🧰 Tools
🪛 GitHub Actions: Docs
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
795-801: Move semantics, use them wisely you must.In the constructor parameter
OutputMode mode = OutputMode::Default, copying occurs. SinceOutputModeis an enum class, no issue it poses. But for consistency, consider passing by value withoutconst, you might.Apply this diff for consistency:
- OutputMode mode = OutputMode::Default) + OutputMode mode = OutputMode::Default)🧰 Tools
🪛 GitHub Actions: Docs
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
769-790: Enum documentation, clarify it could be.The
OutputModeenum lacks detailed comments on each mode's purpose. Enhance the documentation, you should, to aid future developers.🧰 Tools
🪛 GitHub Actions: Docs
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
Core/src/Definitions/Algebra.cpp (2)
15-21: Redundant code, eliminate you can.When the translation is zero, setting
j["translation"]to an empty JSON object may be unnecessary. Omitting the key entirely, or setting it to null, consider you might.Apply this diff to simplify:
- } else { - j["translation"] = nlohmann::json(); - }
35-48: Default initialization, improve readability it can.Initializing
tdirectly with identity and then modifying it may be less clear. Constructingtwith the necessary transformations, consider you might.Simplify the function:
- t = Acts::Transform3::Identity(); + Acts::Transform3 trf = Acts::Transform3::Identity(); if (j.find("rotation") != j.end() && !j["rotation"].empty()) { // existing code } if (j.find("translation") != j.end() && !j["translation"].empty()) { // existing code } - t = trf;Tests/UnitTests/Core/Geometry/ExtentTests.cpp (2)
197-197: Remove debug print statement, hmm.Use std::cout in tests, you should not. Temporary debugging artifact, this appears to be.
- std::cout << j.dump(2) << std::endl;
189-209: Additional test cases needed, I sense.Good start this test is, but more coverage we need. Missing are tests for:
- Edge cases with empty/invalid values
- Error conditions in JSON parsing
- Complex Extent objects with multiple constraints
Assist in generating additional test cases, I can. Want this, do you?
Core/include/Acts/Utilities/BinUtility.hpp (1)
331-333: Documentation for JSON functions, add you must.Missing are the function descriptions. Document parameters and behavior, we should:
+/// Convert BinUtility to JSON representation +/// @param j JSON object to write to +/// @param bu BinUtility to convert void to_json(nlohmann::json& j, const BinUtility& bu); +/// Create BinUtility from JSON representation +/// @param j JSON object to read from +/// @param bu BinUtility to populate void from_json(const nlohmann::json& j, BinUtility& bu);Tests/UnitTests/Core/Utilities/LoggerTests.cpp (2)
150-162: Documentation, improve you must!Good, the struct implementation is. But document the purpose and usage of each field, you should.
struct StructuredInfo { + /// Integer value for structured logging int value; + /// Threshold value for filtering float threshold; + /// Identifier name std::string name;
286-337: Well tested, the structured logging is. But organize better, we can!Thorough tests for structured logging, these are. But group related test cases using test fixtures, we should.
Consider organizing related test cases into a fixture:
struct StructuredLoggingTest { std::stringstream sstr; std::unique_ptr<Logger> _logger; Logger& logger; StructuredLoggingTest() : _logger(detail::create_logger("TestLogger", &sstr, Logging::INFO)) , logger(*_logger) {} }; BOOST_FIXTURE_TEST_SUITE(StructuredLogging, StructuredLoggingTest) // ... existing test cases ... BOOST_AUTO_TEST_SUITE_END()Core/include/Acts/Utilities/RangeXD.hpp (1)
640-650: Approve the JSON serialization functions, I do. Yet improve error handling, we must.Correct implementation for JSON serialization, you have created. But handle JSON parsing errors gracefully, we should.
Consider this improvement, you should:
template <typename Type> void from_json(const nlohmann::json& j, Range1D<Type>& r) { + if (!j.contains("min") || !j.contains("max")) { + throw std::invalid_argument("JSON object missing required fields: min, max"); + } r.setMin(static_cast<Type>(j["min"])); r.setMax(static_cast<Type>(j["max"])); }Tests/UnitTests/Plugins/Json/ProtoDetectorJsonConverterTests.cpp (1)
69-88: Optimize the test assertions, we should.Redundant BOOST_CHECK calls, I see. Move them outside the loop, we must.
Improve the code structure, you 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 (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); } } BOOST_CHECK(equalConstrains); BOOST_CHECK(equalRange); return equalRange && equalConstrains; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
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/Logger.hpp(20 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)Core/src/Utilities/Logger.cpp(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/LoggerTests.cpp(2 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)docs/getting_started.md(2 hunks)
💤 Files with no reviewable changes (20)
- Plugins/Json/CMakeLists.txt
- Plugins/Json/include/Acts/Plugins/Json/AlgebraJsonConverter.hpp
- Plugins/Json/src/IndexedSurfacesJsonConverter.cpp
- Plugins/Json/include/Acts/Plugins/Json/IndexedGridJsonHelper.hpp
- Plugins/Json/src/PortalJsonConverter.cpp
- Examples/Io/Json/src/JsonDigitizationConfig.cpp
- Plugins/Json/src/DetectorVolumeFinderJsonConverter.cpp
- Plugins/Json/src/ProtoDetectorJsonConverter.cpp
- Tests/UnitTests/Plugins/Json/CMakeLists.txt
- Plugins/Json/include/Acts/Plugins/Json/ProtoDetectorJsonConverter.hpp
- Plugins/Json/include/Acts/Plugins/Json/ExtentJsonConverter.hpp
- Plugins/Json/include/Acts/Plugins/Json/GridJsonConverter.hpp
- Core/src/Utilities/BinningData.cpp
- Tests/UnitTests/Plugins/Json/UtilitiesJsonConverterTests.cpp
- Plugins/Json/src/GridJsonConverter.cpp
- Tests/UnitTests/Plugins/Json/ExtentJsonConverterTests.cpp
- Plugins/Json/src/ExtentJsonConverter.cpp
- Plugins/Json/src/AlgebraJsonConverter.cpp
- Tests/UnitTests/Plugins/Json/EqualityHelpers.hpp
- Plugins/Json/include/Acts/Plugins/Json/UtilitiesJsonConverter.hpp
✅ Files skipped from review due to trivial changes (2)
- Plugins/Json/src/MaterialJsonConverter.cpp
- Tests/UnitTests/Examples/Algorithms/Digitization/ModuleClustersTests.cpp
🧰 Additional context used
🪛 GitHub Actions: Checks
Core/include/Acts/Definitions/Algebra.hpp
[error] 26-26: Missing dependency: nlohmann/json.hpp file not found. This header file is required by many components but is not available in the include path.
🪛 GitHub Actions: Docs
Core/include/Acts/Utilities/BinningType.hpp
[error] 62: Found ';' while parsing initializer list! (doxygen could be confused by a macro call without semicolon)
Core/include/Acts/Utilities/Logger.hpp
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
🔇 Additional comments (21)
Core/include/Acts/Utilities/Logger.hpp (2)
278-282: Breaking change to the interface, this introduces. Caution, you must exercise.By adding a pure virtual function
flushwith a new signature, existing derived classes may fail to compile, hmmm? Ensure backward compatibility, or document the breaking change clearly, you should.🧰 Tools
🪛 GitHub Actions: Docs
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
795-801: Constructor change, potentially breaking it is. Default parameter provides safety, but vigilance you must maintain.The addition of the
OutputModeparameter to theLoggerconstructor, with a default value, minimizes the risk of breaking existing code. However, derived classes and factory functions, check them you should, to ensure compatibility.Run this script to check for constructor usages without the default parameter:
✅ Verification successful
Safe the constructor change is, through factory function protection. Worry you need not.
The force of abstraction strong it is - getDefaultLogger() factory function, the primary path of Logger creation remains. Protected the existing code is, through this abstraction layer. Default parameter in constructor, additional safety it provides.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of the Logger constructor that may be affected. # Search for Logger constructor calls not using the default parameter. rg -t cpp -A 3 -e 'Logger\s*\('Length of output: 118045
🧰 Tools
🪛 GitHub Actions: Docs
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
Core/src/Utilities/BinningUtility.cpp (1)
19-29: Error handling, consider you must.In
to_json, if thebu.binningData()is empty, potential issues with serialization there may be. Ensure empty data structures are handled gracefully, you should.Test with an empty
BinUtilityobject, and observe the JSON output, you should.Core/include/Acts/Utilities/BinningType.hpp (1)
14-14: Include this header does, but verify its presence we must.Run this command to check the dependency:
✅ Verification successful
Hmmmm. Valid and well-managed, this dependency is. Worry not, young padawan.
The Force reveals that nlohmann/json is properly integrated into the project through CMake, with both system and bundled options available. In the Core utilities, this pattern of JSON integration, we see frequently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if nlohmann/json.hpp is available in the system find /usr/include -name "json.hpp" -path "*/nlohmann/*" || echo "Missing dependency"Length of output: 150
Script:
#!/bin/bash # Search for json.hpp files echo "=== Searching for json.hpp files ===" fd json.hpp # Search for nlohmann in CMake files echo -e "\n=== Searching for nlohmann in CMake files ===" rg -i "nlohmann" --glob "CMakeLists.txt" --glob "*.cmake" # Search for other files including nlohmann/json.hpp echo -e "\n=== Searching for other nlohmann/json.hpp includes ===" rg "nlohmann/json\.hpp"Length of output: 6426
🧰 Tools
🪛 GitHub Actions: Docs
[error] 62: Found ';' while parsing initializer list! (doxygen could be confused by a macro call without semicolon)
Core/include/Acts/Utilities/AxisFwd.hpp (2)
32-35: Wise implementation of AxisBoundaryType serialization, this is.Complete coverage of all enum values, you have achieved. Approve this change, I do.
70-73: Clear and concise AxisType serialization, implemented well it is.Match the enum values perfectly, these mappings do.
Core/include/Acts/Definitions/Algebra.hpp (1)
96-99: Elegant ADL serialization for Transform3, implemented it is.But verify the implementation in source file, we must.
Also applies to: 102-113
✅ Verification successful
Proper implementation found, it has been. Strong with the Force, this code is!
- Complete serialization logic in
Core/src/Definitions/Algebra.cpp, resides- Handles both rotation and translation with wisdom, it does
- Null checks and optimizations, present they are
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find implementation of Transform3 serialization rg -l "void Acts::to_json.*Transform3" || echo "Missing implementation" rg -l "void Acts::from_json.*Transform3" || echo "Missing implementation"Length of output: 149
Script:
#!/bin/bash # Examine the implementation of Transform3 serialization rg "void Acts::to_json.*Transform3|void Acts::from_json.*Transform3" -A 10 Core/src/Definitions/Algebra.cppLength of output: 1015
Core/src/Utilities/Logger.cpp (1)
106-106: Simple yet effective, this dummy implementation is.Correctly overrides the base class method, it does. Approve this change, I do.
Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp (1)
134-165: Well implemented comparison logic, this is.Thorough and precise, the isEqual function is. Good practices it follows:
- Detailed component-wise comparison
- Appropriate tolerance handling
- Clear error reporting
Tests/UnitTests/Core/Utilities/LoggerTests.cpp (2)
168-179: Wise use of RAII, this is!Clean and efficient, the implementation is. Automatic restoration of previous mode, it ensures.
193-210: Thorough testing of output modes, you have done!Well-structured tests for different output modes, these are. Verify debug level behavior correctly, they do.
Core/include/Acts/Geometry/Extent.hpp (1)
331-334: Clean and proper, these declarations are!Follow nlohmann::json conventions well, they do. Forward declarations wisely used, they are.
Tests/UnitTests/Core/Utilities/Range1DTests.cpp (1)
613-623: Well tested, the JSON serialization is!Proper floating-point comparisons used, they are. Round-trip serialization thoroughly verified, it is.
Core/include/Acts/Utilities/BinningData.hpp (1)
559-561: Consistent with the Force, these declarations are!Follow the same pattern as other JSON serializations, they do. Forward declarations properly used, they are.
Core/include/Acts/Utilities/RangeXD.hpp (1)
18-18: Approve the include addition, I do.Correctly placed, the nlohmann/json header is. After standard includes but before namespace declaration, it resides.
Core/src/Definitions/CMakeLists.txt (1)
1-4: Approve the CMake changes, I do.Clean and well-formatted, the target_sources command is. Addition of Algebra.cpp, proper it is.
Core/src/Utilities/CMakeLists.txt (1)
9-10: Approve the addition of new source files, I do.Properly placed and alphabetically ordered, the new source files are.
Core/CMakeLists.txt (1)
21-24: Hmm, a public dependency on nlohmann_json, I see.Wise decision it is, to make nlohmann_json a public dependency. Transitively available to all targets that link against ActsCore, it will be.
CMakeLists.txt (2)
39-39: Hmm, configuration options for nlohmann_json, modified they are.Default to system installation and explicit setup option added, I see. Align with other dependencies like Boost and Eigen3, this does. Wise choice for consistency, it is.
Also applies to: 93-93
313-320: Conditional block for nlohmann_json setup, added it is.Clear and consistent with other dependency setups, this implementation is. System version or bundled version, choose wisely it will.
Run this command to verify system package availability, we must:
✅ Verification successful
Verified, the implementation is. Strong with the Force, this code structure is!
Follows the sacred patterns of the project, it does:
- Version requirements clearly defined at 3.10.5, they are
- System and bundled paths, properly handled they are
- With other dependencies, consistent the structure is
Intentional choice to default to system version, wise decision it is. Trust in the Force and this implementation, we can!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if system nlohmann_json package is available pkg-config --exists nlohmann_json && echo "System package found" || echo "System package not found"Length of output: 195
Script:
#!/bin/bash # Check bundled nlohmann_json and similar dependency patterns echo "=== Checking bundled nlohmann_json ===" ls -la thirdparty/nlohmann_json 2>/dev/null || echo "Directory not found" echo -e "\n=== Checking CMake dependency patterns ===" rg "ACTS_USE_SYSTEM_" CMakeLists.txt -A 2 -B 2 echo -e "\n=== Checking version requirements ===" rg "_acts_nlohmanjson_version" CMakeLists.txt -A 2 -B 2Length of output: 3673
docs/getting_started.md (1)
271-271: Documentation updates, clear and precise they are.Well documented, the new configuration options are. Help users understand the changes, this will. Matches the implementation in CMake files, it does.
Also applies to: 314-314
| /// @param [in] lvl debug level of debug message | ||
| /// @param [in] input text of debug message | ||
| /// @param [in] args key value pairs to be added to the log message, see @ref |
There was a problem hiding this comment.
Mismatch in parameter name, there is. Correct it, you should.
In the documentation, the parameter is named 'input', but in the function signature, 'message' it is called. Consistency between them, maintain you must.
Apply this diff to fix the mismatch:
- /// @param [in] input text of debug message
+ /// @param [in] message text of debug message📝 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.
| /// @param [in] lvl debug level of debug message | |
| /// @param [in] input text of debug message | |
| /// @param [in] args key value pairs to be added to the log message, see @ref | |
| /// @param [in] lvl debug level of debug message | |
| /// @param [in] message text of debug message | |
| /// @param [in] args key value pairs to be added to the log message, see @ref |
🧰 Tools
🪛 GitHub Actions: Docs
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
| /// @param [in] lvl debug level of debug message | ||
| /// @param [in] input text of debug message | ||
| /// @param [in] args key value pairs to be added to the log message, see @ref | ||
| /// Acts::Logging::Literals::_key |
There was a problem hiding this comment.
Unresolved reference in documentation, hmmm?
The reference to Acts::Logging::Literals::_key in the @ref command, unresolved it is. Ensure that the reference is correct and exists.
Apply this diff to fix the reference:
- /// Acts::Logging::Literals::_key
+ /// `Acts::Logging::Literals::_key`📝 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.
| /// Acts::Logging::Literals::_key | |
| /// `Acts::Logging::Literals::_key` |
🧰 Tools
🪛 GitHub Actions: Docs
[error] 832: argument 'input' of command @param is not found in the argument list of Acts::Logger::log
[error] 835: unable to resolve reference to 'Acts::Logging::Literals::_key' for \ref command
[error] 832: Parameter 'message' is not documented in Acts::Logger::log
[error] 99: argument 'x' of command @param is not found in the argument list of ACTS_DEBUG
[error] 135: argument 'x' of command @param is not found in the argument list of ACTS_ERROR
[error] 147: argument 'x' of command @param is not found in the argument list of ACTS_FATAL
[error] 111: argument 'x' of command @param is not found in the argument list of ACTS_INFO
[error] 87: argument 'x' of command @param is not found in the argument list of ACTS_VERBOSE
[error] 123: argument 'x' of command @param is not found in the argument list of ACTS_WARNING
| void Acts::from_json(const nlohmann::json& j, Acts::BinUtility& bu) { | ||
| bu = Acts::BinUtility(); | ||
| if (j.find("transform") != j.end() && !j.at("transform").empty()) { | ||
| Acts::Transform3 trf = j.at("transform"); | ||
| bu = Acts::BinUtility(trf); | ||
| } | ||
| for (const auto& jdata : j.at("binningdata")) { | ||
| Acts::BinningData bd; | ||
| from_json(jdata, bd); | ||
| bu += Acts::BinUtility(bd); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exception safety, important it is.
In from_json, exceptions may be thrown if expected keys are missing or of incorrect types. Add checks or exception handling, you should, to make the code robust.
Apply this diff to enhance safety:
for (const auto& jdata : j.at("binningdata")) {
+ if (!jdata.is_object()) {
+ throw std::runtime_error("Invalid binning data format");
+ }
Acts::BinningData bd;📝 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.
| void Acts::from_json(const nlohmann::json& j, Acts::BinUtility& bu) { | |
| bu = Acts::BinUtility(); | |
| if (j.find("transform") != j.end() && !j.at("transform").empty()) { | |
| Acts::Transform3 trf = j.at("transform"); | |
| bu = Acts::BinUtility(trf); | |
| } | |
| for (const auto& jdata : j.at("binningdata")) { | |
| Acts::BinningData bd; | |
| from_json(jdata, bd); | |
| bu += Acts::BinUtility(bd); | |
| } | |
| } | |
| void Acts::from_json(const nlohmann::json& j, Acts::BinUtility& bu) { | |
| bu = Acts::BinUtility(); | |
| if (j.find("transform") != j.end() && !j.at("transform").empty()) { | |
| Acts::Transform3 trf = j.at("transform"); | |
| bu = Acts::BinUtility(trf); | |
| } | |
| for (const auto& jdata : j.at("binningdata")) { | |
| if (!jdata.is_object()) { | |
| throw std::runtime_error("Invalid binning data format"); | |
| } | |
| Acts::BinningData bd; | |
| from_json(jdata, bd); | |
| bu += Acts::BinUtility(bd); | |
| } | |
| } |
| auto rotation = t.rotation(); | ||
| if (rotation != Acts::RotationMatrix3::Identity()) { | ||
| std::array<double, 9> rdata = { | ||
| rotation(0, 0), rotation(0, 1), rotation(0, 2), | ||
| rotation(1, 0), rotation(1, 1), rotation(1, 2), | ||
| rotation(2, 0), rotation(2, 1), rotation(2, 2)}; | ||
| j["rotation"] = rdata; | ||
| } else { | ||
| j["rotation"] = nlohmann::json(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Rotation matrix comparison, precision it requires.
Comparing floating-point matrices using != may lead to precision issues. Use isApprox() with a tolerance, you should.
Apply this diff to correct the comparison:
- if (rotation != Acts::RotationMatrix3::Identity()) {
+ if (!rotation.isApprox(Acts::RotationMatrix3::Identity())) {📝 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 rotation = t.rotation(); | |
| if (rotation != Acts::RotationMatrix3::Identity()) { | |
| std::array<double, 9> rdata = { | |
| rotation(0, 0), rotation(0, 1), rotation(0, 2), | |
| rotation(1, 0), rotation(1, 1), rotation(1, 2), | |
| rotation(2, 0), rotation(2, 1), rotation(2, 2)}; | |
| j["rotation"] = rdata; | |
| } else { | |
| j["rotation"] = nlohmann::json(); | |
| } | |
| } | |
| auto rotation = t.rotation(); | |
| if (!rotation.isApprox(Acts::RotationMatrix3::Identity())) { | |
| std::array<double, 9> rdata = { | |
| rotation(0, 0), rotation(0, 1), rotation(0, 2), | |
| rotation(1, 0), rotation(1, 1), rotation(1, 2), | |
| rotation(2, 0), rotation(2, 1), rotation(2, 2)}; | |
| j["rotation"] = rdata; | |
| } else { | |
| j["rotation"] = nlohmann::json(); | |
| } | |
| } |
| NLOHMANN_JSON_SERIALIZE_ENUM(BinningValue, {{BinningValue::binX, "binX"}, | ||
| {BinningValue::binY, "binY"}, | ||
| {BinningValue::binZ, "binZ"}, | ||
| {BinningValue::binR, "binR"}, | ||
| {BinningValue::binPhi, "binPhi"}, | ||
| {BinningValue::binRPhi, "binRPhi"}, | ||
| {BinningValue::binH, "binH"}, | ||
| {BinningValue::binEta, "binEta"}, | ||
| {BinningValue::binMag, "binMag"}}) | ||
|
|
There was a problem hiding this comment.
Hmmmm. Missing semicolon after macro, causing Doxygen confusion this does.
Apply this fix, you must:
NLOHMANN_JSON_SERIALIZE_ENUM(BinningValue, {{BinningValue::binX, "binX"},
{BinningValue::binY, "binY"},
{BinningValue::binZ, "binZ"},
{BinningValue::binR, "binR"},
{BinningValue::binPhi, "binPhi"},
{BinningValue::binRPhi, "binRPhi"},
{BinningValue::binH, "binH"},
{BinningValue::binEta, "binEta"},
- {BinningValue::binMag, "binMag"}})
+ {BinningValue::binMag, "binMag"}});📝 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.
| NLOHMANN_JSON_SERIALIZE_ENUM(BinningValue, {{BinningValue::binX, "binX"}, | |
| {BinningValue::binY, "binY"}, | |
| {BinningValue::binZ, "binZ"}, | |
| {BinningValue::binR, "binR"}, | |
| {BinningValue::binPhi, "binPhi"}, | |
| {BinningValue::binRPhi, "binRPhi"}, | |
| {BinningValue::binH, "binH"}, | |
| {BinningValue::binEta, "binEta"}, | |
| {BinningValue::binMag, "binMag"}}) | |
| NLOHMANN_JSON_SERIALIZE_ENUM(BinningValue, {{BinningValue::binX, "binX"}, | |
| {BinningValue::binY, "binY"}, | |
| {BinningValue::binZ, "binZ"}, | |
| {BinningValue::binR, "binR"}, | |
| {BinningValue::binPhi, "binPhi"}, | |
| {BinningValue::binRPhi, "binRPhi"}, | |
| {BinningValue::binH, "binH"}, | |
| {BinningValue::binEta, "binEta"}, | |
| {BinningValue::binMag, "binMag"}}); |
🧰 Tools
🪛 GitHub Actions: Docs
[error] 62: Found ';' while parsing initializer list! (doxygen could be confused by a macro call without semicolon)
| @@ -23,6 +23,8 @@ | |||
| #include <Eigen/Geometry> | |||
| #endif | |||
|
|
|||
| #include <nlohmann/json.hpp> | |||
There was a problem hiding this comment.
Missing dependency, troubling this is.
Install nlohmann-json package in your build environment, you must. Add this to your system dependencies:
#!/bin/bash
# Check if package is available
if command -v apt-get &> /dev/null; then
apt-get install -y nlohmann-json-dev
elif command -v yum &> /dev/null; then
yum install -y nlohmann-json-devel
fi🧰 Tools
🪛 GitHub Actions: Checks
[error] 26-26: Missing dependency: nlohmann/json.hpp file not found. This header file is required by many components but is not available in the include path.
| const auto& jrange = j["range"]; | ||
|
|
||
| for (const auto& [key, value] : jrange.items()) { | ||
| BinningValue bval = binningValueFromName(key); | ||
| e.set(bval, value["min"], value["max"]); | ||
| } |
There was a problem hiding this comment.
Handle JSON parsing errors, you must.
Dangerous it is to assume JSON structure without validation. Add error handling, we should:
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["range"];
+ if (!jrange.is_object()) {
+ throw std::invalid_argument("'range' must be a JSON object");
+ }
for (const auto& [key, value] : jrange.items()) {📝 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.
| const auto& jrange = j["range"]; | |
| for (const auto& [key, value] : jrange.items()) { | |
| BinningValue bval = binningValueFromName(key); | |
| e.set(bval, value["min"], value["max"]); | |
| } | |
| if (!j.contains("range")) { | |
| throw std::invalid_argument("Missing required 'range' field in JSON"); | |
| } | |
| const auto& jrange = j["range"]; | |
| if (!jrange.is_object()) { | |
| throw std::invalid_argument("'range' must be a JSON object"); | |
| } | |
| for (const auto& [key, value] : jrange.items()) { | |
| BinningValue bval = binningValueFromName(key); | |
| e.set(bval, value["min"], value["max"]); | |
| } |
| bool isEqual(const BinningData& ba, const BinningData& bb, float tolerance) { | ||
| bool equalBool = (ba.type == bb.type) && (ba.option == bb.option) && | ||
| (ba.binvalue == bb.binvalue) && (ba.zdim == bb.zdim) && | ||
| (ba.subBinningAdditive == bb.subBinningAdditive); | ||
|
|
||
| BOOST_CHECK(equalBool); | ||
| bool equalRange = (std::abs(ba.min - bb.min) < tolerance) && | ||
| (std::abs(ba.max - bb.max) < tolerance) && | ||
| (std::abs(ba.step - bb.step) < tolerance); | ||
|
|
||
| BOOST_CHECK(equalRange); | ||
| bool euqalStructure = | ||
| (ba.subBinningData != nullptr) | ||
| ? isEqual(*ba.subBinningData, *bb.subBinningData, tolerance) | ||
| : (bb.subBinningData == nullptr); | ||
|
|
||
| BOOST_CHECK(euqalStructure); | ||
|
|
||
| bool equalBoundaries = (ba.boundaries().size() == bb.boundaries().size()); | ||
| if (equalBoundaries) { | ||
| for (std::size_t ib = 0; ib < ba.boundaries().size(); ++ib) { | ||
| equalBoundaries = | ||
| (std::abs(ba.boundaries()[ib] - bb.boundaries()[ib]) < tolerance); | ||
| if (!equalBoundaries) { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| BOOST_CHECK(equalBoundaries); | ||
|
|
||
| return equalBool && equalRange && euqalStructure; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the typo in variable name and improve floating-point comparison, we must.
Two improvements, I sense:
- A typo in 'euqalStructure', there is
- Direct floating-point comparison with tolerance, risky it can be
Apply these changes, you 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;For floating-point comparisons, a relative tolerance approach, consider:
- bool equalRange = (std::abs(ba.min - bb.min) < tolerance) &&
- (std::abs(ba.max - bb.max) < tolerance) &&
- (std::abs(ba.step - bb.step) < tolerance);
+ bool equalRange = (std::abs(ba.min - bb.min) <= tolerance * std::max(std::abs(ba.min), std::abs(bb.min))) &&
+ (std::abs(ba.max - bb.max) <= tolerance * std::max(std::abs(ba.max), std::abs(bb.max))) &&
+ (std::abs(ba.step - bb.step) <= tolerance * std::max(std::abs(ba.step), std::abs(bb.step)));📝 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.
| bool isEqual(const BinningData& ba, const BinningData& bb, float tolerance) { | |
| bool equalBool = (ba.type == bb.type) && (ba.option == bb.option) && | |
| (ba.binvalue == bb.binvalue) && (ba.zdim == bb.zdim) && | |
| (ba.subBinningAdditive == bb.subBinningAdditive); | |
| BOOST_CHECK(equalBool); | |
| bool equalRange = (std::abs(ba.min - bb.min) < tolerance) && | |
| (std::abs(ba.max - bb.max) < tolerance) && | |
| (std::abs(ba.step - bb.step) < tolerance); | |
| BOOST_CHECK(equalRange); | |
| bool euqalStructure = | |
| (ba.subBinningData != nullptr) | |
| ? isEqual(*ba.subBinningData, *bb.subBinningData, tolerance) | |
| : (bb.subBinningData == nullptr); | |
| BOOST_CHECK(euqalStructure); | |
| bool equalBoundaries = (ba.boundaries().size() == bb.boundaries().size()); | |
| if (equalBoundaries) { | |
| for (std::size_t ib = 0; ib < ba.boundaries().size(); ++ib) { | |
| equalBoundaries = | |
| (std::abs(ba.boundaries()[ib] - bb.boundaries()[ib]) < tolerance); | |
| if (!equalBoundaries) { | |
| break; | |
| } | |
| } | |
| } | |
| BOOST_CHECK(equalBoundaries); | |
| return equalBool && equalRange && euqalStructure; | |
| } | |
| bool isEqual(const BinningData& ba, const BinningData& bb, float tolerance) { | |
| bool equalBool = (ba.type == bb.type) && (ba.option == bb.option) && | |
| (ba.binvalue == bb.binvalue) && (ba.zdim == bb.zdim) && | |
| (ba.subBinningAdditive == bb.subBinningAdditive); | |
| BOOST_CHECK(equalBool); | |
| bool equalRange = (std::abs(ba.min - bb.min) <= tolerance * std::max(std::abs(ba.min), std::abs(bb.min))) && | |
| (std::abs(ba.max - bb.max) <= tolerance * std::max(std::abs(ba.max), std::abs(bb.max))) && | |
| (std::abs(ba.step - bb.step) <= tolerance * std::max(std::abs(ba.step), std::abs(bb.step))); | |
| BOOST_CHECK(equalRange); | |
| bool equalStructure = | |
| (ba.subBinningData != nullptr) | |
| ? isEqual(*ba.subBinningData, *bb.subBinningData, tolerance) | |
| : (bb.subBinningData == nullptr); | |
| BOOST_CHECK(equalStructure); | |
| bool equalBoundaries = (ba.boundaries().size() == bb.boundaries().size()); | |
| if (equalBoundaries) { | |
| for (std::size_t ib = 0; ib < ba.boundaries().size(); ++ib) { | |
| equalBoundaries = | |
| (std::abs(ba.boundaries()[ib] - bb.boundaries()[ib]) < tolerance); | |
| if (!equalBoundaries) { | |
| break; | |
| } | |
| } | |
| } | |
| BOOST_CHECK(equalBoundaries); | |
| return equalBool && equalRange && equalStructure; | |
| } |
This PR introduces structured logging.
This then looks something like:
There are three output modes, which can be configured on
Logger:Default:Plain:and
Structured:{"name":"LoggerName","level":"LEVEL","message":"Message","key1":"value","key2":"value"}Blocked by:
--- END COMMIT MESSAGE ---
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Release Notes
New Features
Transform3,Extent,BinUtility, andRange1DImprovements
nlohmann::jsonChanges
Testing