Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues work on OpenStreetMap Feature (OMF) edge processing by introducing parallelization, f32 precision geometry support, and a flexible segment splitting architecture.
- Converted all geometry types from f64 to f32 precision to match Compass assumptions
- Added parallelized processing for connector vertex creation, split finding, and edge creation using rayon
- Refactored segment splitting logic into extensible types with support for checking and creating missing vertices
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/bambam/src/model/output_plugin/opportunity/source/overture_opportunity_collection_model.rs | Updated return types to use Geometry instead of Geometry |
| rust/bambam/src/model/output_plugin/opportunity/source/lodes/lodes_ops.rs | Added coordinate mapping to f32 precision in chunk_by grouping operation |
| rust/bambam/src/model/output_plugin/opportunity/opportunity_source.rs | Updated return type to Geometry |
| rust/bambam/src/model/output_plugin/opportunity/opportunity_model_config.rs | Changed Point type from f64 to f32 |
| rust/bambam-omf/src/graph/serialize_ops.rs | Major refactor with parallelization, new vertex/edge creation functions, and missing connector detection (contains incomplete TODO) |
| rust/bambam-omf/src/graph/segment_split.rs | Refactored to use ConnectorInSegment type and simplified API for edge creation |
| rust/bambam-omf/src/graph/segment_ops.rs | New module with segment processing functions for creating splits |
| rust/bambam-omf/src/graph/omf_graph.rs | Refactored from try_from_collection to new() with parallelized pipeline architecture |
| rust/bambam-omf/src/graph/mod.rs | Added new module exports for connector_in_segment and segment_ops |
| rust/bambam-omf/src/graph/connector_in_segment.rs | New type representing a connector within a segment with linear reference |
| rust/bambam-omf/src/collection/record/transportation_segment.rs | Changed geometry to f32, added get_linestring() and get_coord_at() helper methods |
| rust/bambam-omf/src/collection/record/transportation_connector.rs | Updated geometry type to f32, removed unnecessary cast in vertex creation |
| rust/bambam-omf/src/collection/record/place.rs | Updated geometry return type to f32 |
| rust/bambam-omf/src/collection/record/common.rs | Added f32 coordinate mapping in geometry deserialization |
| rust/bambam-omf/src/collection/record/building.rs | Updated geometry return type to f32 |
| rust/bambam-omf/src/collection/error.rs | Added InvalidLinearReference and InternalError variants |
| rust/bambam-omf/src/app/omf_app.rs | Added optional output_directory parameter, changed run() signature to take &self |
| rust/bambam-omf/Cargo.toml | Added ordered-float dependency |
| rust/Cargo.toml | Added ordered-float workspace dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Modify in-place a vectorized graph according to a split logic | ||
| pub fn split( | ||
| /// identifies any locations where additional coordinates are needed. | ||
|
|
There was a problem hiding this comment.
The documentation comment on line 21 should be combined with the comment on line 23 (both are part of the same doc comment for the function). There's an inappropriate line break between them.
| let _ = vertex_lookup.insert(vertex_uuid, vertex_id); | ||
| } | ||
| eprintln!(); // end progress bar | ||
| todo!() |
There was a problem hiding this comment.
This TODO indicates incomplete implementation. The function should return Ok(()) after successfully adding missing connectors, but instead it unconditionally calls todo!(). This will cause a panic at runtime when missing connectors are found and added.
| todo!() | |
| Ok(()) |
| let chunk_iter = res.join_dataset.into_iter().chunk_by(|r| { | ||
| r.geometry.map_coords(|c| geo::Coord { | ||
| x: c.x as f32, | ||
| y: c.y as f32, | ||
| }) | ||
| }); |
There was a problem hiding this comment.
The chunk_by closure creates new geometries for each comparison during the grouping operation. Since chunk_by may call the closure multiple times per element for comparison purposes, this could lead to inefficient repeated coordinate mapping. Consider storing the converted f32 geometry once per record before the chunk_by operation.
| }) | ||
| .transpose() | ||
| .map_err(|e| D::Error::custom(format!("Could not decode wkb: {e}"))) | ||
| .map_err(|e: GeozeroError| D::Error::custom(format!("Could not decode wkb: {e}"))) |
There was a problem hiding this comment.
The error type annotation is unnecessary here. Rust can infer the error type from the context (the closure parameter and return type). The explicit type annotation could be removed for cleaner code.
| .map_err(|e: GeozeroError| D::Error::custom(format!("Could not decode wkb: {e}"))) | |
| .map_err(|e| D::Error::custom(format!("Could not decode wkb: {e}"))) |
| /// | ||
| /// this follows the pattern described by OvertureMaps when assigning unique | ||
| /// identifiers to sub-segments by their segment id along with linear reference ranges. | ||
| /// see [[https://docs.overturemaps.org/guides/transportation/#transportation-splitter]] |
There was a problem hiding this comment.
The link format in the documentation is incorrect. It uses double square brackets which is not valid Markdown or rustdoc syntax. The correct format should be single square brackets with parentheses for the URL, or use angle brackets for a bare URL.
| /// see [[https://docs.overturemaps.org/guides/transportation/#transportation-splitter]] | |
| /// see <https://docs.overturemaps.org/guides/transportation/#transportation-splitter> |
this PR continues the work from #66 but adding the following features:
some code style changes to support parallelism and extensibility, for the most part about decoupling mutable operations on large collections and creating an additional type for a "Connection along a Segment".