From 142f1a8c6bcecd856612c9f573ab3b613d1de68a Mon Sep 17 00:00:00 2001 From: Dan Kortschak Date: Fri, 12 Jun 2020 09:30:06 +0930 Subject: [PATCH] graph: make graph analysis routines safe for indeterminate iterators This is a change in design for the graph.NodesOf family of functions. The alternative was to provide an equivalent set of non-panicking routines in graph for internal use. The protection that was intended with the panic was to panic early rather than late when an indeterminate iterator exhausts slice index space. I think in hindsight this was an error and we should let things blow up in that (likely rare) situation. The majority of changes are in test code. Outside the iterator package, which is intimately tied to the determined iterator implementations, only one test now fails if an indeterminate iterator is used, product's Modular extended sub-graph isomorphism example, which is an algorithm that would have time complexity issues with large iterators anyway. --- graph/complement_test.go | 6 +- graph/graphs/gen/batagelj_brandes_test.go | 44 ++++----- graph/layout/eades.go | 6 +- graph/layout/plotter_test.go | 21 +++- graph/nodes_edges.go | 95 ++++++++++--------- graph/path/a_star_test.go | 5 +- graph/path/dynamic/dstarlite_test.go | 5 +- .../path/internal/testgraphs/limited_test.go | 5 +- graph/path/johnson_apsp.go | 3 + graph/product/product_test.go | 36 +++---- graph/spectral/laplacian.go | 7 +- graph/testgraph/testgraph.go | 12 +-- 12 files changed, 138 insertions(+), 107 deletions(-) diff --git a/graph/complement_test.go b/graph/complement_test.go index 0e91e28f..ab57138e 100644 --- a/graph/complement_test.go +++ b/graph/complement_test.go @@ -28,7 +28,7 @@ var complementTests = []struct { func TestComplement(t *testing.T) { for _, test := range complementTests { - n := test.g.Nodes().Len() + n := len(graph.NodesOf(test.g.Nodes())) wantM := n * (n - 1) // Double counting edges, but no self-loops. var gotM int @@ -72,6 +72,10 @@ var nodeFilterIteratorTests = []struct { func TestNodeFilterIterator(t *testing.T) { for _, test := range nodeFilterIteratorTests { it := graph.NewNodeFilterIterator(test.src, test.filter, test.root) + if it.Len() < 0 { + t.Logf("don't test indeterminate iterators: %T", it) + continue + } for i := 0; i < 2; i++ { n := it.Len() if n != test.len { diff --git a/graph/graphs/gen/batagelj_brandes_test.go b/graph/graphs/gen/batagelj_brandes_test.go index fc49e3a2..796c33fe 100644 --- a/graph/graphs/gen/batagelj_brandes_test.go +++ b/graph/graphs/gen/batagelj_brandes_test.go @@ -221,17 +221,16 @@ func TestPowerLawUndirected(t *testing.T) { t.Fatalf("unexpected error: n=%d, d=%d: %v", n, d, err) } - nodes := g.Nodes() - if nodes.Len() != n { - t.Errorf("unexpected number of nodes in graph: n=%d, d=%d: got:%d", n, d, nodes.Len()) + nodes := graph.NodesOf(g.Nodes()) + if len(nodes) != n { + t.Errorf("unexpected number of nodes in graph: n=%d, d=%d: got:%d", n, d, len(nodes)) } - for nodes.Next() { - u := nodes.Node() + for _, u := range nodes { uid := u.ID() var lines int for _, v := range graph.NodesOf(g.From(uid)) { - lines += g.Lines(uid, v.ID()).Len() + lines += len(graph.LinesOf(g.Lines(uid, v.ID()))) } if lines < d { t.Errorf("unexpected degree below d: n=%d, d=%d: got:%d", n, d, lines) @@ -252,17 +251,16 @@ func TestPowerLawDirected(t *testing.T) { t.Fatalf("unexpected error: n=%d, d=%d: %v", n, d, err) } - nodes := g.Nodes() - if nodes.Len() != n { - t.Errorf("unexpected number of nodes in graph: n=%d, d=%d: got:%d", n, d, nodes.Len()) + nodes := graph.NodesOf(g.Nodes()) + if len(nodes) != n { + t.Errorf("unexpected number of nodes in graph: n=%d, d=%d: got:%d", n, d, len(nodes)) } - for nodes.Next() { - u := nodes.Node() + for _, u := range nodes { uid := u.ID() var lines int for _, v := range graph.NodesOf(g.From(uid)) { - lines += g.Lines(uid, v.ID()).Len() + lines += len(graph.LinesOf(g.Lines(uid, v.ID()))) } if lines < d { t.Errorf("unexpected degree below d: n=%d, d=%d: got:%d", n, d, lines) @@ -283,9 +281,9 @@ func TestBipartitePowerLawUndirected(t *testing.T) { t.Fatalf("unexpected error: n=%d, d=%d: %v", n, d, err) } - nodes := g.Nodes() - if nodes.Len() != 2*n { - t.Errorf("unexpected number of nodes in graph: n=%d, d=%d: got:%d", n, d, nodes.Len()) + nodes := graph.NodesOf(g.Nodes()) + if len(nodes) != 2*n { + t.Errorf("unexpected number of nodes in graph: n=%d, d=%d: got:%d", n, d, len(nodes)) } if len(p1) != n { t.Errorf("unexpected number of nodes in p1: n=%d, d=%d: got:%d", n, d, len(p1)) @@ -307,12 +305,11 @@ func TestBipartitePowerLawUndirected(t *testing.T) { t.Errorf("unexpected overlap in partition membership: n=%d, d=%d: got:%d", n, d, len(o)) } - for nodes.Next() { - u := nodes.Node() + for _, u := range nodes { uid := u.ID() var lines int for _, v := range graph.NodesOf(g.From(uid)) { - lines += g.Lines(uid, v.ID()).Len() + lines += len(graph.LinesOf(g.Lines(uid, v.ID()))) } if lines < d { t.Errorf("unexpected degree below d: n=%d, d=%d: got:%d", n, d, lines) @@ -333,9 +330,9 @@ func TestBipartitePowerLawDirected(t *testing.T) { t.Fatalf("unexpected error: n=%d, d=%d: %v", n, d, err) } - nodes := g.Nodes() - if nodes.Len() != 2*n { - t.Errorf("unexpected number of nodes in graph: n=%d, d=%d: got:%d", n, d, nodes.Len()) + nodes := graph.NodesOf(g.Nodes()) + if len(nodes) != 2*n { + t.Errorf("unexpected number of nodes in graph: n=%d, d=%d: got:%d", n, d, len(nodes)) } if len(p1) != n { t.Errorf("unexpected number of nodes in p1: n=%d, d=%d: got:%d", n, d, len(p1)) @@ -357,12 +354,11 @@ func TestBipartitePowerLawDirected(t *testing.T) { t.Errorf("unexpected overlap in partition membership: n=%d, d=%d: got:%d", n, d, len(o)) } - for nodes.Next() { - u := nodes.Node() + for _, u := range nodes { uid := u.ID() var lines int for _, v := range graph.NodesOf(g.From(uid)) { - lines += g.Lines(uid, v.ID()).Len() + lines += len(graph.LinesOf(g.Lines(uid, v.ID()))) } if lines < d { t.Errorf("unexpected degree below d: n=%d, d=%d: got:%d", n, d, lines) diff --git a/graph/layout/eades.go b/graph/layout/eades.go index 0ba5f323..a8e35ac6 100644 --- a/graph/layout/eades.go +++ b/graph/layout/eades.go @@ -65,13 +65,15 @@ func (u *EadesR2) Update(g graph.Graph, layout LayoutR2) bool { } u.nodes = g.Nodes() u.indexOf = make(map[int64]int, u.nodes.Len()) - u.particles = make([]barneshut.Particle2, 0, u.nodes.Len()) - u.forces = make([]r2.Vec, u.nodes.Len()) + if u.nodes.Len() >= 0 { + u.particles = make([]barneshut.Particle2, 0, u.nodes.Len()) + } for u.nodes.Next() { id := u.nodes.Node().ID() u.indexOf[id] = len(u.particles) u.particles = append(u.particles, eadesR2Node{id: id, pos: r2.Vec{X: rnd(), Y: rnd()}}) } + u.forces = make([]r2.Vec, len(u.particles)) } u.nodes.Reset() diff --git a/graph/layout/plotter_test.go b/graph/layout/plotter_test.go index ce67fa13..b3101649 100644 --- a/graph/layout/plotter_test.go +++ b/graph/layout/plotter_test.go @@ -27,8 +27,14 @@ func (p render) Plot(c draw.Canvas, plt *plot.Plot) { if nodes.Len() == 0 { return } - xys := make(plotter.XYs, 0, nodes.Len()) - ids := make([]string, 0, nodes.Len()) + var ( + xys plotter.XYs + ids []string + ) + if nodes.Len() >= 0 { + xys = make(plotter.XYs, 0, nodes.Len()) + ids = make([]string, 0, nodes.Len()) + } for nodes.Next() { u := nodes.Node() uid := u.ID() @@ -81,7 +87,10 @@ func (p render) DataRange() (xmin, xmax, ymin, ymax float64) { if nodes.Len() == 0 { return } - xys := make(plotter.XYs, 0, nodes.Len()) + var xys plotter.XYs + if nodes.Len() >= 0 { + xys = make(plotter.XYs, 0, nodes.Len()) + } for nodes.Next() { u := nodes.Node() uid := u.ID() @@ -98,12 +107,16 @@ func (p render) GlyphBoxes(plt *plot.Plot) []plot.GlyphBox { if nodes.Len() == 0 { return nil } - b := make([]plot.GlyphBox, nodes.Len()) + var b []plot.GlyphBox + if nodes.Len() >= 0 { + b = make([]plot.GlyphBox, 0, nodes.Len()) + } for i := 0; nodes.Next(); i++ { u := nodes.Node() uid := u.ID() ur2 := p.GraphR2.LayoutNodeR2(uid) + b = append(b, plot.GlyphBox{}) b[i].X = plt.X.Norm(ur2.Coord2.X) b[i].Y = plt.Y.Norm(ur2.Coord2.Y) r := radius diff --git a/graph/nodes_edges.go b/graph/nodes_edges.go index 3d5dae1f..35b420b2 100644 --- a/graph/nodes_edges.go +++ b/graph/nodes_edges.go @@ -56,28 +56,29 @@ type NodeSlicer interface { // NodesOf returns it.Len() nodes from it. If it is a NodeSlicer, the NodeSlice method // is used to obtain the nodes. It is safe to pass a nil Nodes to NodesOf. -// -// If the Nodes has an indeterminate length, NodesOf will panic. func NodesOf(it Nodes) []Node { if it == nil { return nil } - len := it.Len() + n := it.Len() switch { - case len == 0: + case n == 0: return nil - case len < 0: - panic("graph: called NodesOf on indeterminate iterator") + case n < 0: + n = 0 } switch it := it.(type) { case NodeSlicer: return it.NodeSlice() } - n := make([]Node, 0, len) + nodes := make([]Node, 0, n) for it.Next() { - n = append(n, it.Node()) + nodes = append(nodes, it.Node()) } - return n + if len(nodes) == 0 { + return nil + } + return nodes } // Edges is an Edge iterator. @@ -101,28 +102,29 @@ type EdgeSlicer interface { // EdgesOf returns it.Len() nodes from it. If it is an EdgeSlicer, the EdgeSlice method is used // to obtain the edges. It is safe to pass a nil Edges to EdgesOf. -// -// If the Edges has an indeterminate length, EdgesOf will panic. func EdgesOf(it Edges) []Edge { if it == nil { return nil } - len := it.Len() + n := it.Len() switch { - case len == 0: + case n == 0: return nil - case len < 0: - panic("graph: called EdgesOf on indeterminate iterator") + case n < 0: + n = 0 } switch it := it.(type) { case EdgeSlicer: return it.EdgeSlice() } - e := make([]Edge, 0, len) + edges := make([]Edge, 0, n) for it.Next() { - e = append(e, it.Edge()) + edges = append(edges, it.Edge()) } - return e + if len(edges) == 0 { + return nil + } + return edges } // WeightedEdges is a WeightedEdge iterator. @@ -147,28 +149,29 @@ type WeightedEdgeSlicer interface { // WeightedEdgesOf returns it.Len() weighted edge from it. If it is a WeightedEdgeSlicer, the // WeightedEdgeSlice method is used to obtain the edges. It is safe to pass a nil WeightedEdges // to WeightedEdgesOf. -// -// If the WeightedEdges has an indeterminate length, WeightedEdgesOf will panic. func WeightedEdgesOf(it WeightedEdges) []WeightedEdge { if it == nil { return nil } - len := it.Len() + n := it.Len() switch { - case len == 0: + case n == 0: return nil - case len < 0: - panic("graph: called WeightedEdgesOf on indeterminate iterator") + case n < 0: + n = 0 } switch it := it.(type) { case WeightedEdgeSlicer: return it.WeightedEdgeSlice() } - e := make([]WeightedEdge, 0, len) + edges := make([]WeightedEdge, 0, n) for it.Next() { - e = append(e, it.WeightedEdge()) + edges = append(edges, it.WeightedEdge()) } - return e + if len(edges) == 0 { + return nil + } + return edges } // Lines is a Line iterator. @@ -192,28 +195,29 @@ type LineSlicer interface { // LinesOf returns it.Len() nodes from it. If it is a LineSlicer, the LineSlice method is used // to obtain the lines. It is safe to pass a nil Lines to LinesOf. -// -// If the Lines has an indeterminate length, LinesOf will panic. func LinesOf(it Lines) []Line { if it == nil { return nil } - len := it.Len() + n := it.Len() switch { - case len == 0: + case n == 0: return nil - case len < 0: - panic("graph: called LinesOf on indeterminate iterator") + case n < 0: + n = 0 } switch it := it.(type) { case LineSlicer: return it.LineSlice() } - l := make([]Line, 0, len) + lines := make([]Line, 0, n) for it.Next() { - l = append(l, it.Line()) + lines = append(lines, it.Line()) } - return l + if len(lines) == 0 { + return nil + } + return lines } // WeightedLines is a WeightedLine iterator. @@ -238,28 +242,29 @@ type WeightedLineSlicer interface { // WeightedLinesOf returns it.Len() weighted line from it. If it is a WeightedLineSlicer, the // WeightedLineSlice method is used to obtain the lines. It is safe to pass a nil WeightedLines // to WeightedLinesOf. -// -// If the WeightedLines has an indeterminate length, WeightedLinesOf will panic. func WeightedLinesOf(it WeightedLines) []WeightedLine { if it == nil { return nil } - len := it.Len() + n := it.Len() switch { - case len == 0: + case n == 0: return nil - case len < 0: - panic("graph: called WeightedLinesOf on indeterminate iterator") + case n < 0: + n = 0 } switch it := it.(type) { case WeightedLineSlicer: return it.WeightedLineSlice() } - l := make([]WeightedLine, 0, len) + lines := make([]WeightedLine, 0, n) for it.Next() { - l = append(l, it.WeightedLine()) + lines = append(lines, it.WeightedLine()) } - return l + if len(lines) == 0 { + return nil + } + return lines } // Empty is an empty set of nodes, edges or lines. It should be used when diff --git a/graph/path/a_star_test.go b/graph/path/a_star_test.go index a26cc4ea..f9328447 100644 --- a/graph/path/a_star_test.go +++ b/graph/path/a_star_test.go @@ -194,8 +194,9 @@ func TestExhaustiveAStar(t *testing.T) { } ps := DijkstraAllPaths(g) - for _, start := range graph.NodesOf(g.Nodes()) { - for _, goal := range graph.NodesOf(g.Nodes()) { + ends := graph.NodesOf(g.Nodes()) + for _, start := range ends { + for _, goal := range ends { pt, _ := AStar(start, goal, g, heuristic) gotPath, gotWeight := pt.To(goal.ID()) wantPath, wantWeight, _ := ps.Between(start.ID(), goal.ID()) diff --git a/graph/path/dynamic/dstarlite_test.go b/graph/path/dynamic/dstarlite_test.go index ea585c2d..fc598db9 100644 --- a/graph/path/dynamic/dstarlite_test.go +++ b/graph/path/dynamic/dstarlite_test.go @@ -401,9 +401,10 @@ var dynamicDStarLiteTests = []struct { l.Known[l.NodeAt(wallRow, wallCol).ID()] = false // Check we have a correctly modified representation. - for _, u := range graph.NodesOf(l.Nodes()) { + nodes := graph.NodesOf(l.Nodes()) + for _, u := range nodes { uid := u.ID() - for _, v := range graph.NodesOf(l.Nodes()) { + for _, v := range nodes { vid := v.ID() if l.HasEdgeBetween(uid, vid) != l.Grid.HasEdgeBetween(uid, vid) { ur, uc := l.RowCol(uid) diff --git a/graph/path/internal/testgraphs/limited_test.go b/graph/path/internal/testgraphs/limited_test.go index 187a3151..dc37730c 100644 --- a/graph/path/internal/testgraphs/limited_test.go +++ b/graph/path/internal/testgraphs/limited_test.go @@ -1167,11 +1167,12 @@ func TestLimitedVisionGrid(t *testing.T) { l.Grid.AllowDiagonal = test.diag x, y := l.XY(test.path[0].ID()) - for _, u := range graph.NodesOf(l.Nodes()) { + nodes := graph.NodesOf(l.Nodes()) + for _, u := range nodes { uid := u.ID() ux, uy := l.XY(uid) uNear := math.Hypot(x-ux, y-uy) <= test.radius - for _, v := range graph.NodesOf(l.Nodes()) { + for _, v := range nodes { vid := v.ID() vx, vy := l.XY(vid) vNear := math.Hypot(x-vx, y-vy) <= test.radius diff --git a/graph/path/johnson_apsp.go b/graph/path/johnson_apsp.go index b16fede3..9f54e051 100644 --- a/graph/path/johnson_apsp.go +++ b/graph/path/johnson_apsp.go @@ -152,6 +152,9 @@ func (it *johnsonNodeIterator) Len() int { var len int if it.nodes != nil { len = it.nodes.Len() + if len < 0 { + return len + } } if !it.qUsed { len++ diff --git a/graph/product/product_test.go b/graph/product/product_test.go index bf24cd83..81683fab 100644 --- a/graph/product/product_test.go +++ b/graph/product/product_test.go @@ -86,11 +86,11 @@ func TestCartesian(t *testing.T) { naiveCartesian(want, test.a, test.b) wantBytes, _ := dot.Marshal(want, "", "", " ") - gotEdgesLen := got.Edges().Len() - nA := test.a.Nodes().Len() - mA := test.a.Edges().Len() - nB := test.b.Nodes().Len() - mB := test.b.Edges().Len() + gotEdgesLen := len(graph.EdgesOf(got.Edges())) + nA := len(graph.NodesOf(test.a.Nodes())) + mA := len(graph.EdgesOf(test.a.Edges())) + nB := len(graph.NodesOf(test.b.Nodes())) + mB := len(graph.EdgesOf(test.b.Edges())) wantEdgesLen := mB*nA + mA*nB if gotEdgesLen != wantEdgesLen { t.Errorf("unexpected number of edges for Cartesian product of %s: got:%d want:%d", @@ -133,9 +133,9 @@ func TestTensor(t *testing.T) { naiveTensor(want, test.a, test.b) wantBytes, _ := dot.Marshal(want, "", "", " ") - gotEdgesLen := got.Edges().Len() - mA := test.a.Edges().Len() - mB := test.b.Edges().Len() + gotEdgesLen := len(graph.EdgesOf(got.Edges())) + mA := len(graph.EdgesOf(test.a.Edges())) + mB := len(graph.EdgesOf(test.b.Edges())) wantEdgesLen := 2 * mA * mB if gotEdgesLen != wantEdgesLen { t.Errorf("unexpected number of edges for Tensor product of %s: got:%d want:%d", @@ -178,11 +178,11 @@ func TestLexicographical(t *testing.T) { naiveLexicographical(want, test.a, test.b) wantBytes, _ := dot.Marshal(want, "", "", " ") - gotEdgesLen := got.Edges().Len() - nA := test.a.Nodes().Len() - mA := test.a.Edges().Len() - nB := test.b.Nodes().Len() - mB := test.b.Edges().Len() + gotEdgesLen := len(graph.EdgesOf(got.Edges())) + nA := len(graph.NodesOf(test.a.Nodes())) + mA := len(graph.EdgesOf(test.a.Edges())) + nB := len(graph.NodesOf(test.b.Nodes())) + mB := len(graph.EdgesOf(test.b.Edges())) wantEdgesLen := mB*nA + mA*nB*nB if gotEdgesLen != wantEdgesLen { t.Errorf("unexpected number of edges for Lexicographical product of %s: got:%d want:%d", @@ -225,11 +225,11 @@ func TestStrong(t *testing.T) { naiveStrong(want, test.a, test.b) wantBytes, _ := dot.Marshal(want, "", "", " ") - gotEdgesLen := got.Edges().Len() - nA := test.a.Nodes().Len() - mA := test.a.Edges().Len() - nB := test.b.Nodes().Len() - mB := test.b.Edges().Len() + gotEdgesLen := len(graph.EdgesOf(got.Edges())) + nA := len(graph.NodesOf(test.a.Nodes())) + mA := len(graph.EdgesOf(test.a.Edges())) + nB := len(graph.NodesOf(test.b.Nodes())) + mB := len(graph.EdgesOf(test.b.Edges())) wantEdgesLen := nA*mB + nB*mA + 2*mA*mB if gotEdgesLen != wantEdgesLen { t.Errorf("unexpected number of edges for Strong product of %s: got:%d want:%d", diff --git a/graph/spectral/laplacian.go b/graph/spectral/laplacian.go index 0c3bd41a..d2502724 100644 --- a/graph/spectral/laplacian.go +++ b/graph/spectral/laplacian.go @@ -84,7 +84,12 @@ func NewSymNormLaplacian(g graph.Undirected) Laplacian { panic("network: self edge in graph") } if uid < vid { - l.SetSym(indexOf[vid], j, -1/(squdeg*math.Sqrt(float64(g.From(vid).Len())))) + to := g.From(vid) + k := to.Len() + if k < 0 { + k = len(graph.NodesOf(to)) + } + l.SetSym(indexOf[vid], j, -1/(squdeg*math.Sqrt(float64(k)))) } } } diff --git a/graph/testgraph/testgraph.go b/graph/testgraph/testgraph.go index de73a3f0..a3c430f4 100644 --- a/graph/testgraph/testgraph.go +++ b/graph/testgraph/testgraph.go @@ -1370,7 +1370,7 @@ func AddNodes(t *testing.T, g NodeAdder, n int) { var addedNodes []graph.Node for i := 0; i < n; i++ { node := g.NewNode() - prev := g.Nodes().Len() + prev := len(graph.NodesOf(g.Nodes())) if g.Node(node.ID()) != nil { curr := g.Nodes().Len() if curr != prev { @@ -1380,7 +1380,7 @@ func AddNodes(t *testing.T, g NodeAdder, n int) { } g.AddNode(node) addedNodes = append(addedNodes, node) - curr := g.Nodes().Len() + curr := len(graph.NodesOf(g.Nodes())) if curr != prev+1 { t.Fatalf("AddNode failed to mutate graph: curr graph order != prev graph order+1, %d != %d", curr, prev+1) } @@ -1425,9 +1425,9 @@ func AddArbitraryNodes(t *testing.T, g NodeAdder, add graph.Nodes) { for add.Next() { node := add.Node() - prev := g.Nodes().Len() + prev := len(graph.NodesOf(g.Nodes())) g.AddNode(node) - curr := g.Nodes().Len() + curr := len(graph.NodesOf(g.Nodes())) if curr != prev+1 { t.Fatalf("AddNode failed to mutate graph: curr graph order != prev graph order+1, %d != %d", curr, prev+1) } @@ -1517,9 +1517,9 @@ func RemoveNodes(t *testing.T, g NodeRemover) { } first = false - prev := g.Nodes().Len() + prev := len(graph.NodesOf(g.Nodes())) g.RemoveNode(u.ID()) - curr := g.Nodes().Len() + curr := len(graph.NodesOf(g.Nodes())) if curr != prev-1 { t.Fatalf("RemoveNode failed to mutate graph: curr graph order != prev graph order-1, %d != %d", curr, prev-1) }