Skip to content
This repository was archived by the owner on Apr 26, 2019. It is now read-only.

search: fix handling of zero-weight cycle case #73

Merged
kortschak merged 1 commit into from
Jun 1, 2015
Merged

search: fix handling of zero-weight cycle case #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions search/floydwarshall.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func FloydWarshall(g graph.Graph, weight graph.CostFunc) (paths ShortestPaths, o
joint := paths.dist.At(i, k) + paths.dist.At(k, j)
if ij > joint {
paths.set(i, j, joint, paths.at(i, k)...)
} else if i != k && k != j && ij-joint == 0 {
} else if ij-joint == 0 {
paths.add(i, j, paths.at(i, k)...)
}
}
Expand Down Expand Up @@ -147,7 +147,8 @@ func (p ShortestPaths) Weight(u, v graph.Node) float64 {

// Between returns a shortest path from u to v and the weight of the path. If more than
// one shortest path exists between u and v, a randomly chosen path will be returned and
// unique is returned false.
// unique is returned false. If a cycle with zero weight exists in the path, it will not
// be included, but unique will be returned false.
func (p ShortestPaths) Between(u, v graph.Node) (path []graph.Node, weight float64, unique bool) {
from, fromOK := p.indexOf[u.ID()]
to, toOK := p.indexOf[v.ID()]
Expand All @@ -156,38 +157,65 @@ func (p ShortestPaths) Between(u, v graph.Node) (path []graph.Node, weight float
}
path = []graph.Node{p.nodes[from]}
unique = true
seen := make([]int, len(p.nodes))
for i := range seen {
seen[i] = -1
}
seen[from] = 0
// TODO(kortschak): Consider a more progressive approach
// to handling zero-weight cycles. One way is outlined
// here https://github.com/gonum/graph/pull/73#discussion_r31398601
for from != to {
c := p.at(from, to)
if len(c) != 1 {
unique = false
}
from = c[rand.Intn(len(c))]
i := rand.Intn(len(c))
from = c[i]
if seen[from] >= 0 {
Copy link
Copy Markdown
Member

@vladimir-ch vladimir-ch Jun 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything that prevents (repeatedly) take the same zero-weight cycle? There may theoretically be many such cycles to take from a node but only one non-cycle path to advance the algorithm.

Here if such a cycle is detected, path is reverted, but on the next iteration from is advanced randomly to the next node on a shortest path which may lie on the same cycle. The path will be trimmed again but from will be already on the cycle. Is my understanding correct or am I missing something?

Copy link
Copy Markdown
Member Author

@kortschak kortschak Jun 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything that prevents (repeatedly) take the same zero-weight cycle? There may theoretically be many such cycles to take from a node but only one non-cycle path to advance the algorithm.

I had something like that in the previous commit, and it was horribly incorrect. I'd be happy to see an approach that deals with this deterministically without excessive allocation. I can't think of one.

At the moment, I depend on probability. Also, zero weight cycles should be a reasonably rare case.

The path will be trimmed again but from will be already on the cycle. Is my understanding correct or am I missing something?

The path is not completely reverted, it is trimmed to one after the seen[from]. I have to say that I am quite unsure of this code, so if you can find a failing case for this, I'd be very grateful. I did many tests runs and I could not get any of the cases in the tests to fail.

Copy link
Copy Markdown
Member Author

@kortschak kortschak Jun 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... adding this test still passes:

    {
        name: "zero-weight n·cycle directed",
        g:    func() graph.Mutable { return concrete.NewDirectedGraph() },
        edges: []concrete.WeightedEdge{
            // Add a path from 0->4 of weight 4
            {concrete.Edge{concrete.Node(0), concrete.Node(1)}, 1},
            {concrete.Edge{concrete.Node(1), concrete.Node(2)}, 1},
            {concrete.Edge{concrete.Node(2), concrete.Node(3)}, 1},
            {concrete.Edge{concrete.Node(3), concrete.Node(4)}, 1},

            // Add n zero-weight cycles.
            {concrete.Edge{concrete.Node(1), concrete.Node(5)}, 0},
            {concrete.Edge{concrete.Node(5), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(6)}, 0},
            {concrete.Edge{concrete.Node(6), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(7)}, 0},
            {concrete.Edge{concrete.Node(7), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(8)}, 0},
            {concrete.Edge{concrete.Node(8), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(9)}, 0},
            {concrete.Edge{concrete.Node(9), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(10)}, 0},
            {concrete.Edge{concrete.Node(10), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(11)}, 0},
            {concrete.Edge{concrete.Node(11), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(12)}, 0},
            {concrete.Edge{concrete.Node(12), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(13)}, 0},
            {concrete.Edge{concrete.Node(13), concrete.Node(1)}, 0},
            {concrete.Edge{concrete.Node(1), concrete.Node(14)}, 0},
            {concrete.Edge{concrete.Node(14), concrete.Node(1)}, 0},
        },

        query:  concrete.Edge{concrete.Node(0), concrete.Node(4)},
        weight: 4,
        want: [][]int{
            {0, 1, 2, 3, 4},
        },
        unique: false,

        none: concrete.Edge{concrete.Node(4), concrete.Node(5)},
    },

Copy link
Copy Markdown
Member

@vladimir-ch vladimir-ch Jun 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zero weight cycles should be a reasonably rare case

I can imagine, that's why I wrote "theoretically".

The path is not completely reverted, it is trimmed to one after the seen[from]

Yes, that's what I meant. Sorry.

I think the code is correct. I originally thought that this repeated walk over zero-weight cycles could be avoided, but no, the correct path can stem from somewhere in the cycle. Negative weights seem to be quite a huge annoyance and the non-determinism in there is scary.

Copy link
Copy Markdown
Member

@vladimir-ch vladimir-ch Jun 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I meant. Sorry.

In fact, that's not what I meant :-) I thought that the path is trimmed back to the beginning of the cycle, which was a ridiculous thought.

Copy link
Copy Markdown
Member Author

@kortschak kortschak Jun 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a more general test for the non-productive path. I'll add that. It works with n=100, but is very slow with n=1000 (33s to complete cf. 0.005s for the n=10 case). If it becomes a problem, we can randomly permute the c slice and iterate through that slice, removing the chosen element. Can we leave this for a later PR when it is seen to be necessary?

Copy link
Copy Markdown
Member

@vladimir-ch vladimir-ch Jun 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Copy link
Copy Markdown
Member Author

@kortschak kortschak Jun 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a go at it and it's not trivial. I think we actually need to keep a map[struct{from, to int}][]int and query that before calling p.at(from, to). If it's not present, copy from p.at(from, to) to the map at struct{from, to int}{from, to} and update the slice there. This means we only allocate when we are on a cycle and we don't disturb the internal representation.

path = path[:seen[from]+1]
continue
}
seen[from] = len(path)
path = append(path, p.nodes[from])
}
// We need to re-access from in this case because from has been mutated.
return path, p.dist.At(p.indexOf[u.ID()], to), unique
}

// AllBetween returns all shortest paths from u to v and the weight of the paths.
// AllBetween returns all shortest paths from u to v and the weight of the paths. Paths
// containing zero-weight cycles are not returned.
func (p ShortestPaths) AllBetween(u, v graph.Node) (paths [][]graph.Node, weight float64) {
from, fromOK := p.indexOf[u.ID()]
to, toOK := p.indexOf[v.ID()]
if !fromOK || !toOK || len(p.at(from, to)) == 0 {
return nil, math.Inf(1)
}
paths = p.allBetween(from, to, []graph.Node{p.nodes[from]}, nil)
seen := make([]bool, len(p.nodes))
seen[from] = true
paths = p.allBetween(from, to, seen, []graph.Node{p.nodes[from]}, nil)
return paths, p.dist.At(from, to)
}

func (p ShortestPaths) allBetween(from, to int, path []graph.Node, paths [][]graph.Node) [][]graph.Node {
func (p ShortestPaths) allBetween(from, to int, seen []bool, path []graph.Node, paths [][]graph.Node) [][]graph.Node {
if from == to {
if path == nil {
return paths
}
return append(paths, path)
}
for i, from := range p.at(from, to) {
if i != 0 {
first := true
for _, from := range p.at(from, to) {
if seen[from] {
continue
}
if first {
path = append([]graph.Node(nil), path...)
first = false
}
paths = p.allBetween(from, to, append(path, p.nodes[from]), paths)
s := append([]bool(nil), seen...)
s[from] = true
paths = p.allBetween(from, to, s, append(path, p.nodes[from]), paths)
}
return paths
}
141 changes: 131 additions & 10 deletions search/floydwarshall_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ var floydWarshallTests = []struct {
query concrete.Edge
weight float64
want [][]int
unique bool

none concrete.Edge
}{
Expand Down Expand Up @@ -57,6 +58,7 @@ var floydWarshallTests = []struct {
want: [][]int{
{0, 1},
},
unique: true,

none: concrete.Edge{concrete.Node(2), concrete.Node(3)},
},
Expand All @@ -72,6 +74,7 @@ var floydWarshallTests = []struct {
want: [][]int{
{0, 1},
},
unique: true,

none: concrete.Edge{concrete.Node(2), concrete.Node(3)},
},
Expand All @@ -90,6 +93,7 @@ var floydWarshallTests = []struct {
{0, 1, 2},
{0, 2},
},
unique: false,

none: concrete.Edge{concrete.Node(2), concrete.Node(1)},
},
Expand All @@ -108,6 +112,7 @@ var floydWarshallTests = []struct {
{0, 1, 2},
{0, 2},
},
unique: false,

none: concrete.Edge{concrete.Node(2), concrete.Node(4)},
},
Expand Down Expand Up @@ -141,6 +146,7 @@ var floydWarshallTests = []struct {
{0, 2, 3, 5},
{0, 5},
},
unique: false,

none: concrete.Edge{concrete.Node(4), concrete.Node(5)},
},
Expand Down Expand Up @@ -174,6 +180,7 @@ var floydWarshallTests = []struct {
{0, 2, 3, 5},
{0, 5},
},
unique: false,

none: concrete.Edge{concrete.Node(5), concrete.Node(6)},
},
Expand Down Expand Up @@ -208,6 +215,7 @@ var floydWarshallTests = []struct {
{0, 2, 3, 5},
{0, 6, 5},
},
unique: false,

none: concrete.Edge{concrete.Node(4), concrete.Node(5)},
},
Expand Down Expand Up @@ -242,9 +250,122 @@ var floydWarshallTests = []struct {
{0, 2, 3, 5},
{0, 6, 5},
},
unique: false,

none: concrete.Edge{concrete.Node(5), concrete.Node(7)},
},
{
name: "zero-weight cycle directed",
g: func() graph.Mutable { return concrete.NewDirectedGraph() },
edges: []concrete.WeightedEdge{
// Add a path from 0->4 of weight 4
{concrete.Edge{concrete.Node(0), concrete.Node(1)}, 1},
{concrete.Edge{concrete.Node(1), concrete.Node(2)}, 1},
{concrete.Edge{concrete.Node(2), concrete.Node(3)}, 1},
{concrete.Edge{concrete.Node(3), concrete.Node(4)}, 1},

// Add a zero-weight cycle.
{concrete.Edge{concrete.Node(1), concrete.Node(5)}, 0},
{concrete.Edge{concrete.Node(5), concrete.Node(1)}, 0},
},

query: concrete.Edge{concrete.Node(0), concrete.Node(4)},
weight: 4,
want: [][]int{
{0, 1, 2, 3, 4},
},
unique: false,

none: concrete.Edge{concrete.Node(4), concrete.Node(5)},
},
{
name: "zero-weight cycle^2 directed",
g: func() graph.Mutable { return concrete.NewDirectedGraph() },
edges: []concrete.WeightedEdge{
// Add a path from 0->4 of weight 4
{concrete.Edge{concrete.Node(0), concrete.Node(1)}, 1},
{concrete.Edge{concrete.Node(1), concrete.Node(2)}, 1},
{concrete.Edge{concrete.Node(2), concrete.Node(3)}, 1},
{concrete.Edge{concrete.Node(3), concrete.Node(4)}, 1},

// Add a zero-weight cycle.
{concrete.Edge{concrete.Node(1), concrete.Node(5)}, 0},
{concrete.Edge{concrete.Node(5), concrete.Node(1)}, 0},
// With its own zero-weight cycle.
{concrete.Edge{concrete.Node(5), concrete.Node(5)}, 0},
},

query: concrete.Edge{concrete.Node(0), concrete.Node(4)},
weight: 4,
want: [][]int{
{0, 1, 2, 3, 4},
},
unique: false,

none: concrete.Edge{concrete.Node(4), concrete.Node(5)},
},
{
name: "zero-weight cycle^3 directed",
g: func() graph.Mutable { return concrete.NewDirectedGraph() },
edges: []concrete.WeightedEdge{
// Add a path from 0->4 of weight 4
{concrete.Edge{concrete.Node(0), concrete.Node(1)}, 1},
{concrete.Edge{concrete.Node(1), concrete.Node(2)}, 1},
{concrete.Edge{concrete.Node(2), concrete.Node(3)}, 1},
{concrete.Edge{concrete.Node(3), concrete.Node(4)}, 1},

// Add a zero-weight cycle.
{concrete.Edge{concrete.Node(1), concrete.Node(5)}, 0},
{concrete.Edge{concrete.Node(5), concrete.Node(1)}, 0},
// With its own zero-weight cycle.
{concrete.Edge{concrete.Node(5), concrete.Node(6)}, 0},
{concrete.Edge{concrete.Node(6), concrete.Node(5)}, 0},
// With its own zero-weight cycle.
{concrete.Edge{concrete.Node(6), concrete.Node(6)}, 0},
},

query: concrete.Edge{concrete.Node(0), concrete.Node(4)},
weight: 4,
want: [][]int{
{0, 1, 2, 3, 4},
},
unique: false,

none: concrete.Edge{concrete.Node(4), concrete.Node(5)},
},
{
name: "zero-weight n·cycle directed",
g: func() graph.Mutable { return concrete.NewDirectedGraph() },
edges: func() []concrete.WeightedEdge {
e := []concrete.WeightedEdge{
// Add a path from 0->4 of weight 4
{concrete.Edge{concrete.Node(0), concrete.Node(1)}, 1},
{concrete.Edge{concrete.Node(1), concrete.Node(2)}, 1},
{concrete.Edge{concrete.Node(2), concrete.Node(3)}, 1},
{concrete.Edge{concrete.Node(3), concrete.Node(4)}, 1},
}
next := len(e) + 1

// Add n zero-weight cycles.
const n = 100
for i := 0; i < n; i++ {
e = append(e,
concrete.WeightedEdge{concrete.Edge{concrete.Node(next + i), concrete.Node(i)}, 0},
concrete.WeightedEdge{concrete.Edge{concrete.Node(i), concrete.Node(next + i)}, 0},
)
}
return e
}(),

query: concrete.Edge{concrete.Node(0), concrete.Node(4)},
weight: 4,
want: [][]int{
{0, 1, 2, 3, 4},
},
unique: false,

none: concrete.Edge{concrete.Node(4), concrete.Node(5)},
},
}

func TestFloydWarshall(t *testing.T) {
Expand Down Expand Up @@ -277,9 +398,9 @@ func TestFloydWarshall(t *testing.T) {
t.Errorf("%q: unexpected weight from Weight: got:%f want:%f",
test.name, weight, test.weight)
}
if unique != (len(test.want) == 1) {
if unique != test.unique {
t.Errorf("%q: unexpected number of paths: got: unique=%t want: unique=%t",
test.name, unique, len(test.want) == 1)
test.name, unique, test.unique)
}

var got []int
Expand All @@ -297,12 +418,12 @@ func TestFloydWarshall(t *testing.T) {
t.Errorf("%q: unexpected shortest path:\ngot: %v\nwant from:%v",
test.name, p, test.want)
}
}

np, weight, unique := pt.Between(test.none.From(), test.none.To())
if np != nil || !math.IsInf(weight, 1) || unique != false {
t.Errorf("%q: unexpected path:\ngot: path=%v weight=%f unique=%t\nwant:path=<nil> weight=+Inf unique=false",
test.name, np, weight, unique)
}
np, weight, unique := pt.Between(test.none.From(), test.none.To())
if np != nil || !math.IsInf(weight, 1) || unique != false {
t.Errorf("%q: unexpected path:\ngot: path=%v weight=%f unique=%t\nwant:path=<nil> weight=+Inf unique=false",
test.name, np, weight, unique)
}

paths, weight := pt.AllBetween(test.query.From(), test.query.To())
Expand All @@ -326,10 +447,10 @@ func TestFloydWarshall(t *testing.T) {
test.name, got, test.want)
}

np, weight := pt.AllBetween(test.none.From(), test.none.To())
if np != nil || !math.IsInf(weight, 1) {
nps, weight := pt.AllBetween(test.none.From(), test.none.To())
if nps != nil || !math.IsInf(weight, 1) {
t.Errorf("%q: unexpected path:\ngot: paths=%v weight=%f\nwant:path=<nil> weight=+Inf",
test.name, np, weight)
test.name, nps, weight)
}
}
}