-
Notifications
You must be signed in to change notification settings - Fork 184
Use matrices in FloydWarshallState #38
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,9 +23,9 @@ | |
# > WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
|
||
|
||
type FloydWarshallState<:AbstractPathState | ||
dists::Vector{Vector{Float64}} | ||
parents::Vector{Vector{Int}} | ||
type FloydWarshallState{T<:Real} <: AbstractPathState | ||
dists::Matrix{T} | ||
parents::Matrix{Int} | ||
end | ||
|
||
# @doc doc""" | ||
|
@@ -36,17 +36,17 @@ end | |
# Note that it is possible to consume large amounts of memory as the | ||
# space required for the FloydWarshallState is O(n^2). | ||
# """ -> | ||
function floyd_warshall_shortest_paths( | ||
function floyd_warshall_shortest_paths{T<:Real}( | ||
g::AbstractGraph; | ||
edge_dists::AbstractArray{Float64, 2} = Array(Float64,(0,0)) | ||
edge_dists::AbstractArray{T,2} = zeros(0,0) | ||
) | ||
|
||
use_dists = issparse(edge_dists)? nnz(edge_dists > 0) : !isempty(edge_dists) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I seeing a bug here? This should certainly be We need to include tests that contains sparse graphs. ETA: this was a bug. Fixed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed via #42. |
||
n_v = nv(g) | ||
dists = fill(convert(Float64,Inf), (n_v,n_v)) | ||
parents = zeros(Int, (n_v,n_v)) | ||
S = typeof(one(T) + one(T)) | ||
dists = fill(typemax(S), n_v, n_v) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will break things down the line. I'd prefer to keep edge weights as Float64 since we have an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did think about it, but the only problem I could see is when the longest path length reaches There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue is that this will all break horribly if someone decides to use UInt8, and we have no "max test" like isinf for these other types (except for constructing something with I've avoided type parameterization in LightGraphs precisely because of the complexity it introduced with Graphs.jl. LightGraphs is designed to be quick, easy to understand, but not as flexible as Graphs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sbromberger does the failure in the case of I genuinely do not see how the type parameter use in this function leads to any additional user-facing complexity, loss of code readability or loss of performance. Appealing to the complexity of Graph.jl seems like a slippery slope argument to me - the complexity there comes from forcing users to assign types to everything, even to initialize an empty graph for metadata the user doesn't necessarily care about; here, the eltype is very naturally expressed as a tag for the user specified data, which is the entire point of Julia. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For each type, we need a value that represents "no edge exists". For floats, it's If we can do that with a single (preferably Base) function across all these parameterized types, I think there's an argument for this sort of flexibility. Keep in mind that this will be the first type parameterization in LightGraphs. I am very worried about a slippery slope here. (It might be unfounded.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, it turns out that this statement is false. There are already polymorphic constructors for Graph and Digraph. Furthermore, the type parameters in those functions probably should not be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, it is - my apologies. This was a recent commit to LightGraphs for @jpfairbanks' work with spectral graph theory. The reason they're Just out of interest, why can't one have complex distances? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
OK, but if we had the proposed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For your g = Graph(somedata)
state = connectedcomponents(g)
for vtx1,vtx2 in someiterator
if isconnected(state, vtx1, vtx2)
f(vtx1,vtx2)
end
end We should probably support this api regardless of what we do on F-W Also I agree with @jiahao on the type stuff. I would use rational arithmetic for making an illustrative example in a paper or when giving a class. I think people prefer to do fraction arithmetic over floating point arithmetic in their head. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Keep in mind that distance matrices are deliberately outside the scope of LightGraphs, and the more code we write to support them the less clean the implementation becomes. (Ah - and if we're talking about testing whether components (subgraphs) are connected, that's a different issue and one that I'd prefer to keep in a separate discussion thread. We should probably have a way to do this, but we need a "view" into the graph itself that describes subgraphs, and that's pretty complicated - and will break memory and speed efficiencies.) |
||
parents = zeros(Int, n_v, n_v) | ||
|
||
fws = FloydWarshallState(Vector{Float64}[], Vector{Int}[]) | ||
for v in 1:n_v | ||
dists[v,v] = 0.0 | ||
end | ||
|
@@ -67,23 +67,21 @@ function floyd_warshall_shortest_paths( | |
end | ||
end | ||
for w in vertices(g), u in vertices(g), v in vertices(g) | ||
if dists[u,v] > dists[u,w] + dists[w,v] | ||
dists[u,v] = dists[u,w] + dists[w,v] | ||
if dists[u,w] == typemax(S) || dists[w,v] == typemax(S) | ||
continue | ||
end | ||
d = dists[u,w] + dists[w,v] | ||
if dists[u,v] > d | ||
dists[u,v] = d | ||
parents[u,v] = parents[w,v] | ||
end | ||
end | ||
for r in 1:size(parents,1) # row by row | ||
push!(fws.parents, vec(parents[r,:])) | ||
end | ||
for r in 1:size(dists,1) | ||
push!(fws.dists, vec(dists[r,:])) | ||
end | ||
|
||
return fws | ||
return FloydWarshallState{S}(dists, parents) | ||
end | ||
|
||
function enumerate_paths(s::FloydWarshallState, v::Integer) | ||
pathinfo = s.parents[v] | ||
function enumerate_paths(s::FloydWarshallState, v::Int) | ||
pathinfo = slice(s.parents, v, :) | ||
paths = Vector{Int}[] | ||
for i in 1:length(pathinfo) | ||
if i == v | ||
|
@@ -101,5 +99,5 @@ function enumerate_paths(s::FloydWarshallState, v::Integer) | |
return paths | ||
end | ||
|
||
enumerate_paths(s::FloydWarshallState) = [enumerate_paths(s, v) for v in 1:length(s.parents)] | ||
enumerate_paths(st::FloydWarshallState, s::Integer, d::Integer) = enumerate_paths(st, s)[d] | ||
enumerate_paths(s::FloydWarshallState) = [enumerate_paths(s, v) for v in 1:size(s.parents, 1)] | ||
enumerate_paths(st::FloydWarshallState, s::Int, d::Int) = enumerate_paths(st, s)[d] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,22 @@ | ||
d = float([ 0 1 2 3 4; 5 0 6 7 8; 9 10 0 11 12; 13 14 15 0 16; 17 18 19 20 0]) | ||
z = floyd_warshall_shortest_paths(g3; edge_dists=d) | ||
@test z.dists[3] == [7, 6, 0, 11, 27] | ||
@test z.parents[3] == [2, 3, 0, 3, 4] | ||
d = [ 0 1 2 3 4; 5 0 6 7 8; 9 10 0 11 12; 13 14 15 0 16; 17 18 19 20 0] | ||
|
||
for T in (Int8, Int16, Int32, Int64, Int128, | ||
Float16, Float32, Float64) | ||
|
||
z = floyd_warshall_shortest_paths(g3; edge_dists=convert(Matrix{T}, d)) | ||
@test z.dists == [ | ||
0 1 7 18 34 | ||
1 0 6 17 33 | ||
7 6 0 11 27 | ||
18 17 11 0 16 | ||
34 33 27 16 0] | ||
@test z.parents == [ | ||
0 1 2 3 4 | ||
2 0 2 3 4 | ||
2 3 0 3 4 | ||
2 3 4 0 4 | ||
2 3 4 5 0] | ||
end | ||
|
||
@test enumerate_paths(z)[2][2] == [] | ||
@test enumerate_paths(z)[2][4] == enumerate_paths(z,2)[4] == enumerate_paths(z,2,4) == [2,3,4] |
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.
Should this be
zeros(T,0,0)
instead?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. A method definition with a default argument is exactly equivalent to two separate method definitions, one of which sets the default value.
is exactly equivalent to
Therefore, the default argument must already specify a concrete type and cannot refer to a typevar. (You cannot have a method signature
f{T}()
.)Were you to change the method definition to
f{T}(x::T=zero(T))
, you would get a runtime error becauseT
is not defined at runtime in the scope of the method that runszero(T)
.