feat: move spline to helper in Visualization #3950
Conversation
WalkthroughA new header file, Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)Examples/Python/src/Output.cpp (1)
Properly integrated, the new parameters for ObjSimHitWriter are. Control the spline interpolation behavior, they do:
Verify the parameter usage in the writer implementation, we must: ✅ Verification successfulLet's search for the configuration and implementation details to verify how these parameters are utilized, hmmmm. Verified, the parameters are! Properly implemented, they have been! In the ObjSimHitWriter implementation, see I do:
Hmmmm, wise implementation choices these are. The Force is strong with this code! 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for ObjSimHitWriter implementation to verify parameter usage
rg -A 10 "ObjSimHitWriter::ObjSimHitWriter"
Length of output: 926 Script: #!/bin/bash
# Search for ObjSimHitWriter configuration and implementation
rg -l "ObjSimHitWriter"
# Then examine the content of relevant files
fd "ObjSimHitWriter" --type f --exec cat {}
Length of output: 9252 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
![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
🧹 Outside diff range and nitpick comments (2)
Core/include/Acts/Visualization/Interpolation3D.hpp (2)
17-21: Documentation improvements, need we do.Incomplete, the documentation is. Template parameter constraints and return value documentation, missing they are.
Add these documentation improvements, you should:
/// @brief Helper function to interpolate points using a spline /// from Eigen /// -/// @tparam input_vector_type +/// @tparam input_vector_type Type that provides a head<3>() method returning a 3D vector /// @param inputs input vector points /// @param nPoints number of interpolation points +/// @pre inputs.size() >= 1 /// -/// @return std::vector<Acts::Vector3> interpolated points +/// @return Vector of interpolated 3D points. If nPoints < 2, returns copy of input points, +/// otherwise returns nPoints interpolated points along the spline
38-39: Magic number in spline degree, I sense.Hard-coded value 2 for spline degree, mysterious it is. As constant or parameter, define it we should.
Consider this improvement:
+ // Quadratic spline interpolation + constexpr int kSplineDegree = 2; Eigen::Spline<double, 3> spline3D = Eigen::SplineFitting<Eigen::Spline<double, 3>>::Interpolate(points, - 2); + kSplineDegree);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Core/include/Acts/Visualization/Interpolation3D.hpp(1 hunks)Examples/Io/Obj/src/ObjSimHitWriter.cpp(2 hunks)
🔇 Additional comments (1)
Examples/Io/Obj/src/ObjSimHitWriter.cpp (1)
14-14: Wise choice, moving interpolation to core library is.
Clean and efficient, the change is. But verify the impact on performance, we must.
Run performance verification, we shall:
Also applies to: 122-123
✅ Verification successful
The search results show only one file using the new interpolation, but to make a wise judgment about performance impact, see the actual usage context we must. Performance configurations, if any exist, helpful they would be.
Safe and sound, this change appears to be
Examined the code thoroughly, I have. The new interpolation implementation, properly integrated it is. Only in visualization context used it is, with configuration parameter nInterpolatedPoints controlling the density, hmm. No performance concerns do I sense, as the core functionality remains unchanged, it does.
- In
ObjSimHitWriter.cpp, wisely used the newActs::Interpolation3D::splineis - Configuration through
m_cfg.nInterpolatedPointsmaintained it is - No other custom implementations lingering in the codebase, there are
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all uses of interpolation to ensure consistent usage
rg -l "interpolatedPoints|Interpolation3D::spline" --type cpp
# Check if any custom interpolation implementations remain
ast-grep --pattern 'class $_ {
$$$
interpolate($$$) {
$$$
}
$$$
}'
Length of output: 184
Script:
#!/bin/bash
# Check the actual usage context of Interpolation3D
rg "Interpolation3D" -B 2 -A 2 --type cpp
# Look for any performance-related configurations or parameters
rg "interpolat.*param|spline.*param" --type cpp
Length of output: 1478
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp (2)
9-14: Documentation for the test suite, suggest I do.Clear the purpose is, but benefit from brief documentation about the test suite's objectives, it would. Help future Padawans understand the ways of the Force, it shall.
Add this documentation before the test suite:
+/** + * @brief Test suite for 3D interpolation functionality + * + * Validates the spline interpolation methods for 3D visualization + */ BOOST_AUTO_TEST_SUITE(Visualization)
21-26: More test cases, require we do.Wise the circle test is, but edge cases we must consider. Empty input vector and single point vector, test we should.
Add these test cases before the circle test:
// Test empty input { std::vector<Acts::Vector3> emptyInputs; auto emptyTrajectory = Acts::Interpolation3D::spline(emptyInputs, 5); BOOST_CHECK(emptyTrajectory.empty()); } // Test single point { std::vector<Acts::Vector3> singleInput = {Acts::Vector3(1., 1., 1.)}; auto singleTrajectory = Acts::Interpolation3D::spline(singleInput, 5); BOOST_CHECK_EQUAL(singleTrajectory.size(), 1); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Core/include/Acts/Visualization/Interpolation3D.hpp(1 hunks)Tests/UnitTests/Core/Visualization/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/include/Acts/Visualization/Interpolation3D.hpp
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.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)
🔇 Additional comments (2)
Tests/UnitTests/Core/Visualization/CMakeLists.txt (1)
3-3: Hmmmm, proper this addition is!
In harmony with other test declarations, this one stands. The Force, strong with this CMakeLists.txt file, it remains.
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp (1)
17-50: Verify the integration of our tests, we must.
Ensure that these tests run as part of the CI pipeline, we should.
✅ Verification successful
The previous results show the test is referenced in CMakeLists.txt, but let's verify the complete test configuration and build setup, hmm. Search for more details about test integration, we must.
Properly integrated, the test is, hmm.
Found in CMakeLists.txt the test registration is, through add_unittest(Interpolation3D Interpolation3DTests.cpp). Part of the Core Visualization test suite it is, yes. Through the standard test framework built and run it will be.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test integration in CI pipeline
# Check if test is included in CI configuration
gh api repos/acts-project/acts/contents/.github/workflows/builds.yml | jq -r '.content' | base64 -d | grep -A 5 "ctest"
# Check for other test references
rg -g "!*.{cpp}" "Interpolation3D"
Length of output: 530
Script:
#!/bin/bash
# Check CMake configuration for test integration
fd CMakeLists.txt Tests/ --exec cat {}
# Check if the test binary is actually built
fd -e cmake -e txt . --exec grep -l "Interpolation3D"
Length of output: 39928
🧰 Tools
🪛 cppcheck (2.10-2)
[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)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
Core/include/Acts/Visualization/Interpolation3D.hpp (4)
13-13: Cautious with unsupported dependencies, we must be.From Eigen's unsupported module, this spline implementation comes. Though work it may, fragile our foundation becomes. Consider documenting this dependency's risks, we should. Or perhaps, implement our own spline algorithm in the future, hmm?
35-38: Reserve space for output vector, we should.Wise it is to pre-allocate memory for known size. Performance gains, this brings.
trajectory_type output; +output.reserve(nPoints); if (inputsRaw.empty()) { return output; }Also applies to: 65-73
62-63: Document the spline degree choice, we must.Why degree 2 for spline fitting, chosen it was? Document this decision, we should, for future Padawans to understand.
+ // Degree 2 spline provides smooth interpolation while avoiding overfitting Eigen::Spline<double, 3> spline3D = Eigen::SplineFitting<Eigen::Spline<double, 3>>::Interpolate(points, 2);
80-90: Extract distance calculation, we shall.Repeated calculations, clarity they lack. A helper function, create we must.
+ auto squaredDistance = [](const auto& a, const auto& b) { + return (a[0] - b[0]) * (a[0] - b[0]) + + (a[1] - b[1]) * (a[1] - b[1]) + + (a[2] - b[2]) * (a[2] - b[2]); + }; std::sort(output.begin(), output.end(), - [&inputs](const auto& a, const auto& b) { - const auto ifront = inputs.front(); - double da2 = (a[0] - ifront[0]) * (a[0] - ifront[0]) + - (a[1] - ifront[1]) * (a[1] - ifront[1]) + - (a[2] - ifront[2]) * (a[2] - ifront[2]); - double db2 = (b[0] - ifront[0]) * (b[0] - ifront[0]) + - (b[1] - ifront[1]) * (b[1] - ifront[1]) + - (b[2] - ifront[2]) * (b[2] - ifront[2]); - return da2 < db2; + [&inputs, &squaredDistance](const auto& a, const auto& b) { + return squaredDistance(a, inputs.front()) < + squaredDistance(b, inputs.front()); });Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjSimHitWriter.hpp (1)
55-59: Document configuration impact, we must.Clear the purpose is, but consequences of changes, documented they are not.
/// Number of points to interpolated between hits to smooth the /// trajectory view in the obj file. + /// @note Lower values (default: 4) provide good visualization while + /// maintaining reasonable file sizes. std::size_t nInterpolatedPoints = 4; /// Keep the original hits in the trajectory file + /// @note When true, both interpolated and original points are included, + /// which may increase file size but preserves exact hit locations. bool keepOriginalHits = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Core/include/Acts/Visualization/Interpolation3D.hpp(1 hunks)Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjSimHitWriter.hpp(2 hunks)Examples/Io/Obj/src/ObjSimHitWriter.cpp(2 hunks)Examples/Python/src/Output.cpp(1 hunks)Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (4)
Examples/Io/Obj/src/ObjSimHitWriter.cpp (3)
14-14: Correct, this inclusion is!
The path to wisdom through Acts::Interpolation3D, it leads. Necessary for the spline functionality, this header is.
117-119: Hmmmm... Balanced, these bindings are!
In harmony with the Force, the new parameters are. Correctly exposed to Python, they have become.
115-125: 🛠️ Refactor suggestion
Validate the input parameters, we must!
Empty vector check, missing it is. Before interpolation begins, verify we should.
Apply this change, you must:
std::vector<Acts::Vector4> trajectory;
+ if (pHits.empty()) {
+ ACTS_WARNING("Empty hits vector for particle " << pId);
+ continue;
+ }
if (pHits.size() < 3 || m_cfg.nInterpolatedPoints == 0) {
trajectory = pHits;
} else {Examples/Python/src/Output.cpp (1)
117-119: Strong with the Force, these declarations are!
Perfectly aligned with the core changes, these bindings are. A bridge between C++ and Python, they build.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp (3)
20-54: More thorough testing, require we do.Good the circle verification is, but stronger assertions we need. Point ordering verify we must, and edge cases test we should.
Apply this diff, you should:
for (const auto& point : trajectory) { // Check the interpolated points are on the circle // with a tolerance of course CHECK_CLOSE_ABS(point.norm(), R, 0.1); // Verify points remain in the XY plane CHECK_CLOSE_ABS(point.z(), 0., 0.1); + // Verify point ordering around circle + if (&point != &trajectory.front()) { + const auto& prev = *(std::prev(&point)); + double currentAngle = std::atan2(point.y(), point.x()); + double prevAngle = std::atan2(prev.y(), prev.x()); + CHECK(currentAngle > prevAngle || + (currentAngle < 0 && prevAngle > M_PI/2)); + } } +// Test with different radii +std::vector<double> testRadii = {1., 100., 1000.}; +for (double testR : testRadii) { + inputs.clear(); + for (double phi = 0; phi < 2 * std::numbers::pi; + phi += std::numbers::pi / 4) { + inputs.push_back(Acts::Vector3(testR * cos(phi), + testR * sin(phi), 0.)); + } + trajectory = Acts::Interpolation3D::spline(inputs, 12); + for (const auto& point : trajectory) { + CHECK_CLOSE_ABS(point.norm(), testR, testR * 0.01); + } +}
67-67: Fix typo in comment, you must.A small typo I sense. "outpu" to "output" change it should be.
76-88: More error cases test, we should.Good start with single and double points, but more error cases handle we must.
Add these test cases, you should:
BOOST_CHECK_EQUAL(result.size(), 2); + +// Test with negative number of points +result = Acts::Interpolation3D::spline(inputs, -1); +BOOST_CHECK_EQUAL(result.size(), 2); + +// Test with zero points requested +result = Acts::Interpolation3D::spline(inputs, 0); +BOOST_CHECK_EQUAL(result.size(), 2); + +// Test with very large number of points +result = Acts::Interpolation3D::spline(inputs, 1000000); +BOOST_CHECK(result.size() > 2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (1)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp (1)
1-18: Proper structure and includes, I sense.
Well organized the file is, with necessary headers and proper namespace it has.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp (1)
76-88: More edge cases test, we must.Good start with single and double points you have, but more thorough testing needed it is. Additional cases and value verification, add we should.
Enhance the test case, like this you should:
BOOST_CHECK_EQUAL(result.size(), 2); + +// Verify point values preserved they are +CHECK_CLOSE_ABS(result[0][0], 0., 1e-6); +CHECK_CLOSE_ABS(result[0][1], 0., 1e-6); +CHECK_CLOSE_ABS(result[0][2], 0., 1e-6); +CHECK_CLOSE_ABS(result[1][0], 1., 1e-6); +CHECK_CLOSE_ABS(result[1][1], 1., 1e-6); +CHECK_CLOSE_ABS(result[1][2], 1., 1e-6); + +// Test with invalid input, handle it we must +std::vector<std::array<double, 3u>> empty_inputs; +auto empty_result = Acts::Interpolation3D::spline(empty_inputs, 10); +BOOST_CHECK(empty_result.empty());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (3)
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp (3)
1-15: Correct, the headers and license are, hmmmm.
Well-organized and properly structured, the file header is. All necessary components for testing, included they are.
20-54: 🛠️ Refactor suggestion
More thorough testing of circle properties, we need.
Good start this test is, but stronger assertions we must have. The path of the circle, more precisely verify we should.
Apply these changes, you must:
for (const auto& point : trajectory) {
// Check the interpolated points are on the circle
// with a tolerance of course
CHECK_CLOSE_ABS(point.norm(), R, 0.1);
// Verify points remain in the XY plane
CHECK_CLOSE_ABS(point.z(), 0., 0.1);
+ // Verify distance from origin matches radius in XY plane
+ CHECK_CLOSE_ABS(std::hypot(point.x(), point.y()), R, 0.1);
+ // Verify point ordering around circle
+ if (&point != &trajectory.front()) {
+ const auto& prev = *(std::prev(&point));
+ CHECK(std::hypot(point.x() - prev.x(), point.y() - prev.y()) > 0);
+ }
}56-74: 🛠️ Refactor suggestion
Verify the path of the parabola, we must.
Test the size and type you do, but confirm the shape of y = x², you do not. Missing, essential validation is.
Add these checks, you should:
BOOST_CHECK_EQUAL(trajectory.size(), 108);
+
+// Verify interpolated points follow the parabola path
+for (const auto& point : trajectory) {
+ // Check each point follows y = x²
+ CHECK_CLOSE_ABS(point[1], point[0] * point[0], 0.1);
+ // Verify z-coordinate remains constant
+ CHECK_CLOSE_ABS(point[2], 0., 0.1);
+}
|
This PR moves the spline-interpolation from
Examples/Plugins/ObjtoCore/Visualizationbecause it can be used in other visualisation areas as well.I've added also a UnitTest since this is from some
experimetnal/usupportedEigen directory, but it works nicely!--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Summary by CodeRabbit
New Features
ObjSimHitWriterwith new parameters for trajectory smoothing and original hit retention.Bug Fixes
Documentation
ObjSimHitWriterclass, clarifying their purpose and usage.