Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: move spline to helper in Visualization #3950

Merged
merged 9 commits into from
Dec 6, 2024

Conversation

asalzburger
Copy link
Contributor

@asalzburger asalzburger commented Dec 5, 2024

This PR moves the spline-interpolation from Examples/Plugins/Obj to Core/Visualization because it can be used in other visualisation areas as well.

I've added also a UnitTest since this is from some experimetnal/usupported Eigen directory, but it works nicely!

--- END COMMIT MESSAGE ---

Any further description goes here, @-mentions are ok here!

  • Use a conventional commits prefix: quick summary
    • We mostly use feat, fix, refactor, docs, chore and build types.
  • A milestone will be assigned by one of the maintainers

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced a new interpolation method for 3D points using splines, enhancing the accuracy of data representation.
    • Expanded configurability of the ObjSimHitWriter with new parameters for trajectory smoothing and original hit retention.
    • Added unit tests for the new interpolation functionality to ensure reliability.
  • Bug Fixes

    • Streamlined the interpolation process by replacing the previous method with a more efficient library function, improving maintainability.
  • Documentation

    • Enhanced documentation for configuration parameters in the ObjSimHitWriter class, clarifying their purpose and usage.

snapshot01

@asalzburger asalzburger added this to the next milestone Dec 5, 2024
Copy link

coderabbitai bot commented Dec 5, 2024

Walkthrough

A new header file, Interpolation3D.hpp, introduced in the Acts::Interpolation3D namespace, it was. A spline interpolation function for 3D points using Eigen, it provides. Removed, the existing interpolation method in ObjSimHitWriter.cpp was, replaced by a call to the new spline function, it was. The overall functionality preserved, while maintainability enhanced, hmmm.

Changes

File Change Summary
Core/include/Acts/Visualization/Interpolation3D.hpp Added: template <typename trajectory_type> trajectory_type spline(const trajectory_type& inputsRaw, std::size_t nPoints, bool keepOriginalHits = false) method.
Examples/Io/Obj/src/ObjSimHitWriter.cpp Removed: interpolatedPoints function; updated to use Acts::Interpolation3D::spline for interpolation; added #include "Acts/Visualization/Interpolation3D.hpp".
Examples/Io/Obj/include/ActsExamples/Io/Obj/ObjSimHitWriter.hpp Updated: nInterpolatedPoints default value changed from 10 to 4; added keepOriginalHits boolean flag.
Examples/Python/src/Output.cpp Updated: Method signatures for addOutput and ObjSimHitWriter to include new parameters.
Tests/UnitTests/Core/Visualization/CMakeLists.txt Added: add_unittest(Interpolation3D Interpolation3DTests.cpp) for new unit tests.
Tests/UnitTests/Core/Visualization/Interpolation3DTests.cpp Added: New test suite and cases for spline functionality, validating output sizes and accuracy.

Possibly related PRs

  • feat: introduction obj simhit writer #3180: The changes in this PR introduce the ObjSimHitWriter class, which utilizes the new spline function from Interpolation3D.hpp for interpolating points, directly linking it to the main PR's addition of the spline functionality.
  • refactor: Reworked HitSelector in Examples #3836: This PR enhances the hit selection process, which could indirectly relate to how hits are processed and potentially interpolated in the context of the ObjSimHitWriter.

Suggested reviewers

  • benjaminhuth

Poem

In the realm of code, a new path we weave,
With splines and points, oh, how we believe!
A helper emerges, swift and bright,
Making our logic shine with delight.
From old to new, the journey is clear,
In the dance of data, we cheer and we cheer! 🎉


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between afb4002 and 670bc9b.

📒 Files selected for processing (1)
  • Examples/Python/src/Output.cpp (1 hunks)
🔇 Additional comments (1)
Examples/Python/src/Output.cpp (1)

114-116: Hmmmm, wise additions to the Python bindings, these parameters are!

Properly integrated, the new parameters for ObjSimHitWriter are. Control the spline interpolation behavior, they do:

  • momentumThreshold and momentumThresholdTraj: Filter tracks by momentum, they will
  • nInterpolatedPoints: Control the interpolation granularity, it does
  • keepOriginalHits: Preserve the original hit points, it shall

Verify the parameter usage in the writer implementation, we must:

✅ Verification successful

Let'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:

  • momentumThreshold and momentumThresholdTraj: Filter hits by momentum, they do indeed
  • nInterpolatedPoints: Used in spline interpolation for trajectory smoothing, it is
  • keepOriginalHits: Controls original hit preservation in trajectory interpolation, as intended it does

Hmmmm, wise implementation choices these are. The Force is strong with this code!

🏁 Scripts executed

The 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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@asalzburger asalzburger requested a review from AJPfleger December 5, 2024 08:04
@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module labels Dec 5, 2024
@asalzburger asalzburger marked this pull request as draft December 5, 2024 08:06
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 7efad56 and d2e0450.

📒 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 new Acts::Interpolation3D::spline is
  • Configuration through m_cfg.nInterpolatedPoints maintained 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

Core/include/Acts/Visualization/Interpolation3D.hpp Outdated Show resolved Hide resolved
Examples/Io/Obj/src/ObjSimHitWriter.cpp Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Dec 5, 2024

📊: Physics performance monitoring for 670bc9b

Full contents

physmon summary

@asalzburger asalzburger marked this pull request as ready for review December 5, 2024 08:20
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between d2e0450 and a3f9746.

📒 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)

@asalzburger asalzburger marked this pull request as draft December 5, 2024 08:45
@asalzburger asalzburger marked this pull request as ready for review December 5, 2024 10:13
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between a3f9746 and 13e263f.

📒 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.

asalzburger and others added 2 commits December 5, 2024 11:52
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 13e263f and c68bcca.

📒 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)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between c68bcca and afb4002.

📒 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);
+}

Copy link

sonarcloud bot commented Dec 6, 2024

@kodiakhq kodiakhq bot merged commit a646e06 into acts-project:main Dec 6, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Component - Examples Affects the Examples module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants