feat: proto axis json converter #4038
Conversation
WalkthroughEnhancements to the ACTS project's axis management system, this pull request introduces. A new Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (18)
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 (10)
Core/src/Utilities/IAxis.cpp (2)
34-43: Handle unknown boundary types, you must carefully.In the default case, a logic error you throw. Consider using an assertion or ensuring all boundary types covered are.
Apply this diff to handle the default case more explicitly:
switch (aBoundaryType) { case Open: return std::make_unique<Axis<Equidistant, Open>>(min, max, nbins); case Bound: return std::make_unique<Axis<Equidistant, Bound>>(min, max, nbins); case Closed: return std::make_unique<Axis<Equidistant, Closed>>(min, max, nbins); + default: + throw std::invalid_argument("Invalid AxisBoundaryType provided."); }
62-71: Default case handling, improve you should.In the switch statement, a logic error you throw. Better to validate
aBoundaryTypebefore the switch, it may be.Consider this change:
if (aBoundaryType != AxisBoundaryType::Open && aBoundaryType != AxisBoundaryType::Bound && aBoundaryType != AxisBoundaryType::Closed) { throw std::invalid_argument("Invalid AxisBoundaryType provided."); } switch (aBoundaryType) { case Open: return std::make_unique<Axis<Variable, Open>>(edges); case Bound: return std::make_unique<Axis<Variable, Bound>>(edges); case Closed: return std::make_unique<Axis<Variable, Closed>>(edges); }Core/include/Acts/Utilities/ProtoAxis.hpp (1)
54-54: Auto-range binning, clarity needed there is.Placeholder constructor you have for auto-range binning. Ensure that users aware are, that implementation provided is not.
Add a comment to indicate unimplemented status:
/// @note that auto-range is only supported for equidistant binning +/// @warning Auto-range binning not yet implemented. ProtoAxis(AxisDirection aDir, AxisBoundaryType abType, std::size_t nbins);Plugins/Json/include/Acts/Plugins/Json/ProtoAxisJsonConverter.hpp (1)
31-31: Documentation clarity, improve you can.Explain the expected format of the JSON object, you should. Helps users understand how to use
fromJson, it does.Add details about the JSON structure in the documentation comment.
Plugins/Json/src/ProtoAxisJsonConverter.cpp (1)
15-21: Hmmmm, robust this implementation is, but safer it could be!Check for null pointer or invalid state, we must. Add validation before accessing properties, I suggest.
nlohmann::json Acts::ProtoAxisJsonConverter::toJson(const Acts::ProtoAxis& pa) { nlohmann::json j; + if (!pa.getAxis()) { + throw std::invalid_argument("Invalid ProtoAxis with null axis"); + } j["axis_dir"] = pa.getAxisDirection(); j["axis"] = AxisJsonConverter::toJson(pa.getAxis()); j["autorange"] = pa.isAutorange(); return j; }Core/src/Utilities/ProtoAxis.cpp (2)
11-22: Clear in purpose this function is, but more descriptive in error, it could be!Add valid combinations to error message, help future developers it will.
void checkConsistency(Acts::AxisDirection aDir, Acts::AxisBoundaryType abType) { if (abType == Acts::AxisBoundaryType::Closed && aDir != Acts::AxisDirection::AxisPhi && aDir != Acts::AxisDirection::AxisRPhi) { std::string msg = - "ProtoBinning: Invalid axis boundary type 'Closed' for direction '"; - msg += axisDirectionName(aDir) + "'."; + "ProtoBinning: Invalid axis boundary type 'Closed' for direction '" + + axisDirectionName(aDir) + "'. Closed boundary type is only valid for " + + "AxisPhi and AxisRPhi directions."; throw std::invalid_argument(msg); } }
56-73: Readable output this provides, but modernize it we could!Consider using std::format (C++20) or fmt library for cleaner string formatting, I suggest.
std::string Acts::ProtoAxis::toString() const { - std::stringstream ss; - ss << "ProtoAxis: " << getAxis().getNBins() << " bins in " - << axisDirectionName(m_axisDir); - ss << (getAxis().getType() == AxisType::Variable ? ", variable " - : ", equidistant "); + std::string axisType = getAxis().getType() == AxisType::Variable ? + "variable" : "equidistant"; + std::string rangeStr; if (!m_autorange) { const auto& edges = getAxis().getBinEdges(); - ss << "within [" << edges.front() << ", " << edges.back() << "]"; + rangeStr = std::format("within [{}, {}]", edges.front(), edges.back()); } else { - ss << "within automatic range"; + rangeStr = "within automatic range"; } - return ss.str(); + return std::format("ProtoAxis: {} bins in {}, {} {}", + getAxis().getNBins(), + axisDirectionName(m_axisDir), + axisType, + rangeStr); }Tests/UnitTests/Plugins/Json/ProtoAxisJsonConverterTests.cpp (1)
21-84: Good test coverage you have, but stronger it could become!Add test cases for error scenarios and edge cases, we should:
- Invalid JSON format
- Missing required fields
- Invalid axis combinations
Example test case to add:
BOOST_AUTO_TEST_CASE(InvalidProtoAxisJsonConversion) { using enum Acts::AxisBoundaryType; using enum Acts::AxisDirection; nlohmann::json invalidJson; // Empty JSON BOOST_CHECK_THROW(Acts::ProtoAxisJsonConverter::fromJson(invalidJson), nlohmann::json::exception); // Missing required fields invalidJson["axis_dir"] = AxisX; BOOST_CHECK_THROW(Acts::ProtoAxisJsonConverter::fromJson(invalidJson), nlohmann::json::exception); }Core/include/Acts/Utilities/IAxis.hpp (1)
65-85: Factory methods, well-designed they are! But documentation, improve we must.Add
@throwsdocumentation for error cases in factory methods, help users handle exceptions it will./// Centralized axis factory for equidistant binning /// /// @param aBoundaryType the axis boundary type /// @param min the minimum edge of the axis /// @param max the maximum edge of the axis /// @param nbins the number of bins /// + /// @throws std::invalid_argument if min >= max or nbins == 0 /// @return a unique pointer to the axis static std::unique_ptr<IAxis> create(AxisBoundaryType aBoundaryType, double min, double max, std::size_t nbins); /// Centralized axis factory for variable binning /// /// @param aBoundaryType the axis boundary type /// @param edges are the bin edges /// + /// @throws std::invalid_argument if edges is empty or not strictly increasing /// @return a unique pointer to the axis static std::unique_ptr<IAxis> create(AxisBoundaryType aBoundaryType, const std::vector<double>& edges);Tests/UnitTests/Core/Utilities/AxesTests.cpp (1)
595-636: Factory method tests, strong they are.Cover all axis types and boundary conditions, these tests do. Invalid cases well-tested, they are. But missing test for memory management of unique pointers, I sense.
Add test to verify proper cleanup of unique pointers, you should:
// Test memory management auto axis = IAxis::create(Bound, 0.0, 10., 10); BOOST_CHECK_NO_THROW(axis.reset());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Core/include/Acts/Utilities/IAxis.hpp(3 hunks)Core/include/Acts/Utilities/ProtoAxis.hpp(1 hunks)Core/src/Utilities/CMakeLists.txt(1 hunks)Core/src/Utilities/IAxis.cpp(1 hunks)Core/src/Utilities/ProtoAxis.cpp(1 hunks)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/Core/Utilities/AxesTests.cpp(1 hunks)Tests/UnitTests/Core/Utilities/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/Utilities/ProtoAxisTests.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/Core/Utilities/ProtoAxisTests.cpp
[error] 17-17: 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/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 (2)
- GitHub Check: merge-sentinel
- GitHub Check: macos
🔇 Additional comments (16)
Core/src/Utilities/IAxis.cpp (2)
16-44: Proper input validation, you have implemented.Validates the axis range and number of bins, your code does. Throws exceptions when inputs invalid are. Good practice, this is.
46-72: Edge sorting validation, well done it is.Checks for at least two edges and that they are sorted, your code does. Exception thrown when invalid inputs detected are. Excellent.
Core/include/Acts/Utilities/ProtoAxis.hpp (3)
35-35: Const correctness, consider you should.Pass the
edgesvector by const reference, you do. Good.
60-60: Defaulted move operations, wise choice it is.Allows efficient resource management, this does.
102-108: Template usage, correct it is.Creates grid with the appropriate axis type, your code does. Ensures compatibility and efficiency.
Plugins/Json/include/Acts/Plugins/Json/ProtoAxisJsonConverter.hpp (1)
26-26: Function naming, consistency important it is.
toJsonfunction you have. Ensure consistent naming with other JSON converters in the codebase, you must.Verify that naming conventions aligned are. If different, consider renaming to match.
Core/src/Utilities/ProtoAxis.cpp (2)
24-42: Well-crafted these constructors are! Approve them, I do.Proper validation and initialization, they maintain.
44-54: Simple yet effective, these getters are!Clear in purpose and implementation, they remain.
Core/include/Acts/Utilities/IAxis.hpp (1)
24-25: Wise decision, adding virtual destructor is! Prevent memory leaks, it will.Proper cleanup of derived classes through base pointer, this ensures.
Tests/UnitTests/Core/Utilities/ProtoAxisTests.cpp (3)
19-93: Comprehensive test coverage for EquidistantProtoAxis, I see.Strong with the Force, these tests are. Cover direct access, IAxis interface, grid creation, and invalid cases, they do. Well-structured and thorough, the test case is.
95-126: Test AutorangeProtoAxis well, you do.Validate autorange functionality and boundary types correctly, this test does. Clear and focused, the assertions are.
128-178: Test VariableProtoAxis thoroughly, you have.Cover all aspects of variable axis behavior, these tests do. Invalid cases for different axis directions, they check. Comprehensive validation of edge cases, I sense.
Core/src/Utilities/CMakeLists.txt (1)
10-12: Added source files correctly, you have.In proper order and alignment with existing files, the additions are.
Tests/UnitTests/Plugins/Json/CMakeLists.txt (1)
13-13: Added unit test correctly, you have.Follow the established pattern of test declarations, this addition does.
Plugins/Json/CMakeLists.txt (1)
18-18: Properly integrated, the new converter file is, hmmmm.In the correct alphabetical order, placed it is. With the existing JSON converter components, aligned well it stands.
Tests/UnitTests/Core/Utilities/CMakeLists.txt (1)
30-30: Wisely placed, the new test file is, yes.In harmony with other tests it stands, alphabetically ordered it remains. The Force is strong with this one.
github-actions
bot
added
Component - Core
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
|
Superseded. |
This PR builds on top of #4034 and adds support for JSON/IO for the
ProtoAxis.It also adds the unit test for reading and writing the ProtoAxis, which largely uses the already existing Axis Json support.
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Release Notes
New Features
IAxisinstances with equidistant and variable binning.ProtoAxisclass for flexible axis configuration.ProtoAxis.Improvements
Testing
IAxis,ProtoAxis, and JSON conversion functionality.