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

Remove need for sensor_model_list, motion_model_list, and publisher_list #5

Conversation

sloretz
Copy link

@sloretz sloretz commented Jan 24, 2023

This adds a helper function to fuse_core (which should really be upstreamed to rclcpp) that allows discovering parameters at a given prefix without declaring them. It the uses that new function to avoid needing new parameters for the list of sensor models, motion models, and publishers.

@methylDragon

…claring them

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
const std::map<std::string, rclcpp::ParameterValue> & overrides,
std::string prefix, size_t depth)
{
// TODO(sloretz) ROS 2 must have this in a header somewhere, right?
Copy link
Owner

Choose a reason for hiding this comment

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

std::unordered_set<std::string>
list_parameter_overrides_at_prefix(
node_interfaces::NodeInterfaces<node_interfaces::Parameters> interfaces,
std::string prefix, size_t depth = 0);
Copy link
Owner

Choose a reason for hiding this comment

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

Let's do this instead (I think it's a slightly more intuitive naming), so it doesn't get confused with "get at n-depth", but instead does "get up-to n-depth"

Suggested change
std::string prefix, size_t depth = 0);
std::string prefix, size_t max_depth = -1);

Changes should be made elsewhere as appropriate, I think

Alternatively, if the intention is to follow list_parameters's signature, disregard this comment

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I didn't even know about depth in list_parameters signature.

In order to avoid confusing the two APIs I renamed list_parameter_overrides_at_prefix to list_parameter_override_prefixes and removed the depth argument in 1343096. It now always behaves like it had been given a depth of 1 in the old implementation, which is all fuse needs.


TEST(parameter, list_parameter_overrides)
{
const std::map<std::string, rclcpp::ParameterValue> overrides = {
Copy link
Owner

Choose a reason for hiding this comment

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

Could we also have something at depth 3 and test the depth=2 case?

Copy link
Author

Choose a reason for hiding this comment

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

In 1343096 I added a few more tests with longer parameter names.

Copy link
Owner

@methylDragon methylDragon left a comment

Choose a reason for hiding this comment

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

Small comments (above!) Otherwise, I'm happy because tests still pass 😬

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Copy link
Owner

@methylDragon methylDragon left a comment

Choose a reason for hiding this comment

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

LGTM! There's a tiny typo to fix but otherwise I think we're good to go

} // namespace detail

/**
* @brief Get pararameter overrides that have a given prefix
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: tiny tiny typo

Copy link
Author

Choose a reason for hiding this comment

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

Fixed typo in bc6467c

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Author

sloretz commented Jan 24, 2023

Merging, then will continue review in locusrobotics#307

@sloretz
Copy link
Author

sloretz commented Jan 24, 2023

Ah, nevermind. I can't merge. @methylDragon mind merging it?

@methylDragon methylDragon merged commit 3cba177 into methylDragon:rolling-optimizers Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants