-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add frequency to topic CLI. #503
Add frequency to topic CLI. #503
Conversation
2fc3947
to
b4305ca
Compare
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! In general, I would recommend using gz::math::SignalStats
for all of this. I don't see examples or tutorials but here's a test that shows how to use it.
Is there an example of this functions performance vs a minimal operations oriented implementation like I provided? Mostly saying this because we use gz-transport on some very very very low compute systems to enable HIL, so if I can cut any extra ops anywhere, I prefer that. Especially something that can be tightly time bounded such as message transports. |
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! In general, I would recommend using
gz::math::SignalStats
for all of this. I don't see examples or tutorials but here's a test that shows how to use it.Is there an example of this functions performance vs a minimal operations oriented implementation like I provided? Mostly saying this because we use gz-transport on some very very very low compute systems to enable HIL, so if I can cut any extra ops anywhere, I prefer that. Especially something that can be tightly time bounded such as message transports.
The SignalStats
would have similar performance as this implementation because it's doing essentially the same computations. The difference is that it doesn't store the interval data (see https://github.com/gazebosim/gz-math/blob/f0d4e59e66b54949937ceed725e50ba26177d667/src/SignalStats.cc#L81-L101). If having the intervals is important, you'd have to store them like you're doing here in addition to using SignalStats
. If you prefer not to use SignalStats
, I've made suggestions to avoid using raw for loops and indices where possible.
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.
Looks good. Just a few nits
There are changes in the base branch, gz-transport13 that haven't been merged. That's why the ABI checker is failing. Can you merge and push? |
Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]> Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]> Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
Signed-off-by: Benjamin Perseghetti <[email protected]>
187233c
to
2dbd0e1
Compare
rebased and pushed. |
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!
🦟 Bug fix
Fixes #
Summary
Adds frequency sampling to a topic through gz topic CLI.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.