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.

Sign up for GitHub

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

Jump to bottom

Allow parameter changes while keeping defaults #146

Merged
merged 7 commits into from
Dec 23, 2023
Merged

Allow parameter changes while keeping defaults #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d9520a9
Made tree_kwargs changeable while keeping defaults
amalnanavati Dec 22, 2023
dd13335
Don't restrict parameter updates to tree_kwargs
amalnanavati Dec 23, 2023
41b09e5
Change action_server_params to dict
amalnanavati Dec 23, 2023
bd1f4e1
Remove WatchdogLsitener params from default
amalnanavati Dec 23, 2023
6b625b4
Make sequences stored as lists by convention
amalnanavati Dec 23, 2023
d9f4af0
Addressed PR comment
amalnanavati Dec 23, 2023
58f1fd8
Addressed PR comments
amalnanavati Dec 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ good-names=a,
ax,
ex,
hz,
kw,
ns,
Run,
_
Expand Down
7 changes: 5 additions & 2 deletions ada_feeding/ada_feeding/trees/move_to_mouth_tree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@
# that they have similar code.

# Standard imports
from collections.abc import Sequence
import os
from typing import Tuple
from typing import Annotated, Tuple

# Third-party imports
from ament_index_python.packages import get_package_share_directory
Expand Down Expand Up @@ -76,6 +77,8 @@ class MoveToMouthTree(MoveToTree):
# pylint: disable=too-many-instance-attributes, too-many-arguments
# Bite transfer is a big part of the robot's behavior, so it makes
# sense that it has lots of attributes/arguments.
# pylint: disable=dangerous-default-value
# plan_distance_from_mouth must be a list because ROS params only supports lists.

def __init__(
self,
Expand All @@ -90,7 +93,7 @@ def __init__(
allowed_face_distance: Tuple[float, float] = (0.4, 1.25),
face_detection_msg_timeout: float = 5.0,
face_detection_timeout: float = 2.5,
plan_distance_from_mouth: Tuple[float, float, float] = (0.025, 0.0, -0.01),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doe we expect this to ever not be 3D? When?

If not, we should keep it a Tuple to enforce the size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We always expect it to be length 3, but the issue is that ROS2 params cannot be of type tuple, only type list. And Python's typing interface doesn't currently allow us to specify a length for Lists.

However, I found this discussion in the mypy library, which is a static type checker for Python. So I'll change it to Annotated[Sequence[float], 3] so that future Python type checkers can verify it.

plan_distance_from_mouth: Annotated[Sequence[float], 3] = [0.025, 0.0, -0.01],
fork_target_orientation_from_mouth: Tuple[float, float, float, float] = (
0.5,
-0.5,
Expand Down
298 changes: 0 additions & 298 deletions ada_feeding/config/ada_feeding_action_servers.yaml
View file
Open in desktop

This file was deleted.

6 changes: 6 additions & 0 deletions ada_feeding/config/ada_feeding_action_servers_current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# This file is auto-generated by create_action_servers.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that this is in a commit and is updated by code, it should be added to .gitignore.

Copy link
Contributor Author

@amalnanavati amalnanavati Dec 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I initially put it in the .gitignore, but then realized that .gitignore doesn't do anything for files that are already in the repository. It seems like (based on this thread) the only thing we can do is running git update-index --assume-unchanged ada_feeding/config/ada_feeding_action_servers_current.yaml in our local copy, but we can't do any global configurations to ignore changes to this file.

I'd say we just have to be vigilant in PRs to verify changes to this file are not pushed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think update-index --assume-unchanged can mess up some git checks; like it thought I had local changes in the current.yaml file even though I didn't, so prevented me from changing branches. I'd say we should not use that command, and instead just be vigilant on the PR side to verify changes to this file don't get passed in.

ada_feeding_action_servers:
ros__parameters:
current:
overridden_parameters:
- ""
Loading