Add pedestrian obstacle forces to simulation environment #84
Conversation
… class documentation and the ability to simulate with peds obstacle forces. Functionality has to be verified.
…b.com/ll7/robot_sf_ll7 into 39-Pedestrians-have-no-obstacle-force
… forces in simulation
…ialization and documentation
…n for MapDefinition
…h default map handling and debug option
…and debug logging
WalkthroughThe pull request introduces several changes primarily focused on enhancing the Python testing configuration in Visual Studio Code, adding new performance metrics to the benchmark results, and modifying the environment setup for robotic simulations. Key updates include enabling pytest, adding a new environment class to handle pedestrian obstacle forces, and improving error handling and logging in the SVG map parser. Additionally, new test files have been created to validate the functionality of these enhancements. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Copilot reviewed 6 out of 10 changed files in this pull request and generated 1 suggestion.
Files not reviewed (4)
- .vscode/settings.json: Language not supported
- benchmark_results.json: Language not supported
- robot_sf/gym_env/robot_env.py: Evaluated as low risk
- robot_sf/gym_env/robot_env_with_pedestrian_obstacle_forces.py: Evaluated as low risk
Comments skipped due to low confidence (4)
tests/test_robot_with_ped_obstacle_force.py:33
- The test function 'test_can_simulate_with_pedestrians' does not have any assertions to verify the expected behavior of the environment. Add assertions to verify the expected behavior.
_, _, terminated, truncated, _ = env.step(rand_action)
robot_sf/sim/simulator.py:93
- [nitpick] The warning message should be more specific. Suggested: "This may lead to pedestrians not interacting with obstacles as expected."
"This may lead to unexpected behavior."
robot_sf/sim/simulator.py:230
- [nitpick] The error message should use consistent naming conventions. Suggested: "MapDefinition should be of type MapDefinition, got {type(map_def)}"
"map_def should be of type MapDefinition, got {type(map_def)}"
robot_sf/sim/simulator.py:399
- The word 'rasie' should be corrected to 'raise'.
"# rasie type error and print the type of map_def"
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
robot_sf/gym_env/robot_env_with_pedestrian_obstacle_forces.py (1)
14-18: Document the performance impact more precisely.The comment about 40% performance impact is important but needs more context:
- What was the baseline for this measurement?
- Under what conditions was this measured?
- Are there any mitigation strategies available?
test_pygame/test_ped_obstacle_forces.py (1)
20-22: Add error handling for environment steps.The environment step calls should handle potential exceptions to ensure proper cleanup.
Consider wrapping the step execution in a try-except block:
- rand_action = env.action_space.sample() - _, _, done, _, _ = env.step(rand_action) - env.render() + try: + rand_action = env.action_space.sample() + _, _, done, _, _ = env.step(rand_action) + env.render() + except Exception as e: + logger.error(f"Error during environment step: {str(e)}") + raisetests/test_robot_with_ped_obstacle_force.py (1)
5-12: Clean up commented imports.Remove commented-out imports if they're not needed, or document why they're kept for future use.
benchmark_results.json (1)
369-371: Performance degradation detected in latest benchmark.The addition of pedestrian obstacle forces has resulted in approximately 30% performance degradation:
- steps_per_second decreased from ~60-64 to 44.63
- avg_step_time_ms increased from ~15-16 to 22.40
Consider profiling the code to identify optimization opportunities.
robot_sf/gym_env/env_util.py (2)
284-287: Address the TODO comment about next goal selection.The TODO comment indicates an unresolved design decision about next goal selection for ego pedestrians.
Would you like me to help implement a next goal selection strategy or create a GitHub issue to track this task?
233-234: Consider optimizing pedestrian position concatenation.The comment suggests potential performance issues with
np.vstack. Consider pre-allocating the array or using alternative approaches for better performance.- lambda: np.vstack((sim.ped_pos, np.array([sim.ego_ped_pos]))), + lambda: np.concatenate((sim.ped_pos, sim.ego_ped_pos[np.newaxis, :]), axis=0),robot_sf/gym_env/robot_env.py (1)
59-60: Consider implementing the TODO commentThe
video_fpsparameter should be read from environment settings rather than using a hardcoded value to maintain consistency and configurability.Would you like me to help implement this enhancement by reading the FPS from environment settings?
robot_sf/nav/svg_map_parser.py (1)
280-289: Improve exception handlingWhile the type verification is good, the exception handling could be improved by using
raise ... from errto maintain the exception chain.- raise TypeError( - f"Map definition is not of type MapDefinition: {type(self.map_definition)}" - ) + raise TypeError( + f"Map definition is not of type MapDefinition: {type(self.map_definition)}" + ) from err🧰 Tools
🪛 Ruff (0.8.0)
287-289: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
robot_sf/sim/simulator.py (1)
226-230: Improve exception handlingSimilar to the issue in svg_map_parser.py, the exception handling should maintain the exception chain.
- raise TypeError(f"map_def should be of type MapDefinition, got {type(map_def)}") + raise TypeError(f"map_def should be of type MapDefinition, got {type(map_def)}") from err🧰 Tools
🪛 Ruff (0.8.0)
230-230: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
maps/svg_maps/example_map_with_obstacles.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
.vscode/settings.json(1 hunks)benchmark_results.json(1 hunks)robot_sf/gym_env/env_util.py(7 hunks)robot_sf/gym_env/robot_env.py(2 hunks)robot_sf/gym_env/robot_env_with_pedestrian_obstacle_forces.py(1 hunks)robot_sf/nav/svg_map_parser.py(4 hunks)robot_sf/sim/simulator.py(7 hunks)test_pygame/test_ped_obstacle_forces.py(1 hunks)tests/test_robot_with_ped_obstacle_force.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
robot_sf/gym_env/env_util.py
55-55: Function definition does not bind loop variable i
(B023)
56-56: Function definition does not bind loop variable i
(B023)
robot_sf/nav/svg_map_parser.py
287-289: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
robot_sf/sim/simulator.py
230-230: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (7)
.vscode/settings.json (1)
30-35: LGTM! Verify test discovery configuration.
The pytest configuration correctly includes both test directories and disables unittest in favor of pytest.
Let's verify the test discovery configuration:
✅ Verification successful
Test directories and files are correctly configured in VS Code settings
The verification confirms that both test directories (tests/ and test_pygame/) exist and contain Python test files as configured in the VS Code settings:
test_pygame/contains test files for visualization and pedestrian forcestests/contains test files for various functionalities including robot-pedestrian interaction
While we couldn't verify pytest discovery due to Python not being available in the sandbox, the directory structure and test files align perfectly with the VS Code configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that both test directories exist and contain relevant test files
# Check if test directories exist and contain Python test files
echo "Checking test directories structure..."
fd -t d "^test" .
fd -t f "^test.*\.py$" tests/ test_pygame/
# Verify pytest can discover the tests
python -m pytest --collect-only tests/ test_pygame/
Length of output: 572
robot_sf/gym_env/robot_env_with_pedestrian_obstacle_forces.py (1)
35-37: LGTM! Verify integration with parent class.
The initialization looks correct, explicitly enabling pedestrian obstacle forces.
Let's verify the parent class integration:
✅ Verification successful
Let me try a different approach to verify the parent class integration.
Let me check the complete implementation of the parent class to verify the parameter handling.
Integration with parent class is correctly implemented
The parent class RobotEnv explicitly accepts and handles the peds_have_obstacle_forces parameter in its __init__ method (line 48) and passes it to the simulator initialization through init_simulators (line 89). The child class RobotEnvWithPedestrianObstacleForces correctly sets this parameter to True in its super call, ensuring proper integration with the parent class's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify RobotEnv parent class accepts peds_have_obstacle_forces parameter
# Check RobotEnv class implementation
echo "Checking RobotEnv implementation..."
ast-grep --pattern 'class RobotEnv:
$$$
def __init__($$$, peds_have_obstacle_forces=$$$):
$$$'
Length of output: 185
Script:
#!/bin/bash
# Let's first find the RobotEnv class implementation
echo "Searching for RobotEnv class files..."
fd "robot_env" --type f
# Then examine the content of potential parent class files
echo -e "\nChecking class implementation..."
rg "class RobotEnv" -A 10
Length of output: 2147
Script:
#!/bin/bash
# Get the complete RobotEnv class implementation
echo "Checking complete RobotEnv implementation..."
cat robot_sf/gym_env/robot_env.py
Length of output: 10998
robot_sf/gym_env/robot_env.py (1)
97-100: LGTM: Clean parameter addition
The peds_have_obstacle_forces parameter is properly integrated into the init_simulators call, with good formatting for readability.
robot_sf/nav/svg_map_parser.py (1)
75-77: LGTM: Improved None check
Using is None is more explicit and safer than the previous truthiness check, as it specifically handles the case of an uninitialized SVG root.
robot_sf/sim/simulator.py (3)
44-58: LGTM: Well-documented parameter
The docstring for peds_have_obstacle_forces is clear and includes valuable performance impact information (40% increase in simulation duration).
90-97: LGTM: Good defensive programming
The code properly handles the case of unset peds_have_obstacle_forces with a clear warning message and sensible default value.
105-109: LGTM: Clean implementation of force filtering
The code efficiently filters out obstacle forces when disabled, with clear logging to indicate the state.
| def test_pedestrian_obstacle_avoidance(): | ||
| logger.info("Testing Pedestrian and Obstacle forces") | ||
| map_def = convert_map("maps/svg_maps/example_map_with_obstacles.svg") | ||
| logger.debug(f"type map_def: {type(map_def)}") | ||
| env = RobotEnvWithPedestrianObstacleForces(map_def=map_def, debug=True) | ||
| logger.info("created envirnoment") | ||
| env.reset() | ||
| for _ in range(1000): | ||
| rand_action = env.action_space.sample() | ||
| _, _, done, _, _ = env.step(rand_action) | ||
| env.render() | ||
| if done: | ||
| env.reset() | ||
| env.close() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test robustness and coverage.
While the visual test is valuable for development, consider these improvements:
- Add non-visual automated tests with specific assertions
- Add test cases for edge cases (e.g., maximum number of pedestrians)
- Consider reducing iterations for CI/CD pipeline efficiency
Here's a suggested improvement:
def test_pedestrian_obstacle_avoidance():
logger.info("Testing Pedestrian and Obstacle forces")
map_def = convert_map("maps/svg_maps/example_map_with_obstacles.svg")
- logger.debug(f"type map_def: {type(map_def)}")
env = RobotEnvWithPedestrianObstacleForces(map_def=map_def, debug=True)
- logger.info("created envirnoment")
env.reset()
- for _ in range(1000):
+ try:
+ # Test initialization
+ assert env.peds_have_obstacle_forces, "Obstacle forces should be enabled"
+
+ # Test basic functionality (reduce iterations for CI)
+ n_steps = 100 if os.getenv("CI") else 1000
+ for step in range(n_steps):
rand_action = env.action_space.sample()
- _, _, done, _, _ = env.step(rand_action)
+ obs, reward, done, truncated, info = env.step(rand_action)
+
+ # Add basic assertions
+ assert obs is not None, "Observation should not be None"
+ assert isinstance(reward, (int, float)), "Reward should be numeric"
+
env.render()
if done:
env.reset()
- env.close()
+ finally:
+ env.close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_pedestrian_obstacle_avoidance(): | |
| logger.info("Testing Pedestrian and Obstacle forces") | |
| map_def = convert_map("maps/svg_maps/example_map_with_obstacles.svg") | |
| logger.debug(f"type map_def: {type(map_def)}") | |
| env = RobotEnvWithPedestrianObstacleForces(map_def=map_def, debug=True) | |
| logger.info("created envirnoment") | |
| env.reset() | |
| for _ in range(1000): | |
| rand_action = env.action_space.sample() | |
| _, _, done, _, _ = env.step(rand_action) | |
| env.render() | |
| if done: | |
| env.reset() | |
| env.close() | |
| def test_pedestrian_obstacle_avoidance(): | |
| logger.info("Testing Pedestrian and Obstacle forces") | |
| map_def = convert_map("maps/svg_maps/example_map_with_obstacles.svg") | |
| env = RobotEnvWithPedestrianObstacleForces(map_def=map_def, debug=True) | |
| env.reset() | |
| try: | |
| # Test initialization | |
| assert env.peds_have_obstacle_forces, "Obstacle forces should be enabled" | |
| # Test basic functionality (reduce iterations for CI) | |
| n_steps = 100 if os.getenv("CI") else 1000 | |
| for step in range(n_steps): | |
| rand_action = env.action_space.sample() | |
| obs, reward, done, truncated, info = env.step(rand_action) | |
| # Add basic assertions | |
| assert obs is not None, "Observation should not be None" | |
| assert isinstance(reward, (int, float)), "Reward should be numeric" | |
| env.render() | |
| if done: | |
| env.reset() | |
| finally: | |
| env.close() |
| occupancies = [ | ||
| ContinuousOccupancy( | ||
| sim.map_def.width, | ||
| sim.map_def.height, | ||
| lambda: sim.robot_pos[i], | ||
| lambda: sim.goal_pos[i], | ||
| lambda: sim.pysf_sim.env.obstacles_raw[:, :4], | ||
| lambda: sim.ped_pos, | ||
| robot_config.radius, | ||
| sim_config.ped_radius, | ||
| sim_config.goal_radius, | ||
| ) | ||
| for i in range(num_robots) | ||
| ] |
There was a problem hiding this comment.
Fix loop variable binding in lambda functions.
The current implementation may lead to unexpected behavior as the lambda functions capture the loop variable 'i' by reference. All lambda functions might end up using the last value of 'i'.
Apply this fix:
occupancies = [
ContinuousOccupancy(
sim.map_def.width,
sim.map_def.height,
- lambda: sim.robot_pos[i],
- lambda: sim.goal_pos[i],
+ lambda i=i: sim.robot_pos[i],
+ lambda i=i: sim.goal_pos[i],
lambda: sim.pysf_sim.env.obstacles_raw[:, :4],
lambda: sim.ped_pos,
robot_config.radius,
sim_config.ped_radius,
sim_config.goal_radius,
)
for i in range(num_robots)
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| occupancies = [ | |
| ContinuousOccupancy( | |
| sim.map_def.width, | |
| sim.map_def.height, | |
| lambda: sim.robot_pos[i], | |
| lambda: sim.goal_pos[i], | |
| lambda: sim.pysf_sim.env.obstacles_raw[:, :4], | |
| lambda: sim.ped_pos, | |
| robot_config.radius, | |
| sim_config.ped_radius, | |
| sim_config.goal_radius, | |
| ) | |
| for i in range(num_robots) | |
| ] | |
| occupancies = [ | |
| ContinuousOccupancy( | |
| sim.map_def.width, | |
| sim.map_def.height, | |
| lambda i=i: sim.robot_pos[i], | |
| lambda i=i: sim.goal_pos[i], | |
| lambda: sim.pysf_sim.env.obstacles_raw[:, :4], | |
| lambda: sim.ped_pos, | |
| robot_config.radius, | |
| sim_config.ped_radius, | |
| sim_config.goal_radius, | |
| ) | |
| for i in range(num_robots) | |
| ] |
🧰 Tools
🪛 Ruff (0.8.0)
55-55: Function definition does not bind loop variable i
(B023)
56-56: Function definition does not bind loop variable i
(B023)
There was a problem hiding this comment.
Can you explain this suggested change in greater detail?
There was a problem hiding this comment.
@ll7, certainly! The issue arises due to how Python handles closures in loops when using lambda functions.
In your code:
occupancies = [
ContinuousOccupancy(
sim.map_def.width,
sim.map_def.height,
lambda: sim.robot_pos[i],
lambda: sim.goal_pos[i],
# ...
)
for i in range(num_robots)
]The lambda functions lambda: sim.robot_pos[i] and lambda: sim.goal_pos[i] capture the variable i, not its current value. Because i changes during the loop and ends up at its final value after the loop completes, all the lambda functions will reference the same final value of i. This means that when you call any of these lambda functions, they will all access sim.robot_pos[num_robots - 1] instead of the intended sim.robot_pos[i] for each specific i.
To fix this, we bind the current value of i to each lambda function by adding it as a default parameter:
occupancies = [
ContinuousOccupancy(
sim.map_def.width,
sim.map_def.height,
lambda i=i: sim.robot_pos[i],
lambda i=i: sim.goal_pos[i],
# ...
)
for i in range(num_robots)
]By setting i=i in the lambda's parameters, we create a new default argument i that holds the value of i at the time the lambda is created. This way, each lambda function retains its own specific i, and when called, it will correctly access sim.robot_pos[i] corresponding to each robot.
This is a common technique in Python to ensure that closures capture the loop variable's value at each iteration, preventing unexpected behavior.
I hope this clarifies the suggested change.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…force' into 39-Pedestrians-have-no-obstacle-force
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.gitignore (1)
21-22: LGTM! Consider grouping system-specific files.Both additions are appropriate:
wandb: Correctly ignores Weights & Biases artifacts which can be large and are environment-specific*.DS_Store: Standard practice to ignore macOS system filesConsider grouping system-specific files together by moving
*.DS_Storenear other OS-specific ignore patterns if any exist in the file.robot_sf/gym_env/robot_env_with_pedestrian_obstacle_forces.py (3)
1-3: Enhance module documentationThe current docstring is quite brief. Consider expanding it to include:
- The purpose and benefits of pedestrian obstacle forces
- The performance impact (~40% simulation time increase)
- Usage examples or configuration options
""" -Create a robot environment with pedestrian obstacle forces +A specialized robot environment that incorporates obstacle forces for pedestrians. + +This environment enhances simulation realism by making pedestrians react to obstacles, +though it increases simulation time by approximately 40%. It extends the base RobotEnv +class with additional force calculations. + +Example: + env = RobotEnvWithPedestrianObstacleForces() + # Or with a custom map + env = RobotEnvWithPedestrianObstacleForces(map_def=custom_map) """
14-18: Enhance class documentationThe class docstring could provide more implementation details and usage information.
""" Robot environment with pedestrian obstacle forces This increases the simulation time by roughly 40% + +This class extends RobotEnv to simulate realistic pedestrian behavior by incorporating +obstacle forces. Pedestrians will actively avoid obstacles in their path, leading to +more natural movement patterns. + +Attributes: + map_def: Optional map definition for the environment + debug: Boolean flag to enable debug mode + +Note: + The increased simulation time is due to additional force calculations + performed for each pedestrian-obstacle interaction. """
27-28: Consider making the default map path configurableThe default map path is currently hardcoded. Consider making it configurable through environment variables or a configuration file for better flexibility.
+DEFAULT_MAP_PATH = os.getenv( + "ROBOT_ENV_DEFAULT_MAP", + "maps/svg_maps/example_map_with_obstacles.svg" +) if map_def is None: logger.warning("No map_def provided. Using default map") - default_map_path = "maps/svg_maps/example_map_with_obstacles.svg" + default_map_path = DEFAULT_MAP_PATH
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
.gitignore(1 hunks)robot_sf/gym_env/robot_env_with_pedestrian_obstacle_forces.py(1 hunks)test_pygame/test_ped_obstacle_forces.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test_pygame/test_ped_obstacle_forces.py
🔇 Additional comments (1)
robot_sf/gym_env/robot_env_with_pedestrian_obstacle_forces.py (1)
20-45: Implementation looks good and aligns with requirements!
The constructor is well-implemented with proper error handling, logging, and configuration. It successfully addresses the issue #39 by enabling pedestrian obstacle forces through the peds_have_obstacle_forces=True parameter.
Key points:
- Proper error handling for map loading
- Clear logging messages
- Correct initialization of map pool and environment settings
- Appropriate use of parent class constructor
Introduce a new environment class for simulating pedestrian obstacle forces, enhance testing with pytest, and improve code readability and documentation. Default values and initialization for obstacle forces are now included.
Fixes #39
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores
.gitignoreto exclude Weights and Biases files and macOS-specific files.