Edge speeds bugs #95
Edge speeds bugs #95
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes bugs preventing edge speeds from being computed for many edges. The changes enable proper handling of vehicle dimension restrictions (e.g., height limits) in access control logic and corrects a boolean condition that was filtering out valid speed limits.
Changes:
- Fixed a boolean value in speed limit filtering to include speed limits without heading constraints
- Added vehicle dimension filtering logic to handle restrictions like maximum vehicle height
- Implemented unit conversion methods for length and weight units to support cross-unit comparisons
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/bambam-omf/src/graph/segment_split.rs | Changed None => false to None => true to include speed limits without heading constraints in speed calculations |
| rust/bambam-omf/src/graph/segment_ops.rs | Added vehicle dimension compatibility checking in when_is_compatible function and added comprehensive test coverage |
| rust/bambam-omf/src/collection/record/transportation_segment.rs | Added unit conversion methods for length/weight units, visibility modifiers for vehicle restriction fields, and validation logic for vehicle restrictions |
| rust/bambam-omf/src/collection/record/mod.rs | Added exports for newly public types used in vehicle restrictions |
| rust/bambam-omf/src/collection/filter/mod.rs | Exported MatchBehavior for use in tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rust/bambam-omf/src/collection/record/transportation_segment.rs
Outdated
Show resolved
Hide resolved
rust/bambam-omf/src/collection/record/transportation_segment.rs
Outdated
Show resolved
Hide resolved
robfitzgerald
left a comment
robfitzgerald
left a comment
There was a problem hiding this comment.
looks good! i may be misunderstanding but i think the unit conversions may be a mix-up. did i get that right? and there's a nit pick about match nesting which doesn't need to get fixed. thank you for digging into this!
rust/bambam-omf/src/collection/record/transportation_segment.rs
Outdated
Show resolved
Hide resolved
|
@yamilbknsu just checking, is this ready for review again? |
|
@robfitzgerald yes! Sorry I didn’t ping. It’s ready for review again |
robfitzgerald
left a comment
robfitzgerald
left a comment
There was a problem hiding this comment.
looks good. thanks for sorting out these details. the logic appears sound, let's see how it scales.
Couldn't find "Stone" so we use the transformation to Kg
i know! i had never heard of this before either! 🪨
|
Something is not working in the CI pipeline. I'll try again tomorrow |
Seeing this happening on the compass repo as well. Perhaps a GitHub problem. |
|
It's an issue with GitHub. Status page here |
|
@yamilbknsu re-ran the test after GitHub status cleared, everything looks good! |
2 participants
In this PR I patch a bug preventing many edges to get the speeds computed. One of the changes is flipping a boolean value, the other is adding logic to capture "vehicle" logic in access restrictions. The example I was using was specifying that vehicles should not be taller than 13 feet.
I had to do some funny business to get the units to work in a reasonable manner. Let me know if you think we should refactor that part to some other pattern.