-
Notifications
You must be signed in to change notification settings - Fork 29
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 more coverage to World
class tests
#138
Conversation
@@ -1055,7 +1061,7 @@ def is_connectable(self, start, goal, max_dist=None): | |||
|
|||
# Build up the array of test X and Y coordinates for sampling between | |||
# the start and goal points. | |||
dist_array = np.arange(0, dist, 0.01) | |||
dist_array = np.arange(0, dist, step_dist) |
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.
Removed this hard-coded pose in this PR as well, and made it a parameter of the path planners.
# Select a graph node using the same resolution strategy. | ||
graph_node = apply_resolution_strategy( | ||
graph_nodes, resolution_strategy, robot=robot | ||
) |
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.
Ended up implementing this TODO in the PR
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.
Only have a couple of suggestions here
pyrobosim/pyrobosim/core/world.py
Outdated
# If the new robot has a bigger inflation radius than previously, | ||
# use this new one. Otherwise, we can leave it as is. | ||
old_inflation_radius = self.inflation_radius | ||
new_inflation_radius = max([r.radius for r in self.robots] + [robot.radius]) |
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.
There are few more places with single variable.
Should we enforce the "no single letter variable" ?
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.
Probably a good idea, updated all the ones I found in world.py
.
Is this enforceable in e.g. the pre-commit-config.yaml
file or something? If so, might be worth automating in another PR.
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.
Should be possible for normal variables, not sure about the ones in list comprehension.
This PR addresses the missing test coverage highlighted in #94 -- namely tests around some methods of the
World
class that were not covered in the existing set of tests.As always, this fixes some additional issues found along the way as well.
Changes to
world.py
are actually very minor (except for one bug incollides_with_robots
) -- I just moved some functions around to be in the right "sections" next to related functions.Closes #94.