-
Notifications
You must be signed in to change notification settings - Fork 85
Configurable linking distance and subgraph size #974
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
base: dev
Are you sure you want to change the base?
Conversation
|
Commit 438ebfd gets the CI to work again, so could be cherry-picked into a separate PR. Github has switched to Gradle 9, and a few things in I have confirmed that the This change Latest Github release: r5-v7.4-rc.4-3-g76124de.jar: |
|
It may also make sense to use |
438ebfd to
5200455
Compare
Just trying to understand the context: Do you mean that disconnected subgraphs of sidewalk legitimately exist in the input OSM data, so there are bus stops surrounded by sidewalks that do not connect to the rest of the street network? Does "improvements in the network" mean scenarios containing changes to the street network only, and does "create links to islands" mean that such scenarios add new road links that connect disconnected subgraphs to the rest of the graph? So then the problem is that R5 is removing these disconnected subgraphs before the scenario ever gets a chance to connect them? |
We don't have the gradle wrapper set up. I know it's the recommended way of using gradle but I have always been a bit wary of it. My sense is that like curl-bash it may normalize practices that increase the risk of supply chain attacks. In this case running a script that employs an opaque JAR to download executable code from the internet, then directly run that code to build an application that will be distributed. It may belong to a family of convenience mechanisms that I find a little too complacent. I think the only reason we switched to Gradle was that it allowed dynamically setting the artifact version from |
| public static final int MIN_SUBGRAPH_SIZE = 40; | ||
| private int minSubgraphSize = 40; | ||
|
|
||
| public int getMinSubgraphSize() { |
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.
R5 generally eschews the ceremony of Java accessor methods and uses public fields for everything, treating instances as immutable after construction but not relying on the compiler to enforce 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.
Sure, I can make that change.
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.
Done in ceb8cb0
Yes, that's exactly it - there are lots of places in NC where there is a little bit of sidewalk around a bus stop, and then a sidewalk gap, so the bus stop legitimately is connected to an island (of course the people who use these stops find a way to get to them—walking on major arterials and/or cutting through the bushes—but that's not a very good user experience, which is what I'm modeling here). The working paper about this is here if you're interested. |
5200455 to
ceb8cb0
Compare
This makes the linking distance and minimum subgraph size configurable through the TransportNetworkConfig, and adds a test that it works. There's been a long-standing comment that these should be configurable but it has not previously been done. It also creates two separate linking distances—one for stops and one for pointsets/StreetRouter origins and destinations.
I have a project now where I'm combining R5 with MissingLinks.jl to identify places in the street network where modifications could be made to improve transit accessibility. One of the issues I ran into was that island removal and the very long default maximum linking distance meant that many improvements in the network did not actually have any effect in R5, because they would create links to islands of sidewalk around a bus stop; R5 would remove those islands and link the bus stop to a neighborhood several hundred meters away instead. So I needed to configure these to be lower in that particular application.
This PR doesn't change the defaults so I would not expect any changes to results. I will admit I don't fully know how TransportNetworkConfig is used internally in Conveyal infrastructure, but based on the comments I think that by using boxed ints and doubles and defaulting them to NULL in the config means this will be a backwards-compatible change.