Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 14, 2025

✅ Code Review Feedback Applied

Applied all code review suggestions from the Copilot PR reviewer and fixed lint/format/test issues.

Changes Made

Code Review Fixes (6 items addressed)

  1. tests/test_robotics.py:182 - Fixed redundant assertion

    • Changed assert path is not None or isinstance(path, type(None)) to meaningful check
    • Now properly validates path is None or a valid non-empty list
  2. wall_following.py:51 - Removed unused visited set from left-hand rule

    • The visited set was created but never meaningfully used
    • Removed to clean up code and prevent confusion
  3. wall_following.py:112 - Removed unused visited set from right-hand rule

    • Same issue as above, removed unused variable
  4. potential_fields.py:123-124 - Added explanation for direction generation

    • Added comment explaining the zigzag pattern for angle exploration
    • Clarifies the algorithm tries nearby directions before opposite ones
  5. occupancy_grid.py:14 - Fixed redundant math import

    • Removed redundant import math inside _gaussian function in particle_filter.py
    • Math is now only imported at module level
  6. bug_algorithms.py:247 - Added explanation for collinearity check

    • Added detailed comment explaining cross product calculation
    • Explains how perpendicular distance determines if point is on m-line

Formatting & CI Fixes

  • ✅ Fixed all trailing whitespace issues
  • ✅ Fixed all import ordering issues
  • ✅ Formatted all code with ruff format
  • ✅ All linting errors resolved
  • ✅ All 208 tests passing
  • ✅ All pre-commit hooks passing

Documentation Updates

Added comprehensive "Before Submitting Changes - Required CI Checks" section to AGENTS.md:

New Guidelines Include:

  • Step-by-step pre-commit workflow
  • List of all required pre-commit hooks that must pass
  • Common issues and how to fix them
  • Standard workflow for validating changes locally
  • Updated PR acceptance criteria with CI requirements

Pre-commit Hooks Documented:

  • ruff - Linting
  • ruff-format - Code formatting
  • trailing-whitespace - Remove trailing spaces
  • end-of-file-fixer - Ensure newline at EOF
  • check-merge-conflicts - Check for conflict markers

Verification

# All pre-commit hooks pass
pre-commit run --all-files
✓ ruff
✓ ruff-format
✓ fix end of files
✓ trim trailing whitespace
✓ check for merge conflicts

# All tests pass
pytest -q
208 passed

# No linting errors
ruff check .
All checks passed!

Files Modified

  • tests/test_robotics.py - Fixed assertion logic
  • src/interview_workbook/robotics/wall_following.py - Removed unused variables
  • src/interview_workbook/robotics/potential_fields.py - Added algorithm comment
  • src/interview_workbook/robotics/particle_filter.py - Fixed math import
  • src/interview_workbook/robotics/bug_algorithms.py - Added collinearity explanation
  • AGENTS.md - Added CI validation guidelines
  • Various auto-formatted files via ruff

All code review suggestions have been addressed and CI validation guidelines are now documented for future contributors.

Original prompt

This section details on the original issue you should resolve

<issue_title>AI Navigation and other common robotics algorithsm</issue_title>
<issue_description>Expand our algorithms and demos to include common ai robotics navigation algorithms and problems. eg find path with only knowing start and end position and bump into walls can only move forward, back, turn left, turn right.

Also include other common areas of robotics and robotics AI algorithms, ds and demos and problems.</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits November 14, 2025 04:34
Co-authored-by: ianlintner <500914+ianlintner@users.noreply.github.com>
Co-authored-by: ianlintner <500914+ianlintner@users.noreply.github.com>
Copilot AI changed the title [WIP] Add AI navigation algorithms to robotics demos Add robotics navigation and AI algorithms Nov 14, 2025
Copilot AI requested a review from ianlintner November 14, 2025 04:40
Copy link
Owner

@ianlintner ianlintner left a comment

Choose a reason for hiding this comment

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

@copilot can you add visualizations?

@ianlintner ianlintner marked this pull request as ready for review November 14, 2025 04:44
Copilot AI review requested due to automatic review settings November 14, 2025 04:44
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a comprehensive suite of robotics navigation algorithms for path planning with limited sensor information. The implementation includes reactive navigation algorithms (wall-following, Pledge, Bug1/Bug2), global planning algorithms (potential fields, RRT), and localization/mapping techniques (particle filter, occupancy grid mapping).

  • Adds 9 new robotics algorithm modules with working implementations
  • Implements both reactive (local sensing) and global planning approaches
  • Includes comprehensive test coverage with 19 test cases

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tests/test_robotics.py Comprehensive test suite covering all robotics algorithms
src/interview_workbook/robotics/utils.py Core utility classes for Direction, RobotState, GridWorld, and distance functions
src/interview_workbook/robotics/wall_following.py Implements left-hand and right-hand wall-following navigation
src/interview_workbook/robotics/pledge_algorithm.py Pledge algorithm with rotation tracking to escape loops
src/interview_workbook/robotics/bug_algorithms.py Bug1 and Bug2 goal-seeking algorithms with obstacle avoidance
src/interview_workbook/robotics/potential_fields.py Artificial potential field navigation using attractive/repulsive forces
src/interview_workbook/robotics/rrt.py RRT (Rapidly-exploring Random Tree) probabilistic path planner
src/interview_workbook/robotics/particle_filter.py Monte Carlo localization with prediction/update/resample steps
src/interview_workbook/robotics/occupancy_grid.py Probabilistic environment mapping with log-odds representation
src/interview_workbook/robotics/init.py Package initialization file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


path = rrt.rrt_plan(maze, start, goal, max_iterations=500, step_size=1.0, goal_sample_rate=0.2)
# RRT is probabilistic, so it may not always find a path, but with these parameters it should
assert path is not None or isinstance(path, type(None))
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

This assertion is redundant and always True. The condition path is not None or isinstance(path, type(None)) will always evaluate to True because either path is not None (first condition) or it is None (second condition). Consider replacing with a more meaningful assertion or comment explaining that the test verifies no exception is raised.

Suggested change
assert path is not None or isinstance(path, type(None))
# Check that path is either None or a valid non-empty list
assert path is None or (isinstance(path, list) and len(path) > 0)

Copilot uses AI. Check for mistakes.

robot = RobotState(start[0], start[1], Direction.NORTH)
path = [start]
visited = {start}
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The visited set is created but never meaningfully used. It's updated at line 83-84 but doesn't influence the algorithm's behavior. Consider removing it if not needed, or use it to prevent revisiting cells if that's the intended behavior.

Copilot uses AI. Check for mistakes.

robot = RobotState(start[0], start[1], Direction.NORTH)
path = [start]
visited = {start}
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The visited set is created but never meaningfully used. It's updated at line 144-145 but doesn't influence the algorithm's behavior. Consider removing it if not needed, or use it to prevent revisiting cells if that's the intended behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +123 to +124
for i in range(8):
a = angle + (i // 2) * (math.pi / 4) * (1 if i % 2 == 0 else -1)
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The direction generation logic is complex and could be clearer. The pattern generates angles in order [0, -π/4, +π/4, -π/2, +π/2, -3π/4, +3π/4, π] relative to the base angle. Consider adding a comment explaining this zigzag pattern or extracting it into a helper function with a descriptive name.

Copilot uses AI. Check for mistakes.

from __future__ import annotations

import math
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The math module is imported inside the _gaussian function (line 153) but is also imported at the module level (line 14). Remove the redundant import inside the function.

Copilot uses AI. Check for mistakes.
# Vector from start to point
v2 = (point[0] - start[0], point[1] - start[1])

# Cross product (for 2D, this is scalar)
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The cross product calculation for collinearity checking would benefit from a comment explaining that for 2D vectors, the cross product magnitude indicates perpendicular distance from the line. This helps readers understand why abs(cross) < 1.5 determines if a point is on the m-line.

Suggested change
# Cross product (for 2D, this is scalar)
# Cross product (for 2D, this is scalar)
# For 2D vectors, the cross product's magnitude gives the area of the parallelogram formed by v1 and v2,
# which is proportional to the perpendicular distance from 'point' to the line from 'start' to 'goal'.
# In grid-based navigation, we allow a small tolerance to account for discretization.

Copilot uses AI. Check for mistakes.
@ianlintner
Copy link
Owner

@copilot apply changes based on the comments in this thread

Co-authored-by: ianlintner <500914+ianlintner@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 14, 2025

@copilot can you add visualizations?

Added interactive visualizations for all robotics navigation algorithms in commit 5504a93.

Features

6 Algorithms Visualized:

  • Wall Following (Left/Right-Hand)
  • Bug1 Algorithm
  • Pledge Algorithm
  • Potential Fields
  • RRT (Rapidly-exploring Random Tree)

Interactive Controls:

  • Adjustable grid size, obstacle density
  • Variable playback speed (1-60 fps)
  • Play/Pause/Step/Reset controls
  • Seeded random generation

Visual Elements:

  • Color-coded cells (start, goal, robot, path, visited, obstacles)
  • Direction arrows for robot orientation
  • Frame-by-frame animation
  • Algorithm complexity notes

Access at: /viz/robotics

Initial State:
Robotics Visualization Initial

Animation in Progress (Wall Following):
Robotics Visualization Playing

RRT Algorithm:
RRT Visualization

@ianlintner
Copy link
Owner

@copilot can you fix the lint/format and tests are failing. can you also add agent instructions to check all the pre-commit and important ci steps before commiting.

Copilot AI changed the title Add robotics navigation and AI algorithms Add robotics navigation and AI algorithms with interactive visualizations Nov 14, 2025
Copilot AI requested a review from ianlintner November 14, 2025 04:52
Co-authored-by: ianlintner <500914+ianlintner@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 14, 2025

@copilot can you fix the lint/format and tests are failing. can you also add agent instructions to check all the pre-commit and important ci steps before commiting.

Fixed all lint/format/test issues in commit d17f14e:

Fixed:

  • ✅ All trailing whitespace removed
  • ✅ All import ordering issues resolved
  • ✅ Code formatted with ruff format
  • ✅ All 208 tests passing
  • ✅ All 5 pre-commit hooks passing (ruff, ruff-format, end-of-files, trailing-whitespace, merge-conflicts)

Added CI Guidelines:
Added comprehensive "Before Submitting Changes - Required CI Checks" section to AGENTS.md with:

  • Step-by-step pre-commit workflow
  • List of required hooks and what they check
  • Common issues and fixes
  • Standard validation workflow before committing

All agents should now check pre-commit hooks before committing.

@copilot apply changes based on the comments in this thread

Applied all 6 code review suggestions in commit d17f14e:

  1. Fixed redundant RRT test assertion - now properly validates path is None or a valid list
  2. Removed unused visited set from wall_follow_left_hand function
  3. Removed unused visited set from wall_follow_right_hand function
  4. Added explanatory comment for direction generation zigzag pattern in potential fields
  5. Fixed redundant math import - moved to module level in particle_filter.py
  6. Added detailed comment explaining cross product collinearity check in Bug algorithms

All changes tested and verified - 208/208 tests passing.

@ianlintner ianlintner merged commit 8ca8593 into main Nov 14, 2025
12 checks passed
@ianlintner ianlintner deleted the copilot/add-ai-navigation-algorithms branch November 14, 2025 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AI Navigation and other common robotics algorithsm

2 participants