-
Notifications
You must be signed in to change notification settings - Fork 115
Exponantial filter refactoring #493
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
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 exponential filter implementation to use a common backend filter from control_toolbox instead of maintaining separate logic. The refactoring introduces support for additional data types including std::vector<double> and geometry_msgs::msg::WrenchStamped.
- Adds a new
ExponentialFilterclass incontrol_toolboxwith template support for multiple data types - Updates
control_filters::ExponentialFilterto delegate filtering operations to thecontrol_toolboximplementation - Expands test coverage with comprehensive tests for the new data types and edge cases
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| control_toolbox/include/control_toolbox/exponential_filter.hpp | New template-based exponential filter implementation with support for multiple data types |
| control_toolbox/include/control_filters/exponential_filter.hpp | Refactored to use control_toolbox backend with specialized templates for WrenchStamped and vector |
| control_toolbox/src/control_filters/exponential_filter.cpp | Added plugin exports for new supported data types |
| control_toolbox/test/control_filters/test_exponential_filter.cpp | Comprehensive test suite covering all supported data types and edge cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
control_toolbox/test/control_filters/test_exponential_filter.cpp
Outdated
Show resolved
Hide resolved
control_toolbox/test/control_filters/test_exponential_filter.cpp
Outdated
Show resolved
Hide resolved
control_toolbox/test/control_filters/test_exponential_filter.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #493 +/- ##
==========================================
+ Coverage 83.01% 83.73% +0.71%
==========================================
Files 29 30 +1
Lines 1967 2078 +111
Branches 110 112 +2
==========================================
+ Hits 1633 1740 +107
Misses 268 268
- Partials 66 70 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
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.
Thanks for the contribution. I think this can be simplified a bit, but maybe I've overseen something.
Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
|
@christophfroehlich Sorry for late response uncontrollabe delay. I copied the low_pass_filter structure. Seems like those specializations are unnecessary . Thanks for the review! |
|
no worries, thanks for coming back on this. I have removed the specialization in the master branch already, have a look. |
Signed-off-by: silanus23 <berkantali23@outlook.com>
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.
Thanks for the follow-up, there is only one open conversation left.
Signed-off-by: silanus23 <berkantali23@outlook.com>
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
control_toolbox/test/control_filters/test_exponential_filter.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Some thoughts about filters::exponentialSmoothing, what do you think?
|
|
||
| #include "control_toolbox/exponential_filter.hpp" | ||
| #include "control_toolbox/exponential_filter_parameters.hpp" | ||
| #include "control_toolbox/filters.hpp" |
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.
| #include "control_toolbox/filters.hpp" |
This is not needed any more here, right?
| Traits::assign(storage_in, data_in); | ||
|
|
||
| // Exponential filter update: y[n] = α * x[n] + (1-α) * y[n-1] | ||
| old_value_ = storage_in * alpha_ + old_value_ * (1.0 - alpha_); |
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.
but here we could use filters::exponentialSmoothing if we also change that to a templated method, right?
control_toolbox/control_toolbox/include/control_toolbox/filters.hpp
Lines 43 to 47 in d12a587
| static inline double exponentialSmoothing( | |
| double current_raw_value, double last_smoothed_value, double alpha) | |
| { | |
| return alpha * current_raw_value + (1 - alpha) * last_smoothed_value; | |
| } |
we would increase code coverage for free then ;)
I have tried to solve the issue on #365. Tried to go with the way of
low_pass_filter:std::vector<double>andgeometry_msgs::msg::WrenchStamped.control_toolboxand making it usecontrol_filtersheader.