Repository navigation
fix(mobile): resolve the workspace in one place, with a documented precedence - #776
Merged
Merged
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 2 high |
🟢 Metrics 24 complexity · 0 duplication
Metric Results Complexity 24 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #776 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 143 144 +1
Lines 14671 14689 +18
=====================================
- Misses 14671 14689 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
petercorke
force-pushed
the
refactor/mobile-workspace
branch
from
October 15, 2026 01:07
57ab852 to
825f136
Compare
…ecedence A workspace was resolved four different ways in seven classes, with two directions of authority (EKF overwrote the vehicle's private _workspace with the map's, ParticleFilter used another order) and the vehicle's workspace never reached its driver. - roboticstoolbox.mobile.workspace.resolve_workspace: scalar, 2-vector, 4-vector or an object with a workspace attribute, first usable candidate wins - precedence: explicit argument, map, vehicle (documented in mobile.rst) - VehicleBase, drivers, LandmarkMap, BaseMap/PolygonMap, EKF and ParticleFilter use it; objects with a workspace are now accepted by the maps - VehicleBase.workspace has a setter; EKF uses it instead of writing _workspace - RandomPath takes the workspace of its vehicle when it has none of its own, so the error for no workspace moves from construction to init Also fixes, found on the way: - Bicycle(workspace=<ndarray>) raised on the truth value of an array - EKF(workspace=...) raised NameError (dim) - a BinaryOccupancyGrid made from a non-square workspace had width and height swapped - PolygonMap(polygons=[]) shared its default list between maps Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
petercorke
force-pushed
the
refactor/mobile-workspace
branch
from
October 15, 2026 01:17
825f136 to
9b70b32
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A workspace (
[xmin, xmax, ymin, ymax]) was accepted by seven classes and resolved four different ways, with two directions of authority and no documentation. This makes it one function and one rule.Before
VehicleBase,LandmarkMap:try: workspace.workspace except AttributeError. Drivers:hasattr. Maps/grids:expand_dimsonly, so an object with aworkspacewas not accepted.EKFandParticleFilter: their own variants.EKFoverwrote the vehicle's private_workspacewith its map's;ParticleFilterused a different order.RandomPathalways needed its own.After
roboticstoolbox.mobile.workspace.resolve_workspace(*sources): scalar, 2-vector, 4-vector, or an object with aworkspace; the first usable candidate wins.mobile.rst: explicit argument, then a map with a concrete extent, then the vehicle, then none. Something with a concrete extent beats something declared.VehicleBase.workspacehas a setter;EKFuses it rather than writing a private attribute.RandomPathuses its vehicle's workspace when it has none. The error for "no workspace at all" therefore moves from construction toinit(). This is the one deliberate behaviour change; the old test is updated.RandomPath(workspace=map),Bicycle(workspace=map)) is unchanged.Bugs fixed along the way
Bicycle(workspace=np.array([-5, 5]))raised "truth value of an array is ambiguous".EKF(workspace=...)raisedNameError(dim).BinaryOccupancyGridmade from a non-square workspace had width and height swapped:[0, 10, 2, 8]gave bounds[0, 6, 2, 12].PolygonMap(polygons=[])shared its default list between maps.Testing
17 new tests (
tests/test_mobile_workspace.py) for the resolver, the precedence, each class and the four bugs. Full suite: 19 failed on bothmainand this branch with identical names (the known C++ extension tests), 943 passed against 926. mypy is clean.Conflicts to expect: #772 (drivers) and #774 (
VehicleBase) touch neighbouring lines ofdrivers.pyandVehicle.py; I will rebase whichever merges second.Relates to petercorke/rvc-ecosystem#15 (naming convention).
🤖 Generated with Claude Code