OpenStreetMap.jl
OpenStreetMap.jl copied to clipboard
tedsteiner
[WIP] refactor routing algorithms
One thing that hasn't made sense to me yet: why are the nodes in the road network (constructed by createGraph) represented as follows:
Dict{Int64,KeyVertex{Int64}} with 92 entries:
575440648 => KeyVertex{Int64}(1,575440648)
1053454370 => KeyVertex{Int64}(2,1053454370)
2 => KeyVertex{Int64}(3,2)
⋮ => ⋮
and not
Dict{Int64,KeyVertex{Int64}} with 92 entries:
575440648 => 1
1053454370 => 2
2 => 3
⋮ => ⋮
or
[575440648, 1053454370, 2, ...] # Vector{Int} with 92 entries
Okay, I think I've gotten to the root of it: the KeyVertex was used for the OSM ID to keep track of its index within the road network that we construct. This had the unfortunate consequence of forcing us to write collect(keys(network.v)), when we really meant to write network.v (to retrieve the vertices in the [road] network)
My proposal is that we introduce an index field (to the Network type) as follows:
type Network
g # Graph object
v::Vector{Int} # OSM IDs
index::Dict{Int,Int} # (OSM ID) => index (for the reverse mapping)
w::Vector{Float64} # Edge weights
class::Vector{Int} # Road class of each edge
end
This makes it easier to reason about the Network, without having to second-guess what we're working with -- It's unclear why the nodes shouldn't be the OSM IDs themselves, and the additional indices (in KeyVertex) was really an implementation detail that I don't usually care about (as a user). Pretty often I had to ask --
- is it a node, an index, or a OSM ID?
- is it the key, or the vertex that I want?
and I think a natural choice should be: Node = OSM ID, Edge = (OSM ID -> OSM ID). The index is then introduced for the off-chance we might need the index of a particular OSM ID.
Yes, Node = OSM ID, Edge = (OSM ID -> OSM ID) sounds nice. I think this is what I was going for originally, but it has been like 6 months now since I wrote that code and I don't remember exactly. I know I've gone through and tried to keep as little extraneous information as possible in the routing graph, so there is less chance for error.
I think the reasoning behind
v::Dict{Int,Graphs.KeyVertex{Int}} # (node id) => (graph vertex)
was that the KeyVertex object is needed to get information out of the graph, or something like that. I was having an issue with just giving the graph an Int and getting the vertex I wanted. But like I said, I'm having a really hard time remembering.
Yeah, I've gone through the code (replacing everything), and the only place where it was needed so far was in extractRoutes, where you need to be able to retrieve the ID of the node, in order to look out its parent (from hasparent), but hasparent would describe the OSM ID instead.
I do think that it's cleaner to have a Dict index-mapping from the OSM IDs to their indices in the graph though. Sorry I'd been away on a ski trip, and will continue working on it when I'm back. I'll let you know when this PR is ready for review
Ok, if there's a cleaner way to do it, then that's cool with me. And don't worry about delays, this won't add any functionality to me so I'm in no rush.
By the way, I've added you as a collaborator to the repository, since you've been doing so much work with the project and I haven't been all that great at keeping up with your pace lately. Thanks for all your help so far!