-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: restructure vortex_utils and add more C++ utils #10
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
- Coverage 90.54% 89.40% -1.15%
==========================================
Files 6 6
Lines 423 500 +77
Branches 53 142 +89
==========================================
+ Hits 383 447 +64
+ Misses 26 4 -22
- Partials 14 49 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the vortex_utils package to restructure C++ and Python utilities with improved organization, extended C++ functionality, and more fitting namespaces.
Key changes:
- Restructured test folders from
test/andtests/tocpp_test/andpy_test/for clarity - Extended C++ library with new mathematical utilities and QoS profile definitions
- Migrated from
vortex_utilstovortex::utilsnamespace hierarchy with separate modules for math, types, and qos_profiles
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| vortex_utils/qos_profiles.py | Adds Python QoS profile utility functions |
| src/math.cpp | New C++ math utilities replacing old cpp_utils.cpp |
| src/cpp_utils.cpp | Removed old C++ utilities file |
| include/vortex/utils/*.hpp | New header structure with separate math, types, and qos_profiles modules |
| cpp_test/ | Restructured C++ tests with comprehensive coverage |
| py_test/ | Restructured Python tests with updated file paths |
| CMakeLists.txt | Updated build configuration for new structure |
| README.md | Updated documentation reflecting new API |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cpp_test/test_math.cpp
Outdated
| } | ||
| } | ||
|
|
||
| // Test that a quaternion with flipped signs is correctly convverted to euler |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'convverted' to 'converted'.
| // Test that a quaternion with flipped signs is correctly convverted to euler | |
| // Test that a quaternion with flipped signs is correctly converted to euler |
cpp_test/test_math.cpp
Outdated
| Eigen::Quaterniond q{euler_to_quat(roll, pitch, yaw)}; | ||
| Eigen::Vector4d result{q.x(), q.y(), q.z(), q.w()}; | ||
| Eigen::Vector4d expected{0.0, 0.0, 0.0, 1.0}; | ||
| for (int i = 0; i < 3; ++i) { |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop should iterate through all 4 quaternion components (0 to 3), not just the first 3. The quaternion has x, y, z, w components.
| for (int i = 0; i < 3; ++i) { | |
| for (int i = 0; i < 4; ++i) { |
cpp_test/test_math.cpp
Outdated
| Eigen::Quaterniond q{euler_to_quat(roll, pitch, yaw)}; | ||
| Eigen::Vector4d result{q.x(), q.y(), q.z(), q.w()}; | ||
| Eigen::Vector4d expected{0.479, 0.0, 0.0, 0.877}; | ||
| for (int i = 0; i < 3; ++i) { |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop should iterate through all 4 quaternion components (0 to 3), not just the first 3. The quaternion has x, y, z, w components.
cpp_test/test_math.cpp
Outdated
| Eigen::Quaterniond q{euler_to_quat(roll, pitch, yaw)}; | ||
| Eigen::Vector4d result{q.x(), q.y(), q.z(), q.w()}; | ||
| Eigen::Vector4d expected{0.0, 0.479, 0.0, 0.877}; | ||
| for (int i = 0; i < 3; ++i) { |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop should iterate through all 4 quaternion components (0 to 3), not just the first 3. The quaternion has x, y, z, w components.
cpp_test/test_math.cpp
Outdated
| Eigen::Quaterniond q{euler_to_quat(roll, pitch, yaw)}; | ||
| Eigen::Vector4d result{q.x(), q.y(), q.z(), q.w()}; | ||
| Eigen::Vector4d expected{0.0, 0.0, 0.479, 0.877}; | ||
| for (int i = 0; i < 3; ++i) { |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop should iterate through all 4 quaternion components (0 to 3), not just the first 3. The quaternion has x, y, z, w components.
cpp_test/test_math.cpp
Outdated
| Eigen::Quaterniond q{euler_to_quat(roll, pitch, yaw)}; | ||
| Eigen::Vector4d result{q.x(), q.y(), q.z(), q.w()}; | ||
| Eigen::Vector4d expected{0.1675, 0.5709, 0.1675, 0.786}; | ||
| for (int i = 0; i < 3; ++i) { |
Copilot
AI
Oct 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop should iterate through all 4 quaternion components (0 to 3), not just the first 3. The quaternion has x, y, z, w components.
kluge7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
🎉 This PR is included in version 1.4.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Next thing to do after this PR is merged is to refactor the code in vortex-auv to use this library and enforce the new standard of using it.