Extracting semantic tags, road and lane types & speed limits#484
Extracting semantic tags, road and lane types & speed limits#484aprabou wants to merge 8 commits into
Conversation
… junction in Road class. junction is now a raw string and later converted in assign_semantic_tags
| if speed_mps is not None: | ||
| current_speed = speed_mps |
There was a problem hiding this comment.
If speed_mps is None (which I assume could happen if the corresponding type element has no speed element or has one specifying "no limit"), wouldn't this code mean we inherit the speed limit from the previous range? That doesn't seem right to me: if there is no speed limit for part of the road, we should respect that (and keep None as the speed limit there).
There was a problem hiding this comment.
That does make sense. I made the following changes and would like to know if this aligns with dealing with None values:
_speed_limit_ranges_from_s_speed_records: removedcurrent_speedinheritance. Each record now has its ownspeed_mpswhich can beNone. Also added a prepend record starting at 0.0, so coverage always starts at s = 0, if the first record has s > 0.speed_limit_ranges_from_type_records: no more inheritance across types, allows forNonespeeds.assign_speed_limit_from_ranges: ifNonegaps exist, it is kept track of rather than discarded.speed_limit_for_s_interval: interval overlapping only undefined stretches returnsNone.
| ] | ||
|
|
||
|
|
||
| def speed_limit_for_s_interval(ranges, s_start, s_end): |
There was a problem hiding this comment.
I think the code would be simpler if you had this function just return the set of speed limits (you can call it speed_limits_for_s_interval) and move the min to the one place where you call this function.
|
|
||
| element.speedLimit = min(speeds) | ||
| if len(speeds) > 1: | ||
| element.speedLimitRanges = tuple(ranges) |
There was a problem hiding this comment.
I think we should set this attribute unconditionally, not only when there are multiple ranges. (That way consumer code can assume speedLimitRanges always has a meaningful value.)
| element.speedLimitRanges = () | ||
|
|
||
|
|
||
| def merge_scenic_tags(*tag_sets): |
There was a problem hiding this comment.
This is just a union -- I don't think it needs a special helper function and unit tests. You can just write frozenset(type_tags | self.extra_tags).
| if junction.name: | ||
| tags.add(junction.name) |
There was a problem hiding this comment.
This should go into the name attribute, not the tags.
| tags = _junction_tags(junction) | ||
| if not tags: | ||
| continue | ||
| for road_id, road in road_map.roads.items(): |
There was a problem hiding this comment.
This nested loop will take quadratic time. I'd compute the tags for each junction first (might as well just do it in the Junction initializer), then loop over roads and pull out the tags from the appropriate junction.
| for element in elements: | ||
| if isinstance(element, _ROAD_PROPAGATION_TYPES): |
There was a problem hiding this comment.
I think iterating over a bunch of elements and only taking the ones of the correct type is roundabout and confusing. Where this function is used, you know exactly which sub-elements to use (e.g. road.laneGroups and road.sections), so I would propagate to those directly.
| element.speedLimit = speed_limit | ||
|
|
||
|
|
||
| def apply_lane_speed_limits(elements): |
There was a problem hiding this comment.
It should be the min over the sections. Also I don't think we need a helper function for this: just do it directly when creating the Lane.
| element.speedLimit = max(section_limits) | ||
|
|
||
|
|
||
| def propagate_tags(tags, elements): |
There was a problem hiding this comment.
As above, let's do this directly with the correct elements and not use a helper function.
| self.id_ = id_ | ||
| self.width = [] # List of tuples (Poly3, int) for width and s-offset. | ||
| self.speed_records = [] # [(sOffset, speed_mps), ...] from <speed> elements. | ||
| self.speed_limit = None # First record's speed; kept for backward compatibility. |
There was a problem hiding this comment.
There are no backwards-compatibility concerns since this is a new attribute. I think you can drop it. (Unlike NetworkElement.speedLimit, which we could deprecate but shouldn't remove immediately.)
| if prev_id not in offsets: | ||
| continue |
There was a problem hiding this comment.
What's the reason for this change? This condition should never be true.
| type_tags = frozenset( | ||
| open_drive_type | ||
| for _, open_drive_type, _ in self.type_records | ||
| if open_drive_type |
There was a problem hiding this comment.
How can open_drive_type be falsy?
| f"road {self.id_} lane {id_} section s=[{s_start},{s_end})" | ||
| ), | ||
| ) | ||
| else: |
There was a problem hiding this comment.
It's not obvious to me how we get into this case. Do we really need two cases, or is there a way to make it uniform?
| "offRamp", | ||
| "connectingRamp", | ||
| "slipLane", | ||
| "biking", |
There was a problem hiding this comment.
This is a bike lane, not a driving lane.
There was a problem hiding this comment.
In that case, similar to how the code deals with sidewalk and shoulder lane types, should I model biking lanes as a separate LaneGroup.bikeLane?
| "restricted", | ||
| "curb", | ||
| "parking", | ||
| "none", |
There was a problem hiding this comment.
What's the motivation for including none? It isn't necessarily a shoulder.
There was a problem hiding this comment.
The spec mentions that while it does not have actual content, it serves for applications to register that OpenDRIVE is still present in case the driver leaves the road. I'm not sure if this would be relevant to Scenic but I wanted to add that in case it was.
| road_map = RoadMap() | ||
| road_map.parse(path) | ||
| road_map.calculate_geometry(num=5, calc_intersect=True) | ||
| road = road_map.toScenicNetwork().roads[0] |
There was a problem hiding this comment.
Can you use parse_scenic_network (or a variant) to simplify this kind of test?
| internal_road = road_map.roads[7] | ||
| internal_road.extra_tags = frozenset({"onramp"}) | ||
| road, _ = internal_road.toScenicRoad(road_map.tolerance) |
There was a problem hiding this comment.
Rather than modifying internal attributes like this, I think it's clearer to generate the whole Network object and then assert things about it.
| road_map = RoadMap() | ||
| road_map.parse(path) | ||
| road_map.calculate_geometry(num=5, calc_intersect=True) | ||
| return road_map.toScenicNetwork() |
There was a problem hiding this comment.
Until now no tests in this folder used Network: tests that do are in tests/domains/driving instead. Since you're checking OpenDRIVE-specific things you could keep them in this folder, but maybe make a new file to distinguish them (e.g. test_speeds.py or test_tags.py).
dfremont
left a comment
dfremont
left a comment
There was a problem hiding this comment.
This is definitely headed in the right direction, thanks Ashwin! I think we can simplify the code a lot, though: see the comments above. Here are a few other suggestions:
- Let's add a
Network.speedLimitAtmethod to get the speed limit (if any) at a given point. I think it can just callNetwork.laneSectionAtto get the lane section, computesin the same way that the first line ofPolylineRegion.nearestSegmentTodoes (only necessary if there's more than 1 speed limit range), and then look up the right speed limit range. - I haven't finished reading all the tests yet (I'll wait until you refactor things), but they're pretty verbose: please see if you can simplify the code. One note is that I don't think we need unit tests for things like
propagate_speed_limit: that's an internal function which could easily get refactored away. I'd rather have integration tests which load a simple map and then assert things about the resultingNetworkobject. (Of course, for functions that will be used in many places and are easily testable on their own, unit tests make sense.)
2 participants
Description
This extension allows for the extraction of semantic tags and speed limits/types from roads and lanes.
The changes are made to the following files:
roads.py:
xodr_parser.py:
Speed parsing/among helpers:
When an interval straddles multiple limits, the minimum is used and an OpenDriveWarning is emitted.
Tag helpers:
Propagation helpers:
Road, Lane, and Junction now capture:
Road.toScenicRoad wires per-section and per-lane speed limits and tags into the Scenic objects.
Expanded drivable_lane_types / sidewalk_lane_types / shoulder_lane_types defaults (e.g. slipLane, biking, walking, restricted, curb, none; parking classified as a shoulder).
test_opendrive.py:
Checklist
pytestand/or other means