-
Notifications
You must be signed in to change notification settings - Fork 28
Ndr/misc updates #217
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
Ndr/misc updates #217
Conversation
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.
Pull request overview
This PR adds support for handling disconnected graph components and refactors the plotting utilities into a more modular structure. The changes introduce a filter_to_largest_component parameter (defaulting to True) that allows users to optionally keep all graph components instead of only the largest strongly connected component, which could prevent routing failures in some cases. Additionally, the PR fixes an edge case where paths with only a single road segment were not handled correctly.
Key changes:
- Adds graceful handling of disconnected graph components by catching NetworkXNoPath exceptions and returning empty lists
- Refactors monolithic
mappymatch/utils/plot.pyinto separate module files for better code organization - Fixes edge case logic for paths with single road segments (changed from
n < 2ton == 0andlen > 1tolen > 0)
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_disconnected_components.py | Comprehensive test suite for disconnected component handling with tests for graph parsing, shortest path, and LCSS merge operations |
| mappymatch/utils/plot/trajectory_segment.py | New module for plotting trajectory segments with trace, path, matches, and cutting points |
| mappymatch/utils/plot/trace.py | New module for plotting trace objects |
| mappymatch/utils/plot/path.py | New module for plotting road paths |
| mappymatch/utils/plot/matches.py | New module for plotting match results and match distance histograms |
| mappymatch/utils/plot/map.py | New module for plotting NxMap objects with enhanced features like hover highlighting |
| mappymatch/utils/plot/geofence.py | New module for plotting geofence objects |
| mappymatch/utils/plot/init.py | Module initialization with public API exports for all plotting functions |
| mappymatch/utils/plot.py | Removed - functionality moved to modular plot/ directory |
| mappymatch/matchers/lcss/ops.py | Extracted join_segment function from lcss.py and updated logic to handle single-segment paths |
| mappymatch/matchers/lcss/lcss.py | Refactored to use extracted join_segment function from ops module |
| mappymatch/matchers/lcss/constructs.py | Fixed edge case to properly handle empty paths (n == 0 instead of n < 2) |
| mappymatch/maps/nx/readers/osm_readers.py | Added filter_to_largest_component parameter to allow keeping all graph components |
| mappymatch/maps/nx/nx_map.py | Added exception handling for NetworkXNoPath to return empty list for disconnected components and added filter_to_largest_component parameter to from_geofence |
| pixi.lock | Version bump to 0.7.0 and removed Python upper version constraint |
| .pre-commit-config.yaml | Simplified to use pixi run check command |
| CLAUDE.md | Added project context documentation for AI assistants |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@jhoshiko - this should be ready for your review |
jhoshiko
left a comment
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.
@nreinicke This all looks great to me! Tested and working on my end!
Adds in a fix to the edge case where we have a path with only one road segment.
Updates the plotting module to break out the plots into separate files and adds a new file to plot a trajectory segment.
Adds an optional flag
filter_to_largest_componentto the methodNxMap.from_geofence(), set to True by default. When this flag is false, we will not extract the largest strongly connected component which could lead to routing failures but will keep the entire graph rather than opaquely removing some of it that is not strongly connected.