Fix lane count calculation - use Scheme structs to avoid re-reading/re-interpreting tags #111

BudgieInWA · 2022-03-25T09:34:23Z

I am submitting this draft PR to get input on the new approach to driving_lane_directions, rust style and tests.yml. This PR is aiming towards ~~#69~~ #72.

Are we happy with this overall structure for `driving_lane_directions`?

bf5df9d shows the concise version, and dc5de27 shows how useful tag validation warnings can be added (thinking about #101 and how an editor would like to use tags_to_lanes).

Any feedback on my rust style?

I want to fit into this project, more than anything else, but couldn't help trying out is_some_and and using lots of unwrap_or.

How do the roundtrip tests work?

I couldn't figure it out just yet. Some guidance in the docs about writing and running tests.yml tests would be super awesome. (I figured out cargo test --feature tests, which I'll add to the docs if I get there first.)

Forward and backward lanes are independently guessed to be half of the lanes, but the tags on the website assume that providing `lanes` and `lanes:backward` is enough.

…o this structure

data/tests.yml

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs

droogmic

Looks good, thanks for the work so far.

You are working on issue #72

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs

droogmic · 2022-03-25T16:29:18Z

but couldn't help trying out is_some_and and using lots of unwrap_or

Looks pretty idiomatic to me, but then again I am not expert.

droogmic · 2022-03-25T16:31:14Z

I figured out cargo test --feature tests

This was a bug, it should be fixed now.

You should be able to run all the tests with just cargo test

The roundtrip works by taking the output (only the output), and checking that you can get back to the same spec by going to tags and back again (round trip).

Why not tags -> spec -> tags?
Well: spec to tags is a one to many mapping, so this shouldn't guarantee a roundtrip.

Documentation is very welcome!

dabreegster

Thanks @droogmic for reviewing, feel free to take the lead on this. I may have some time over the weekend to catch up, but other stuff first...

rust/osm2lanes/src/lib.rs

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs

Co-authored-by: droogmic <droogmic@gmail.com>

Incorporate lanes:both_ways and centre_turn_lanes. Add assumption that oneway=no, lanes=1 means lanes:both_ways=1

droogmic · 2022-03-27T07:36:19Z

Agreed, the content and location of the warnings appears correct.

…

On Sun, 27 Mar 2022, 07:52 Ben Ritter, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In rust/osm2lanes/src/transform/tags_to_lanes/mod.rs <#111 (comment)>: > + .get("lanes:bus") + .and_then(|num| num.parse::<usize>().ok()); + + if oneway.into() { + // TODO support oneway=-1 + if tagged_backward.is_some() { + warn!("lanes:backwards tag on a oneway road") + } + if tagged_both_ways.is_some() { + warn!("lanes:both_ways tag on a oneway road") + } + match (tagged_lanes, tagged_forward) { + (Some(l), None) => (l, 0), + (Some(l), Some(f)) => { + if l != f { + warn!("lanes tag does not agree with lanes:forward on oneway road") This does seem like the right place to generate most of these warnings, so I will upgrade all of the warn!s to instead feed into the RoadWarnings vec. — Reply to this email directly, view it on GitHub <#111 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACEL2Z6UD6DUGQ2WZS7ZFEDVB7ZRTANCNFSM5RTT6CFA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Reveals a bunch of failed tests...

…sway and lanes:{bus,psv}

droogmic

Looking good so far, surprisingly complex :)

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs

droogmic · 2022-03-28T16:57:33Z

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs

+    // Always assume no center turn lane unless tagged, so we already know:
+    let num_both_ways = tagged_both_ways.unwrap_or(tagged_center_turn_lanes.unwrap_or(0));


Is centre_turn_lane=no and lanes:both_ways=1 or centre_turn_lane=yes and lanes:both_ways=0 valid.

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs

droogmic · 2022-03-28T17:16:11Z

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs

+            }
+            (Some(l), Some(f), None) => (f, num_both_ways, l - f - num_both_ways),
+            (Some(l), None, Some(b)) => (l - b - num_both_ways, num_both_ways, b),
+            (Some(1), None, None) => (0, 1, 0),


1 lane becomes a centre lane? seems fishy?

It is a bit fishy, but I think it is because describing "how many lanes in each direction does a skinny allyway have" is fishy. I might tag them "lane_markings=no width=5", but we have to represent that in schema somehow.

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs

droogmic · 2022-03-28T17:18:55Z

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs

+fn driving_lane_directions(
+    tags: &Tags,
+    locale: &Locale,
+    oneway: Oneway,
+    warnings: &mut RoadWarnings,
+) -> (usize, usize, usize) {


can we test this in isolation, it is quite complex now :)

I agree! (if this function ends up keeping all of its responsibilities.)

Does that mean a set of tests in tests.yaml, or rust tests?

Rust tests I'd imagine, since the input and output are quite specialized here. The unit tests can be in this file, since it's private

BudgieInWA · 2022-03-28T17:40:03Z

I am finding more and more out about Tags library and the parsing techniques and good RoadMsg construction, still a little way to go. But, there is a fundamental problem:

The problem with locking in a final guess on the lanes distribution at the start

We cannot assume or guess missing parts of the lanes:{forward,backward}= scheme without parsing everything else. See how 28300bc adds tons of code parsing other schemes. Definitely smelly.

We need a way to narrow down our guess as we go. Random thoughts:

Maybe we use Infer like values too keep track of guesses.
Maybe in guessing mode, start with the assumption that there is one normal lane in each direction, then add lanes instead of mutating existing lanes. Then at the end we can do a round of assuming (e.g. distribute remaining lanes).
Maybe each tagging scheme has its own struct type using Infered values, which remains internally consistent, to help in the code which validates each tag schema, checks for conflicts, and wants to jump between them in order to do these complicated guesses. Then the validated and consistent schemes are used to build the road. Maybe having code that defines the test and guesses for each requirement between schemes is exactly the right amount of code (e.g. lanes:bus:forward=2 requires at least 2 forward lanes, or increases the guesses forward lanes by 2), but it still feels a bit like double handling (the whole point of the Builder pattern is to keep track of the jumping between different inputs, right?)
It ties into validating: psv:lanes=designated| requires lanes=2 to parse properly, so it implies lanes=2 (and is an error if lanes!=2)
It feels a bit like Logic Programming where different tags are facts that work to collapse the possibility space.

droogmic · 2022-03-31T06:52:27Z

I agree with everything you say, but I also don't really know the best way forward.

but it still feels a bit like double handling (the whole point of the Builder pattern is to keep track of the jumping between different inputs, right?)

correct

Maybe in guessing mode, start with the assumption that there is one normal lane in each direction, then add lanes instead of mutating existing lanes.

I agree, that we should maybe try this instead. The downside is that all the other logic needs to account for this new system, so it will be a painful refactor :(.

A simplified version of the logic you have now should probably still exist, to populate a top level Infer field in roadbuilder that may get invalidated by clearer tags later (like your bus example)?

In any event, it sounds like you understand the problem. Now you just need to decide how hard you make the solution for yourself ;)

droogmic · 2022-03-31T08:28:17Z

I added you to the project, so hopefully I dont need to approve the CI anymore

Probably doesn't compile, but close

…tructure. The code is a mess, gesturing generally at the idea, without compiling. I feel like the whole lot should mean close to what it actually says.

data/tests.yml

droogmic · 2022-04-20T15:47:53Z

data/tests.yml

@@ -1054,6 +1055,8 @@

 - way_id: 389654080
  mapillary: https://www.mapillary.com/app/?pKey=331760328316020
+  rust:
+    expect_warnings: true # deprecated centre_turn_lane


good idea to comment like that, need to do that myself

rust/osm2lanes/src/locale.rs

droogmic

I can agree to this once you look at what I have done and can justify why this is better.

For example:
How do we extend this to include https://taginfo.openstreetmap.org/keys/taxi:lanes?
it feels a lot of the lane handling is hardcoded to bus and only bus?

droogmic · 2022-04-20T16:07:24Z

data/tests.yml

    sidewalk: "no"
    shoulder: "no"
+    lanes: "3"
+    lanes:forward: "2"


can this not be inferred from bus:lanes:forward
what happens when this is removed?

It could, but lanes:forward should be tagged in OSM anyway. Inferring it is being a bit fancy and I'm not trying to solve the entirety of lane tagging in this PR.

but lanes:forward should be tagged in OSM anyway

it should but it often isn't, we shouldn't depend on perfect OSM tagging.
we can leave it in to simplify the implementation for now, but please add a comment describing why this data is redundant so that we can try to remove it later?

droogmic · 2022-04-20T16:07:55Z

rust/osm2lanes/src/locale.rs

+impl ToString for DrivingSide {
+    fn to_string(&self) -> String {
+        match self {
+            Self::Right => String::from("right"),


not sure if important,

but I avoided implementing this to avoid confusing right hand drive with right hand travel.

I am confused about the difference, so maybe it is important! Is right hand drive when the driver gets in the right hand door of the car? I suppose this should be Into<TagKey> instead of ToString or something?

droogmic · 2022-04-20T16:09:41Z

rust/osm2lanes/src/tag/mod.rs

@@ -11,6 +11,8 @@ pub use key::TagKey;
 mod osm;
 pub use osm::{Highway, HighwayType, Lifecycle, HIGHWAY, LIFECYCLE};

+use crate::transform::{RoadWarnings, TagsToLanesMsg};


it feels strange including these in this mod.

I would expect tag to eventually become its own crate or similar, with no dependencies on the osm2lanes transformation logic.

Agreed, get_parsed is a convenience function for parsing a bunch of tags at the same time, and should probably live in transform somewhere. It was suggested that it be moved in here, but maybe it should be a function, not a method: let l: Option<usize> = get_parsed_tag(tags, "lanes", &warnings); or a method on RoadWarnings: let l: Option<usize> = warnings.parse_tag(tags, "lanes")?

droogmic · 2022-04-20T16:11:21Z

rust/osm2lanes/src/tag/mod.rs

+        &self,
+        key: &K,
+        warnings: &mut RoadWarnings,
+    ) -> Option<T> {


Result<Option<T>, String>

where the Err is the value that cannot be parsed?

or

Result<Option<T>, E>

where E comes from the underlying FromStr

maybe Option<Result> makes more sense?

It would be Result<T, E> (which would be converted to an Option with ok() most of the time, because the error is already added to the warnings).

droogmic · 2022-04-20T16:16:25Z

rust/osm2lanes/src/transform/tags_to_lanes/lane.rs

+
+const LANES: TagKey = TagKey::from("lanes");
+#[derive(Debug)]
+pub struct LanesScheme {


the name of the mod and this struct are unclear for me

in my PR I named this Counts, as it isn't really defining a scheme?

I didn't know what to name the mod. The struct is named LanesScheme because it represents the lanes:* tags (which together make up what I was calling "scheme").

rust/osm2lanes/src/transform/tags_to_lanes/lane.rs

droogmic · 2022-04-20T16:18:59Z

rust/osm2lanes/src/transform/tags_to_lanes/lane.rs

+
+const CENTRE_TURN_LANE: TagKey = TagKey::from("centre_turn_lane");
+pub struct CentreTurnLaneScheme {
+    pub present: Infer<bool>,


what does "present" mean in this context?

It refers to the presence of such a lane: "is a centre turn lane present on this road?".

Is something like struct CentreTurnLaneScheme(Infer<bool>) possible? I haven't used that language feature before.

yes, that would be the correct construct.

https://doc.rust-lang.org/book/ch19-03-advanced-traits.html#using-the-newtype-pattern-to-implement-external-traits-on-external-types

droogmic · 2022-04-20T16:28:41Z

rust/osm2lanes/src/transform/tags_to_lanes/modes/bus.rs

+pub struct BuswayScheme {
+    pub forward_side_direction: Infer<Option<Direction>>,
+    pub backward_side_direction: Infer<Option<Direction>>,
+}


I don't think I found a case of a Backward in forward_side_direction or Forward in backward_side_direction
nor would I expect it.

what would go wrong with BuswayScheme(Infer<(bool, bool)>) to cover the existence of a forward and/or backward lane?

second: this would be nice to add this as a standalone intermediate refactor PR, to make reviewing easier.

I don't think I found a case of a Backward in forward_side_direction or Forward in backward_side_direction
nor would I expect it.

Given this, I agree that BuswayScheme(Infer<(bool, bool)>) is better.

rust/osm2lanes/src/transform/tags_to_lanes/modes/bus.rs

rust/osm2lanes/src/transform/tags_to_lanes/modes/mod.rs

BudgieInWA · 2022-04-21T11:58:41Z

I can agree to this once you look at what I have done and can justify why this is better.

How do we extend this to include taginfo.openstreetmap.org/keys/taxi:lanes?
it feels a lot of the lane handling is hardcoded to bus and only bus?

My intention is to achieve a couple of things:

Separate tag parsing from road building, by introducing Scheme structs that represent the valid states described by smalls sets of tags. BuswayScheme is a good example: the busway tags are a complicated way of describing "is there a bus lane on the forward side, and is there a bus lane on the backward side", so we parse all those tags into BuswayScheme(Infer<(bool, bool)>) and never have to look up a busway tag value again.
Fix the guessing of the total number of lanes, by inverting the order that the tags are looked at.
Move towards tackling the ordering of looking at different tags at the top level tags_to_lanes. a.k.a. express the direction of dependencies between tags in one place. I have only taken a step in that direction, creating BuswayScheme, CentreTurnLaneScheme, and LanesScheme (which take other Scheme arguments to show dependencies), and constructing them in the right order at the top level.

These ideas would be further helped by:

Implementing a Scheme for every tag or set of tags that we parse (even Oneway has way more complexity that we don't support yet).
Bringing the guessing logic out of the Scheme constructors, and into road builder. Maybe Scheme constructors should not Infer::Guess at all, and only return Direct, Calculated (for internally calculated values, like lanes=lanes:forward + lanes:backward) and Default values. At the top level, or as a set of RoadBuilder methods, follow an ordered procedure for building up the road using the Schemes, but no tags.
- This would allow us to remove all dependencies from LanesScheme::new, (except maybe oneway) and figure out the guesses in a context which has access to a RoadBuilder with some lanes already added and the parsed LanesScheme (like Mitigation for more than one bus lanes scheme used #136 (comment)). That step would treat Direct values in LanesScheme differently to Default values.

droogmic · 2022-04-21T12:28:14Z

Ok, but then I think we might be trying to do too much in one PR (which is where the difficulty is coming from).

Either we fix the lane counting, or we fix the scheme parsing. Doing both at the same time is blocking quick progress :)

droogmic · 2022-04-23T17:53:01Z

kotlin is probably failing in the tests.yml file, so I suggest bisecting the changes you made there until kotlin is happy?

droogmic · 2022-04-23T17:53:31Z

my bad, the button for close is very close to the one with comment 😆

droogmic · 2022-04-24T08:39:05Z

data/tests.yml

+    lanes: 4
+    lanes:backward: 1


these are interpreted as integers, when they should be strings, that causes the kotlin tests to fail

dabreegster · 2022-04-25T16:49:58Z

Thanks for splitting into smaller PRs and pushing them through. Just checking in, was closing this PR intentional, or a side effect of the commit message there? Are any of the ideas in this PR still not merged in by one of the smaller PRs?

droogmic · 2022-04-25T17:17:01Z

A side-effect. I removed a lot of stuff from this PR that were not needed to make the tests pass

@BudgieInWA will probably want to go through the final status to reintroduce some of the less critical ideas he had (I removed the Infer::Guessed variant for example), preferably as a TDD when they become needed.

BudgieInWA · 2022-04-26T01:26:05Z

A side-effect. I removed a lot of stuff from this PR that were not needed to make the tests pass

@BudgieInWA will probably want to go through the final status to reintroduce some of the less critical ideas he had (I removed the Infer::Guessed variant for example), preferably as a TDD when they become needed.

This is a great outcome. Getting these ideas our into the open is probably the appropriate amount of progress for now.

BudgieInWA added 3 commits March 25, 2022 17:09

Add a failing test for the case on the website

a491eef

Forward and backward lanes are independently guessed to be half of the lanes, but the tags on the website assume that providing `lanes` and `lanes:backward` is enough.

Rewrite driving_lane_directions to consider all tags together

bf5df9d

Add placeholder warn! calls to show how tag validation can be added t…

dc5de27

…o this structure

droogmic reviewed Mar 25, 2022

View reviewed changes

data/tests.yml Outdated Show resolved Hide resolved

droogmic reviewed Mar 25, 2022

View reviewed changes

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs Outdated Show resolved Hide resolved

droogmic suggested changes Mar 25, 2022

View reviewed changes

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs Outdated Show resolved Hide resolved

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs Outdated Show resolved Hide resolved

droogmic linked an issue Mar 25, 2022 that may be closed by this pull request

Fix Lane Logic on Website #72

Closed

dabreegster reviewed Mar 25, 2022

View reviewed changes

rust/osm2lanes/src/lib.rs Outdated Show resolved Hide resolved

droogmic reviewed Mar 25, 2022

View reviewed changes

rust/osm2lanes/src/transform/tags_to_lanes/mod.rs Outdated Show resolved Hide resolved

BudgieInWA and others added 4 commits March 26, 2022 18:38

replace use of is_some_and

f2c96fc

Co-authored-by: droogmic <droogmic@gmail.com>

Remove unused #![feature(is_some_with)]

fe479f4

Add lanes:{forward,backward} to lanes_to_tags

42ae376

Calculate both_ways in driving_lane_directions too

e3cf57e

Incorporate lanes:both_ways and centre_turn_lanes. Add assumption that oneway=no, lanes=1 means lanes:both_ways=1

BudgieInWA added 6 commits March 27, 2022 20:44

Convert temporary warn! calls into RoadMsg warnings.

b032684

Reveals a bunch of failed tests...

Fix misleading panic message when failing to parse data.yaml

4a30b89

Represent in the tests that odd lane count warns about guessing

295e8bc

Avoid guessing lanes= and lanes:forward= in the tests just yet

f472da3

guess lanes= and lanes:{forward,backward}= in more situations with bu…

28300bc

…sway and lanes:{bus,psv}

fmt

a68efd7

droogmic suggested changes Mar 28, 2022

View reviewed changes

BudgieInWA requested review from dabreegster and droogmic March 28, 2022 17:40

BudgieInWA added 2 commits April 2, 2022 09:54

Start to split parsing into schema understanding, then combining.

c9ca0eb

Probably doesn't compile, but close

wip: Experimental top down rewrite of tag parsing and road building s…

0496070

…tructure. The code is a mess, gesturing generally at the idea, without compiling. I feel like the whole lot should mean close to what it actually says.

BudgieInWA added 4 commits April 20, 2022 18:50

warn on calculated inconsistency, as well as direct

9c94da2

Settle on TurnLanesScheme (with #[allow(unused)])

7303b56

Remove commented code and TODOs

1267f2d

Remove TurnMarking as struct with bools, do we need a bitset?

2f36852

droogmic reviewed Apr 20, 2022

View reviewed changes

data/tests.yml Show resolved Hide resolved

droogmic reviewed Apr 20, 2022

View reviewed changes

rust/osm2lanes/src/locale.rs Show resolved Hide resolved

droogmic suggested changes Apr 20, 2022

View reviewed changes

droogmic reviewed Apr 20, 2022

View reviewed changes

rust/osm2lanes/src/transform/tags_to_lanes/modes/bus.rs Show resolved Hide resolved

droogmic reviewed Apr 20, 2022

View reviewed changes

rust/osm2lanes/src/transform/tags_to_lanes/modes/mod.rs Show resolved Hide resolved

More succinct centre_turn_lane parsing

c8f2774

BudgieInWA added 3 commits April 22, 2022 12:30

rename bothways to proper both_ways

41694ba

rename bothways proper both_ways

c0df013

whitespace

a264b3c

BudgieInWA changed the title ~~Fix lane count calculation~~ Fix lane count calculation - use Scheme structs to avoid re-reading/re-interpreting tags Apr 23, 2022

droogmic closed this Apr 23, 2022

droogmic reopened this Apr 23, 2022

droogmic reviewed Apr 24, 2022

View reviewed changes

This was referenced Apr 24, 2022

Add Lane Count Tests #144

Closed

Add Lane Count Tests #145

Merged

Split of "#111 Fix lane count calculation" #146

Merged

droogmic closed this in b387937 Apr 25, 2022

		// Always assume no center turn lane unless tagged, so we already know:
		let num_both_ways = tagged_both_ways.unwrap_or(tagged_center_turn_lanes.unwrap_or(0));

Fix lane count calculation - use Scheme structs to avoid re-reading/re-interpreting tags #111

Fix lane count calculation - use Scheme structs to avoid re-reading/re-interpreting tags #111

Conversation

BudgieInWA commented Mar 25, 2022 • edited Loading

Are we happy with this overall structure for driving_lane_directions?

Any feedback on my rust style?

How do the roundtrip tests work?

droogmic left a comment

Choose a reason for hiding this comment

droogmic commented Mar 25, 2022

droogmic commented Mar 25, 2022

dabreegster left a comment

Choose a reason for hiding this comment

droogmic commented Mar 27, 2022 via email

droogmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudgieInWA commented Mar 28, 2022

The problem with locking in a final guess on the lanes distribution at the start

droogmic commented Mar 31, 2022 • edited Loading

droogmic commented Mar 31, 2022

Choose a reason for hiding this comment

droogmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudgieInWA commented Apr 21, 2022

droogmic commented Apr 21, 2022

droogmic commented Apr 23, 2022

droogmic commented Apr 23, 2022

Choose a reason for hiding this comment

dabreegster commented Apr 25, 2022

droogmic commented Apr 25, 2022

BudgieInWA commented Apr 26, 2022

BudgieInWA commented Mar 25, 2022 •

edited

Loading

Are we happy with this overall structure for `driving_lane_directions`?

droogmic commented Mar 31, 2022 •

edited

Loading