Add window size #210
Open
Add window size #210
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Tatsuro Sakaguchi <[email protected]>
Tacha-S
requested review from
emersonknapp and
MichaelOrlov
and removed request for
a team
christophebedard
requested changes
Feb 20, 2025
There was a problem hiding this comment.
Unfortunately, we have to consider multiple things:
- By default, this should probably behave like it did before, i.e., it should consider all values. The tests are currently failing because this is not the case.
- Tests should be added to cover the case where a window is used (probably in
test/moving_average_statistics/test_moving_average_statistics.cpp). - Finally: How would an end user use this? In this case, this code is ultimately used in
rclcpp(seerclcpp::topic_statistics::SubscriptionTopicStatistics). From looking at the subscription creation code inrclcpp, there is no way to provide awindow_sizevalue.- One solution might be to add a
window_sizeoption toTopicStatisticsOptions: https://github.com/ros2/rclcpp/blob/9c493c4615aee3b5cbc68e9101b588564869b5dc/rclcpp/include/rclcpp/subscription_options.hpp#L69-L85 - Then this would be used here: https://github.com/ros2/rclcpp/blob/9c493c4615aee3b5cbc68e9101b588564869b5dc/rclcpp/include/rclcpp/create_subscription.hpp#L56
- We have to figure out which statistics this should affect.
- One solution might be to add a
@emersonknapp do you have any thoughts on this?
|
I understood points 1 and 2. I noticed this feature in the controller manager, so I thought the user would create an instance, but it turns out that it can be enabled or disabled through the options when subscribing in rclcpp. |
Signed-off-by: Tatsuro Sakaguchi <[email protected]>
Signed-off-by: Tatsuro Sakaguchi <[email protected]>
Signed-off-by: Tatsuro Sakaguchi <[email protected]>
Tacha-S
force-pushed
the
feature/window-size
branch
from
ed4d967 to
87c9821
Compare
| @@ -52,6 +53,9 @@ class MovingAverageStatistics | |||
| LIBSTATISTICS_COLLECTOR_PUBLIC | |||
| MovingAverageStatistics() = default; | |||
|
|
|||
| LIBSTATISTICS_COLLECTOR_PUBLIC | |||
| explicit MovingAverageStatistics(const std::size_t & window_size); | |||
There was a problem hiding this comment.
Suggested change
| explicit MovingAverageStatistics(const std::size_t & window_size); | |
| explicit MovingAverageStatistics(std::size_t window_size); |
There was a problem hiding this comment.
Watch out for the extra space:
Suggested change
| explicit MovingAverageStatistics(const std::size_t & window_size); | |
| explicit MovingAverageStatistics(std::size_t window_size); |
And we could keep the const.
There was a problem hiding this comment.
For primitives types, we should always pass by value, not reference.
Signed-off-by: Tatsuro Sakaguchi <[email protected]>
Signed-off-by: Tatsuro Sakaguchi <[email protected]>
Tacha-S
force-pushed
the
feature/window-size
branch
from
87c9821 to
74d468c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have added a window size to calculate the moving average