-
Notifications
You must be signed in to change notification settings - Fork 16
Add state to map matching results #464
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
base: main
Are you sure you want to change the base?
Conversation
…output and map matching input format
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 enhances map matching functionality by adding state information to results and restructuring the traversal summary output format. The changes enable users to quickly access trip-level or edge-level metrics like total energy consumed or travel time directly from map matching results.
Changes:
- Added
recalculate_pathmethod to SearchInstance for recomputing state transitions on paths generated by map matching algorithms - Restructured traversal summary serialization to include operation type, unit, and value as structured JSON objects
- Refactored route output generation into a new RouteOutput module, extracting logic from TraversalPlugin
- Extended MapMatchingRequest/Response to support multiple output formats and summary operations
- Updated Python utilities, examples, and tests to work with the new response structure
- Added battery_capacity as an output feature in energy models for BEV and PHEV vehicles
- Increased tolerance distance from 15.0 to 100.0 meters in default configuration files
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/routee-compass-core/src/algorithm/search/search_instance.rs | Adds recalculate_path method to recompute edge traversals with correct state accumulation |
| rust/routee-compass/src/app/search/route_output.rs | New module extracting route output generation logic with structured summary format |
| rust/routee-compass/src/app/search/mod.rs | Exports new RouteOutput and SummaryOp types |
| rust/routee-compass/src/plugin/output/default/traversal/plugin.rs | Refactored to use RouteOutput module and support query-level summary ops |
| rust/routee-compass/src/plugin/output/default/traversal/builder.rs | Updated imports for moved SummaryOp type |
| rust/routee-compass/src/plugin/output/default/traversal/traversal_output_format.rs | Added PartialEq and Eq derives for TraversalOutputFormat |
| rust/routee-compass/src/plugin/output/default/traversal/traversal_ops.rs | Removed redundant traversal field from GeoJSON feature properties |
| rust/routee-compass/src/app/map_matching/map_matching_request.rs | Added output_format and summary_ops fields to support configurable output |
| rust/routee-compass/src/app/map_matching/map_matching_response.rs | Changed matched_path to generic JSON and added traversal_summary field |
| rust/routee-compass/src/app/compass/compass_map_matching.rs | Updated to use recalculate_path and RouteOutput for consistent state handling |
| rust/routee-compass/src/app/compass/compass_app.rs | Added recalculate_path call in map matching flow |
| rust/routee-compass-core/src/model/state/state_variable_config.rs | Added get_unit_name method for serializing unit information |
| rust/routee-compass-core/src/model/cost/traversal_cost.rs | Added PartialEq derive for TraversalCost |
| rust/routee-compass-core/src/algorithm/search/edge_traversal.rs | Added PartialEq derive for EdgeTraversal |
| rust/routee-compass-core/src/model/map/matching_type.rs | Fixed bug in Combined matching type to properly handle NotFound results |
| rust/routee-compass-core/src/model/map/map_model.rs | Updated to use result pattern matching for destination processing |
| rust/routee-compass-core/src/algorithm/map_matching/model/lcss/trajectory_segment.rs | Minor refactoring for comparison consistency |
| rust/routee-compass-core/src/algorithm/map_matching/model/lcss/lcss_ops.rs | Minor refactoring in shortest path result handling |
| rust/routee-compass-powertrain/src/model/bev_energy_model.rs | Added battery_capacity as output feature |
| rust/routee-compass-powertrain/src/model/phev_energy_model.rs | Added battery_capacity as output feature |
| rust/routee-compass/src/app/map_matching/map_matching_tests.rs | Updated tests for new response structure and output formats |
| python/nrel/routee/compass/map_matching/utils.py | Updated to handle GeoJSON FeatureCollection format and added optional parameters |
| python/nrel/routee/compass/plot/plot_folium.py | Updated to work with GeoJSON FeatureCollection format |
| python/tests/* | Updated test expectations for new structured summary format |
| docs/examples/*.py | Updated examples to access nested value field in summary |
| python/nrel/routee/compass/resources/*.toml | Increased tolerance distance from 15.0 to 100.0 meters |
| AGENTS.md | Updated Python development setup instructions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
python/nrel/routee/compass/resources/osm_default_temperature.toml
Outdated
Show resolved
Hide resolved
|
@nreinicke noting your request for review but also un-addressed reviews from our robot overlords, did you want me to continue to wait for those changes? |
|
@robfitzgerald - sorry about that, I've gone through and addressed the copilot comments and this should now be ready for your review |
robfitzgerald
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.
makes sense to improve the output to indicate aggregation method and unit. it's a little noisier in the JSON but i think it's the best option as opposed to splitting out the value and the metadata (unit, summary_op) to some other place in the output.
i've got some questions for you and some requests to improve readability, but in general i'm on board and appreciate the improvement.
| /// this is useful for updating the state of a path that was | ||
| /// generated by a map matching algorithm or some other process | ||
| /// that does not maintain the full search state. | ||
| pub fn recalculate_path( |
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.
perhaps rename "calculate_path". i would expect a recalculate function would take a Vec and return a Vec.
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.
Good suggestion, I've updated to rename
| &self.cost_model, | ||
| )?; | ||
|
|
||
| // Use indexed labels to handle cycles in the path correctly |
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.
oh i get it, we need to make sure we track the vector index of each insertion into the tree as we artificially inject our path into the tree. could you make this clearer with comments in this function?
can you explain what this means about handling cycles correctly? i would understand if map matching always just used the simple Vertex label type since we're actually not doing any search. i'm guessing in the map matching setting, it's possible to end up with loops in the path that wouldn't appear in an optimal traversal. but then how is the index value being used here, just to explicitly track what segment index we are on?
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.
Yes, I can expand the comments here to try to make it clearer
|
|
||
| /// the cost of an edge traversal. | ||
| #[derive(Serialize, Deserialize, Default, Clone, Debug, Allocative)] | ||
| #[derive(Serialize, Deserialize, Default, Clone, Debug, Allocative, PartialEq)] |
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.
just checking here, a derived PartialEq will check for equality on all fields of TraversalCost. was this the intent, or, do we only need to compare objective_costs between TraversalCost instances?
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.
Good catch, that was actually a relic from some previous changes, I will remove that derivation since it's not needed.
| } | ||
| } | ||
|
|
||
| pub fn get_unit_name(&self) -> String { |
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.
this seems to duplicate the functionality above. could we 1) delete this, 2) give it doc comments and explain why we need it? if we do need it, could we give these two functions clearer names and also share implementation where possible?
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.
Yes, you're right, I will consolidate.
| } | ||
|
|
||
| impl SummaryOp { | ||
| pub fn default_summary_ops() -> HashMap<String, SummaryOp> { |
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.
this shouldn't be set here in code, right? it should be passed down from (state variable) config? like for instance, what happens in downstream libraries that have other fields not listed here?
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.
Yes, good point. I think it might make sense to move these defaults to the config so a downstream library with custom state features can add them into the config. Then, we have layers where the summary op can be specified:
- defaults in the config file
- can be overridden at query time
- if there is no op in default or in query, we fall back to "avg" for non-accumulators and "last" for accumulators.
| } | ||
| } | ||
|
|
||
| pub struct RouteOutput; |
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.
doesn't need to be a struct
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.
You're right, I'll adjust to just be a function
|
|
||
| impl RouteOutput { | ||
| /// Generates the JSON output for a route, including the path and a summary of the state. | ||
| pub fn generate( |
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.
this wasn't clear since it wasn't clear what a RouteOutput was. perhaps "generate_route_output" as a module-level pub function without the struct
| }); | ||
|
|
||
| let value = match op { | ||
| SummaryOp::Sum => route |
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.
this could be a function on SummaryOp, like SummaryOp::aggregate(route) -> StateVariable
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.
Yeah I like that, I'll adjust
| @@ -0,0 +1,163 @@ | |||
| use crate::plugin::output::default::traversal::TraversalOutputFormat; | |||
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.
let's break up this file into parts, perhaps into a module named route/
|
@robfitzgerald - Thanks for your detailed review. I've gone in and addressed your comments. I'm also working to integrate compass with routee-transit and to support that I ended up making a few more changes for review. Most notably, I added a new run_calculate_path method to the CompassApp. This is important in the routee transit application since we want to apply map matching once and then compute the energy and state over an already discovered path. Open to discussions about the best way to represent this in the application API |
yes, let's set up a call or meet tomorrow (i'll be in-person) to talk through a plan for these top-level changes to the compass API before we commit them. |
Extends upon the map matching results to also include the search state in the matched path and a final traversal summary in the map matched result. Now, when map matching, a user can quickly get trip or edge level metrics like total energy consumed or total travel time.
Note that this update changes the serialization of the traversal summary a bit. Now, we encode which summary op was used and what unit the variable has. For example:
{ "edge_speed": { "value": 42.02192799852017, "unit": "miles/hour", "op": "avg" }, "edge_energy_liquid": { "value": 0.40017462491453454, "unit": "gallons_gasoline_equivalent", "op": "sum" }, "trip_elevation_gain": { "value": 0.0328126010941803, "unit": "miles", "op": "last" } }This update also required a new method on the search instance
recalculate_paththat can take an existing path (like the one we get back from map matching as a vector of edge ids) and recompute the state transitions. This is needed since the LCSS algorithm compute paths recursively and then joins them back without state context. This ensures that accumulators liketrip_timeare correct in the final result.Lastly, I made some fixes the examples and python utilities to be compatible with these changes.