-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
primary vs perimeter #216
primary vs perimeter #216
Conversation
The perimeter roads are not always the high traffic roads. == PERF This change wasn't in pursuit of performance, but it does seem to have a positive effect. I haven't dug in, but I assume it's largely because we no longer have to do the "Is this segment close to the perimeter?" calculation, which was somewhat expensive. Big improvement in `Neighbourhood::new`: bristol_east/build neighbourhood time: [3.9877 ms 3.9944 ms 4.0019 ms] change: [-47.623% -47.510% -47.390%] (p = 0.00 < 0.05) Performance has improved. bristol_west/build neighbourhood time: [7.7382 ms 7.7747 ms 7.8169 ms] change: [-36.777% -36.434% -36.081%] (p = 0.00 < 0.05) Performance has improved. strasbourg/build neighbourhood time: [4.1041 ms 4.1126 ms 4.1185 ms] change: [-48.165% -47.997% -47.823%] (p = 0.00 < 0.05) Performance has improved. Moderate improvements in shortcuts calculation: shortcuts in bristol_east time: [705.70 µs 706.09 µs 706.53 µs] change: [-10.140% -9.9083% -9.6860%] (p = 0.00 < 0.05) Performance has improved. shortcuts in bristol_west time: [2.7486 ms 2.7512 ms 2.7540 ms] change: [-2.1496% -1.9193% -1.6998%] (p = 0.00 < 0.05) Performance has improved. shortcuts in strasbourg time: [1.2332 ms 1.2350 ms 1.2368 ms] change: [-34.681% -34.518% -34.340%] (p = 0.00 < 0.05) Performance has improved.
), | ||
|b| { | ||
c.benchmark_group(neighbourhood.savefile_name) | ||
.sample_size(neighbourhood.bench_sample_size()) |
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.
Some of the Stasbourg benches were painfully slow, so I've lowered the sample size for them.
Something that's clear from the screenshots, but that I should have called out explicitly, the traffic cells for LTN's with interior primary roads are quite different from before - separating cells by the severance roads. |
Thinking about this a little bit, that feels like a bug. I think cell color is just about communicating connectivity - you can still get between these cells by taking the primary road so they should be the same color. I'm going to revert this to a draft until I fix that. Feel free to hold off on reviewing until then, or you can take it for a spin if you're curious. |
...actually. I'm kind of spiraling on this. Every time I post a message I realize the opposite thing is true. 😆 |
So are those interior primary roads automatically extracted from OSM data? (like primary/secondary roads tags ?). Or can I also modify them? The cells coloring seems hierarchical. 😉 So maybe you could paint that this way:
|
It may be morning before I can review this properly...
Kind of relatedly... sometimes when people draw a boundary with interior perimeter roads, they want to reason about the shortcuts going through them, and place a bus gate or something like that, and see how the traffic will detour. Road classification in the UK (and I'm guessing many places) could've been done a long time ago in different policy contexts, and now there could be a desire to reconsider a "primary road" as actually just being a high street with only buses and delivery traffic. Edit: To clarify, there could be multiple reasons somebody includes a primary (according to OSM) road through the middle of their boundary. I'm not sure if we need to behave differently.
I don't think we can distinguish between the two cases without further input. I'm also not sure how common the second case is -- it's come up during some conversations about real things people are trying to do, but only a few times. |
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.
Code-wise, this looks great. The perf boost is awesome.
I need to think through the implications / questions raised about border intersections for any interaction between a primary and interior road, will do that tomorrow!
// so it counts as disconnected (because it doesn't touch a border intersection), and | ||
// thus we can't calculate shortcuts through it without those intersections. | ||
bail!("No perimeter roads"); | ||
bail!("No primary roads"); |
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.
Interesting... so we can't operate on something at all if it's all residential-highway according to OSM. I don't know any use cases for that in the first place, though
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 meant to call this out - I actually wasn't sure what the original comment was about, but I'm assuming it applies equally to the new primary-road based regime.
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.
Previously, this was kind of impossible unless the user freehand drew a boundary with no roads, or not even a polygon full of roads, and we didn't see any perimeter roads almost parallel to the boundary.
Now, it's defined by primary roads, so you can't do something like this:
I've surfaced the error message better. We can think through if it ever makes sense to allow this -- if the user can override the primary/interior roads, then maybe so.
@@ -80,7 +80,8 @@ | |||
{...layerId("interior-roads-outlines")} | |||
filter={["==", ["get", "kind"], "interior_road"]} | |||
paint={{ | |||
"line-width": lineWidth($thickRoadsForShortcuts, gj.maxShortcuts, 0), | |||
// REVIEW: the interior-roads-outline was previously not visible - was that intentional? |
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 opening a comment to remind myself to look at this...
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.
They are indeed based on OSM tags. Currently you cannot modify them in the LTN app. But I think that's a reasonable idea, and something we could add if we go forward with this direction. |
Ahh... imagine drawing your boundary around the entire study area. The fact that cells split on either side of these severances makes sense. You would just be showing "all" of the LTNs everywhere, on "both sides" of a severance. |
After having it sit in the back of my mind for a couple hours, my current best guess is as @TFCx implied:
|
I think that conclusion sounds good to me too. This PR is a great self-contained step, and we can separately think through the flow for overriding this definition. I'll try a few things out in the morning and likely merge |
All of the examples I'm looking at are great!
Going back to this thought, here's an example Shortcuts go between two primary roads. The perimeter of the drawn boundary is not important at all. This is intuitive to me. Looking again at #47, @TFCx had the right idea a year ago. It took us a while to catch up! |
- Confirm interior road outlines should be visible - Adjust shortcut mode's explanation of a shortcut - Show the error when setting an invalid boundary
Well, now i'm blushing 😊. Everything would have been faster if my english wasn't such an obstacle. 😅 Anyway, all the results are wonderful (and coloring the main arteries in grey (or at least widening the roads) is very intuitive). I can't wait to test it on Montpellier or Vendargues. Just to be sure for the new logic, you should also test it on the non-plannar examples you had (with bridges and tunnels). :) |
Michael, I pushed a commit with a few hopefully not-controversial fixes. I'll merge this now and file another issue to track some next steps for this. Really really awesome!
You explained it quite well originally! I've just been really stuck on some assumptions about what the boundary should look like. Thank you for continuing to push us in the correct direction here.
I tried out the Bristol example and it behaves reasonably. There's a very crazy case in #123 we'll try later. |
@@ -108,10 +108,7 @@ | |||
mode: "neighbourhood", | |||
}; | |||
} catch (err) { | |||
console.log("error when setting auto-generated neighborhood", err); |
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.
No problem with this change, but I'm curious - were you able to trigger this? (How?)
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.
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.
Ah! I think this is related to #216 (comment)
And I've triggered it by having no primary roads in my hand-drawn neighbourood:
Fixes #176
Note: the title of #176 is a bit understated - this isn't just about "hiding a toggle" to streamline the UI. Really this is about refining the domain model. With LTNs we're concerned about the separation of busy roads (
primary_road
) from quieter neighbourhood roads (interior_road
).Because we were using "busy roads" as part of our severance logic (osm tag:
[highway]=primary|secondary|etc.
), the perimeter roads often ended up being busy roads, so they were almost synonymous, but not always. @dabreegster posted some good counter examples.With this PR, we simply use the same severance logic to classify
primary_road
, and use those primary_roads in pretty much the same way we were previously using the perimeter roads.In a lot of cases the output is pretty much identical (minus the shortcuts/calculation and styling change):
before:
after:
But things definitely get more interesting when you have a primary road cutting through your LTN:
before:
after:
before:
after:
One diversion I took: We'd previously talked about having border intersections only on roads that were both
perimeter
andprimary
- but looking at some actual neighbourhoods, I'm not sure that makes sense. I feel like people will be concerned with traffic fromprimary_roads
whether that road is on the perimeter or through the middle of their LTN. Consequently, I opted to define a border intersection as any connection between a primary and interior (aka non-primary) road. Certainly it's up for discussion though!A side benefit is we're no longer using "is_perimeter" for anything in the app. It's kind of an expensive calculation, so if we don't need it, that speeds things up
e.g.
Note #194 is still outstanding.