Skip to content
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

Ferry Reclassification - Can reclassify nearly all edges on small islands #4358

Closed
dnesbitt61 opened this issue Oct 26, 2023 · 4 comments · Fixed by #4378
Closed

Ferry Reclassification - Can reclassify nearly all edges on small islands #4358

dnesbitt61 opened this issue Oct 26, 2023 · 4 comments · Fixed by #4378
Labels

Comments

@dnesbitt61
Copy link
Member

I found a case in NY (other cases may be likely) on Fishers Island, NY (south of New London, Connecticut) where all edges are reclassified when reclassifying edges connected to ferries. This island has no ways/edges that meet the target classification (primary) - so the algorithm continues to expand from the ferry and essentially reclassify nearly all edges. The algorithm continues until the adjacency queue is empty or:

  if (n > 400 && GetBestNonFerryClass(expanded_bundle.node_edges) <= kFerryUpClass) {
    break;
  }

the 2nd half of this condition is never met on Fisher Island.

I think we should:

  1. Alter this condition to only increment n if the 2nd condition is met and break sooner (e.g. maybe after 32 nodes meet this criteria).
  2. If after breaking out of the loop (or adj queue is empty which exits the loop) there are no connections to nodes with edges meeting the road classification target perhaps we should not reclassify any edges.
@kevinkreiser
Copy link
Member

if we dont reclassify them the island becomes a "destination (or origin) only" island right? this would essentially make rural ferry ports unusable on longer routes, which might be ok but it would be interesting to see on how many ferries the reclassify algorithm would give up. we need to log these so we can at least quanitfy the problem and maybe hone in on a magical value that works for most ports. maybe its 32, maybe its 42, maybe 111?

also maybe it might be better to base it on distance rather than on number of nodes. after we fix the other issue were we only upclass the shortest path, if we find that the expansion has made it 10 miles (or whatever number) away from the ferry or something maybe its best not to upclass anything there.

@gknisely
Copy link
Member

The answer is 42. Always.

@nilsnolde
Copy link
Member

nilsnolde commented Oct 27, 2023

The main reason why the referenced condition in there like that, seems to be that some ferry connections are high class but only connect to low classes further inland, right? (FWIW, the Tsawwassen terminal is only trunk roads these days)

Couldn't we do smth like we do for dest_only edges connecting to ferries: expand from the connection edge and if it's a primary or higher don't let the GetBestNonFerryClass condition kick in as long as we didn't expand onto a lower class first? If there's stuff that does this

----------x--------------x-----------------x----------------x--------------------x
ferry       primary        service           primary           service

that wouldn't hold of course, but honestly, that seems pretty messed up.

We'd need to protect against ferry terminals like Tsawwassen where the ferry directly connects to the full trunk network, e.g. if we saw more than 20-30 primary edges in a row, we'd break and assume we don't need to upclass. It's not accurate either in all situations but I think it might lead to less problems.

Then we wouldn't have to care about any random limit of found primaries or even number of iterations. Any limit would (and currently does) very likely also cause non-shortest paths to be upclassed if the 33rd primary road is randomly somewhere pretty far away (though a distance instead of node limit would alleviate some of that). And if there's no primary to be found, the adj list is run empty and we don't upclass anything (I agree, that's a gross oversight that we still do upclass currently).

@nilsnolde
Copy link
Member

Hm, just seeing that we don't even try to reclassify if the connection edge is a primary.. I wonder what Tsawwassen looked like before.. Still, the thought is still similar: if we expand on more than 20-30 consecutive primary (or higher) edges on the same expansion branch, we assume it's fine, break and reclassify?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants