feat: adding json conversion and unit test for ProtoAxis
#4045
Conversation
WalkthroughA new JSON converter for ProtoAxis, discovered we have! In the ACTS project's Plugins/Json directory, changes have emerged. A comprehensive implementation for serializing and deserializing ProtoAxis objects using the nlohmann::json library, this pull request brings. Unit tests accompany the implementation, ensuring correctness of the conversion process, they do. Changes
Sequence DiagramsequenceDiagram
participant PA as ProtoAxis
participant JC as JsonConverter
participant JSON as JSON Object
PA->>JC: Convert to JSON
JC-->>JSON: Serialize properties
JSON->>JC: Return JSON representation
JC-->>PA: Validate conversion
JSON->>JC: Deserialize
JC-->>PA: Reconstruct ProtoAxis
JC->>PA: Verify original properties
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp (2)
43-63: Additional test scenarios, consider you should.Good coverage of basic autorange functionality, you have. But edge cases, test we must - negative bin counts and extreme values, important they are.
// Add these test cases Acts::ProtoAxis epaEdge1(AxisX, Bound, 1); // Minimum bins Acts::ProtoAxis epaEdge2(AxisX, Bound, std::numeric_limits<int>::max()); // Maximum bins
70-71: Misleading, this comment is. Fix it, we must.The comment speaks of equidistant axis, yet variable binning the code implements. Clarity in documentation, essential it is.
- // Bound, equidistant axis + // Bound, variable-binned axisPlugins/Json/include/Acts/Plugins/Json/ProtoAxisJsonConverter.hpp (2)
16-22: Fix the typo in documentation, you must. Clarify the explanation, you should.Spotted issues in the documentation, I have:
- "nemclature" -> "nomenclature", the correct spelling is
- "to to" -> "due to", the proper wording should be
- /// This uses a custom API and nemclature as it would - /// otherwise require the ProtoAxis to have a default - /// constructor which is disallowed to to the unique_ptr + /// This uses a custom API and nomenclature as it would + /// otherwise require the ProtoAxis to have a default + /// constructor which is disallowed due to the unique_ptr
23-32: Document the exceptions, we should.Well-structured the functions are, but missing information about potential exceptions, I sense. Document what exceptions these functions may throw, we must.
/// @brief Write the ProtoAxis to a json object /// /// @param pa the proto axis to be written out + /// @throws nlohmann::json::exception if serialization fails nlohmann::json toJson(const ProtoAxis& pa); /// Create a ProtoAxis from a json object /// /// @param j the json object to be read from + /// @throws nlohmann::json::exception if json format is invalid + /// @throws std::runtime_error if required fields are missing Acts::ProtoAxis fromJson(const nlohmann::json& j);Plugins/Json/src/ProtoAxisJsonConverter.cpp (1)
15-21: Document the ways of the Force, we must.Missing documentation for this function, I sense. Help future Padawans understand its purpose and usage, we should.
Add this documentation before the function:
+/** + * @brief Convert a ProtoAxis to its JSON representation + * @param pa The ProtoAxis to convert + * @return JSON object containing the axis configuration + */ nlohmann::json Acts::ProtoAxisJsonConverter::toJson(const Acts::ProtoAxis& pa) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Plugins/Json/CMakeLists.txt(1 hunks)Plugins/Json/include/Acts/Plugins/Json/ProtoAxisJsonConverter.hpp(1 hunks)Plugins/Json/src/ProtoAxisJsonConverter.cpp(1 hunks)Tests/UnitTests/Plugins/Json/CMakeLists.txt(1 hunks)Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp
[error] 19-19: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (7)
Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp (1)
19-41: Well structured, this test case is!Comprehensive testing of equidistant ProtoAxis serialization and deserialization, I see. All essential properties verified are - direction, axis type, autorange status, and string representation. Pleased with this implementation, I am.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 19-19: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
Tests/UnitTests/Plugins/Json/CMakeLists.txt (1)
13-13: In proper order, this test addition is.Follows the established pattern, it does. Alphabetically placed and properly configured, the new test is.
Plugins/Json/CMakeLists.txt (1)
18-18: Correctly integrated, this new source file is.In harmony with existing configuration, the ProtoAxisJsonConverter stands. Dependencies and build settings, properly maintained they are.
Plugins/Json/include/Acts/Plugins/Json/ProtoAxisJsonConverter.hpp (2)
1-8: Proper license and copyright information, I sense.In order, all things are. The Mozilla Public License and copyright notice, correctly placed they are.
11-14: Verify the path of ActsJson.hpp, we must.Correct the includes seem, but confirm the existence of ActsJson.hpp in the specified path, we should.
✅ Verification successful
Correct the path is, young padawan!
Located at its rightful place, ActsJson.hpp is. The Force flows strong through this include statement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ActsJson.hpp in the repository fd --type f "ActsJson.hpp$"Length of output: 81
Plugins/Json/src/ProtoAxisJsonConverter.cpp (2)
1-14: Proper organization of includes and license header, I sense.Well-structured and complete, the foundation of this file is. The Force is strong with the licensing and dependencies.
1-41: Verify the ways of JSON conversion, we must.Strong in the Force this implementation is, but test it thoroughly we should. Run the following script to verify JSON conversion with various axis configurations:
✅ Verification successful
Hmmmm... Safe and functional your code is, but stronger it could become.
Through the Force, improvements I sense:
- Use
at()consistently for all JSON access, safer from exceptions it is- Add explicit error handling, protect against the Dark Side of malformed input you must
Yet verified your implementation is, for all configurations it handles:
- Equidistant axes
- Autorange capabilities
- Variable binning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases to ensure comprehensive coverage echo "Searching for ProtoAxis test cases..." rg -l "ProtoAxis.*Test" # Look for other JSON converters to ensure consistent patterns echo "Checking consistency with other JSON converters..." rg -l "JsonConverter" --type cppLength of output: 4273
Script:
#!/bin/bash # Check test implementation details echo "=== ProtoAxis Test Implementation ===" rg -A 10 "TEST" Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp # Check error handling patterns in other converters echo "=== Error Handling Patterns ===" rg "throw|at\(" "Plugins/Json/*JsonConverter.cpp" -A 2Length of output: 1574
Script:
#!/bin/bash # Check error handling in ProtoAxisJsonConverter echo "=== ProtoAxis Error Handling ===" rg "throw|at\(" Plugins/Json/src/ProtoAxisJsonConverter.cpp -A 2 # Check implementation of similar converters for validation patterns echo "=== Similar Converter Patterns ===" rg "at\(" Plugins/Json/src/GridJsonConverter.cpp -A 2Length of output: 2422
Plugins/Json/include/Acts/Plugins/Json/ProtoAxisJsonConverter.hpp
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp (2)
21-41: Additional test cases, we must consider!Strong the test case is, but stronger it could become! Consider these scenarios, you should:
- Negative values for bounds
- Zero-width axis testing
- Invalid bin count handling
Help you implement these additional test cases, shall I?
65-84: More complex variable configurations, test we should!Simple the current test is. Expand it with these scenarios, we must:
- Non-uniform bin sizes
- Large number of bins
- Ascending and descending bin arrangements
// Example test to add: std::vector<float> edges = {0.0, 0.1, 0.3, 0.6, 1.0}; // Non-uniform bins Acts::ProtoAxis vpab(AxisX, Bound, edges); nlohmann::json jProtoAxis = Acts::ProtoAxisJsonConverter::toJson(vpab); Acts::ProtoAxis vpabRead = Acts::ProtoAxisJsonConverter::fromJson(jProtoAxis); BOOST_CHECK_EQUAL(vpabRead.getBinEdges(), vpab.getBinEdges());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Json/src/ProtoAxisJsonConverter.cpp(1 hunks)Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/Json/src/ProtoAxisJsonConverter.cpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp
[error] 19-19: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: macos
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: build_debug
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
🔇 Additional comments (1)
Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp (1)
1-18: Proper organization of includes and licensing, I sense!Well-structured the header section is, with all necessary components present. The Force is strong with this one!
asalzburger
requested review from
AJPfleger
and removed request for
paulgessinger
|
Hey @AJPfleger - little PR which should be uncontroversial |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp (2)
21-41: Additional test scenarios, we must consider!Good, the basic conversion test is. But strengthen it further with edge cases, we must:
- Negative range values
- Very large numbers
- Special floating-point values (infinity, NaN)
// Additional test cases to add: Acts::ProtoAxis epabNeg(AxisX, Bound, -10.0, -1.0, 10); Acts::ProtoAxis epabLarge(AxisX, Bound, 1e6, 1e7, 10); Acts::ProtoAxis epabSpecial(AxisX, Bound, -std::numeric_limits<double>::infinity(), std::numeric_limits<double>::infinity(), 10);
86-153: More validation scenarios, require we do!Well-tested, the basic error cases are. But expand our test coverage, we should:
- Missing required JSON fields
- Invalid axis direction values
- Invalid boundary type values
- Invalid axis type values
// Additional invalid test cases to add: nlohmann::json jMissingField = { {"axis_dir", "AxisX"}, {"autorange", false} // Missing "axis" field }; nlohmann::json jInvalidAxisDir = jValidEq; jInvalidAxisDir["axis_dir"] = "InvalidAxis"; nlohmann::json jInvalidBoundary = jValidEqAxis; jInvalidBoundary["boundary_type"] = "InvalidBoundary"; BOOST_CHECK_THROW(Acts::ProtoAxisJsonConverter::fromJson(jMissingField), std::invalid_argument); BOOST_CHECK_THROW(Acts::ProtoAxisJsonConverter::fromJson(jInvalidAxisDir), std::invalid_argument); BOOST_CHECK_THROW(Acts::ProtoAxisJsonConverter::fromJson(jInvalidBoundary), std::invalid_argument);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Plugins/Json/include/Acts/Plugins/Json/ProtoAxisJsonConverter.hpp(1 hunks)Plugins/Json/src/ProtoAxisJsonConverter.cpp(1 hunks)Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Plugins/Json/include/Acts/Plugins/Json/ProtoAxisJsonConverter.hpp
- Plugins/Json/src/ProtoAxisJsonConverter.cpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp
[error] 19-19: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
🔇 Additional comments (2)
Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp (2)
1-18: Well-organized, the includes are!Proper license header and necessary includes for testing framework and functionality, you have included. Pleased with this organization, I am.
43-63: Test the dynamic behavior of autorange, we must!Missing, the tests for dynamic range adjustment are. Add tests for:
- Range expansion behavior
- Initial state validation
- Edge case handling when range updates occur
// Example test to add: Acts::ProtoAxis epa(AxisX, Bound, 10); // Simulate range updates epa.extend(-5.0); epa.extend(15.0); nlohmann::json jProtoAxis = Acts::ProtoAxisJsonConverter::toJson(epa); Acts::ProtoAxis epaRead = Acts::ProtoAxisJsonConverter::fromJson(jProtoAxis); BOOST_CHECK_EQUAL(epaRead.getMin(), epa.getMin()); BOOST_CHECK_EQUAL(epaRead.getMax(), epa.getMax());
|
acts-project-service
added
the
Breaks Athena build
This PR adds json writing/reading to the new ProtoAxis. It supersedes #4038
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
New Features
Tests