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

[WIP] refactor routing algorithms #58

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[WIP] refactor routing algorithms #58

wants to merge 3 commits into from

Conversation

yeesian
Copy link
Collaborator

@yeesian yeesian commented Jan 8, 2015

to use hasparent (to be introduced in Graphs.jl); not ready for merge yet!

@yeesian
Copy link
Collaborator Author

yeesian commented Jan 8, 2015

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

@yeesian
Copy link
Collaborator Author

yeesian commented Jan 9, 2015

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.

@tedsteiner
Copy link
Owner

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.

@yeesian
Copy link
Collaborator Author

yeesian commented Jan 12, 2015

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

@tedsteiner
Copy link
Owner

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!

@tedsteiner tedsteiner added this to the Release 0.8 milestone Jan 14, 2015
This was referenced Jan 19, 2015
@yeesian
Copy link
Collaborator Author

yeesian commented Jan 22, 2015

related: JuliaAttic/OldGraphs.jl#160

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

Successfully merging this pull request may close these issues.

2 participants