diff --git a/.changeset/fix-graph-undirected-traversal.md b/.changeset/fix-graph-undirected-traversal.md new file mode 100644 index 00000000000..e80e04680b7 --- /dev/null +++ b/.changeset/fix-graph-undirected-traversal.md @@ -0,0 +1,5 @@ +--- +"effect": patch +--- + +Fix Graph traversal and shortest-path algorithms to traverse undirected edges independently of their stored source/target orientation. diff --git a/packages/effect/src/Graph.ts b/packages/effect/src/Graph.ts index 77f6ec5e2ba..0cc47d94bce 100644 --- a/packages/effect/src/Graph.ts +++ b/packages/effect/src/Graph.ts @@ -1894,6 +1894,37 @@ export const isAcyclic = ( return graph.isAcyclic.value } + if (graph.type === "undirected") { + const visited = new Set() + + for (const startNode of graph.nodes.keys()) { + if (visited.has(startNode)) { + continue + } + + visited.add(startNode) + const stack: Array<{ node: NodeIndex; parent: NodeIndex | null }> = [{ node: startNode, parent: null }] + + while (stack.length > 0) { + const { node, parent } = stack.pop()! + const nodeNeighbors = getUndirectedNeighbors(graph as any, node) + + for (const neighbor of nodeNeighbors) { + if (!visited.has(neighbor)) { + visited.add(neighbor) + stack.push({ node: neighbor, parent: node }) + } else if (neighbor !== parent) { + graph.isAcyclic = Option.some(false) + return false + } + } + } + } + + graph.isAcyclic = Option.some(true) + return true + } + // Stack-safe DFS cycle detection using iterative approach const visited = new Set() const recursionStack = new Set() @@ -2074,6 +2105,21 @@ const getUndirectedNeighbors = ( return Array.from(neighbors) } +const getTraversalNeighbors = ( + graph: Graph | MutableGraph, + nodeIndex: NodeIndex, + direction: Direction +): Array => + graph.type === "undirected" + ? getUndirectedNeighbors(graph as any, nodeIndex) + : neighborsDirected(graph, nodeIndex, direction) + +const getTraversableNeighbor = ( + graph: Graph | MutableGraph, + current: NodeIndex, + edge: Edge +): NodeIndex => graph.type === "undirected" && edge.target === current ? edge.source : edge.target + /** * Find connected components in an undirected graph. * Each component is represented as an array of node indices. @@ -2403,7 +2449,7 @@ export const dijkstra = ( for (const edgeIndex of adjacencyList) { const edge = graph.edges.get(edgeIndex) if (edge !== undefined) { - const neighbor = edge.target + const neighbor = getTraversableNeighbor(graph, currentNode, edge) const weight = cost(edge.data) // Validate non-negative weights @@ -2535,6 +2581,15 @@ export const floydWarshall = ( next.get(i)!.set(j, j) edgeMatrix.get(i)!.set(j, edgeData.data) } + + if (graph.type === "undirected") { + const reverseWeight = dist.get(j)!.get(i)! + if (weight < reverseWeight) { + dist.get(j)!.set(i, weight) + next.get(j)!.set(i, i) + edgeMatrix.get(j)!.set(i, edgeData.data) + } + } } // Floyd-Warshall main loop @@ -2728,7 +2783,7 @@ export const astar = ( for (const edgeIndex of adjacencyList) { const edge = graph.edges.get(edgeIndex) if (edge !== undefined) { - const neighbor = edge.target + const neighbor = getTraversableNeighbor(graph, currentNode, edge) const weight = cost(edge.data) // Validate non-negative weights @@ -2864,6 +2919,14 @@ export const bellmanFord = ( weight, edgeData: edgeData.data }) + if (graph.type === "undirected" && edgeData.source !== edgeData.target) { + edges.push({ + source: edgeData.target, + target: edgeData.source, + weight, + edgeData: edgeData.data + }) + } } // Relax edges up to V-1 times @@ -2905,14 +2968,8 @@ export const bellmanFord = ( affectedNodes.add(node) // Add all nodes reachable from this node - const adjacencyList = graph.adjacency.get(node) - if (adjacencyList !== undefined) { - for (const edgeIndex of adjacencyList) { - const edge = graph.edges.get(edgeIndex) - if (edge !== undefined) { - queue.push(edge.target) - } - } + for (const neighbor of getTraversalNeighbors(graph, node, "outgoing")) { + queue.push(neighbor) } } @@ -3230,7 +3287,7 @@ export const dfs = ( continue } - const neighbors = neighborsDirected(graph, current, direction) + const neighbors = getTraversalNeighbors(graph, current, direction) for (let i = neighbors.length - 1; i >= 0; i--) { const neighbor = neighbors[i] if (!discovered.has(neighbor)) { @@ -3307,7 +3364,7 @@ export const bfs = ( if (!discovered.has(current)) { discovered.add(current) - const neighbors = neighborsDirected(graph, current, direction) + const neighbors = getTraversalNeighbors(graph, current, direction) for (const neighbor of neighbors) { if (!discovered.has(neighbor)) { queue.push(neighbor) @@ -3389,6 +3446,10 @@ export const topo = ( graph: Graph | MutableGraph, config: TopoConfig = {} ): NodeWalker => { + if (graph.type === "undirected") { + throw new Error("Cannot perform topological sort on undirected graph") + } + // Check if graph is acyclic first if (!isAcyclic(graph)) { throw new Error("Cannot perform topological sort on cyclic graph") @@ -3533,7 +3594,7 @@ export const dfsPostOrder = ( if (!current.visitedChildren) { current.visitedChildren = true - const neighbors = neighborsDirected(graph, current.node, direction) + const neighbors = getTraversalNeighbors(graph, current.node, direction) for (let i = neighbors.length - 1; i >= 0; i--) { const neighbor = neighbors[i] diff --git a/packages/effect/test/Graph.test.ts b/packages/effect/test/Graph.test.ts index 42d03a2d6e0..5cb9d252ffa 100644 --- a/packages/effect/test/Graph.test.ts +++ b/packages/effect/test/Graph.test.ts @@ -1,6 +1,22 @@ import { describe, expect, it } from "@effect/vitest" import { Equal, Graph, Hash, Option } from "effect" +const makeReversedUndirectedPath = () => + Graph.undirected((mutable) => { + const a = Graph.addNode(mutable, "A") + const b = Graph.addNode(mutable, "B") + const c = Graph.addNode(mutable, "C") + Graph.addEdge(mutable, a, b, 1) + Graph.addEdge(mutable, c, b, 1) + }) + +const expectSomePath = (result: Option.Option>, expected: Graph.PathResult) => { + expect(Option.isSome(result)).toBe(true) + if (Option.isSome(result)) { + expect(result.value).toEqual(expected) + } +} + describe("Graph", () => { describe("constructors", () => { it("should create empty directed graph", () => { @@ -2133,6 +2149,25 @@ describe("Graph", () => { expect(Graph.isAcyclic(mixedComponents)).toBe(false) }) + + it("should treat a reversed-storage undirected chain as acyclic", () => { + const graph = makeReversedUndirectedPath() + + expect(Graph.isAcyclic(graph)).toBe(true) + }) + + it("should detect cycles in undirected graphs", () => { + const graph = Graph.undirected((mutable) => { + const a = Graph.addNode(mutable, "A") + const b = Graph.addNode(mutable, "B") + const c = Graph.addNode(mutable, "C") + Graph.addEdge(mutable, a, b, 1) + Graph.addEdge(mutable, b, c, 1) + Graph.addEdge(mutable, c, a, 1) + }) + + expect(Graph.isAcyclic(graph)).toBe(false) + }) }) describe("isBipartite", () => { @@ -2410,6 +2445,18 @@ describe("Graph", () => { "Node 0 does not exist" ) }) + + it("should traverse undirected edges in reverse storage direction", () => { + const graph = makeReversedUndirectedPath() + + const result = Graph.dijkstra(graph, { + source: 0, + target: 2, + cost: (edge) => edge + }) + + expectSomePath(result, { path: [0, 1, 2], distance: 2, costs: [1, 1] }) + }) }) describe("astar", () => { @@ -2485,6 +2532,19 @@ describe("Graph", () => { "A* algorithm requires non-negative edge weights" ) }) + + it("should traverse undirected edges in reverse storage direction", () => { + const graph = makeReversedUndirectedPath() + + const result = Graph.astar(graph, { + source: 0, + target: 2, + cost: (edge) => edge, + heuristic: () => 0 + }) + + expectSomePath(result, { path: [0, 1, 2], distance: 2, costs: [1, 1] }) + }) }) describe("bellmanFord", () => { @@ -2547,6 +2607,34 @@ describe("Graph", () => { const result = Graph.bellmanFord(graph, { source: 0, target: 2, cost: (edge) => edge }) expect(Option.isNone(result)).toBe(true) }) + + it("should traverse undirected edges in reverse storage direction", () => { + const graph = makeReversedUndirectedPath() + + const result = Graph.bellmanFord(graph, { + source: 0, + target: 2, + cost: (edge) => edge + }) + + expectSomePath(result, { path: [0, 1, 2], distance: 2, costs: [1, 1] }) + }) + + it("should treat a reachable negative undirected edge as a negative cycle", () => { + const graph = Graph.undirected((mutable) => { + const a = Graph.addNode(mutable, "A") + const b = Graph.addNode(mutable, "B") + Graph.addEdge(mutable, a, b, -1) + }) + + const result = Graph.bellmanFord(graph, { + source: 0, + target: 1, + cost: (edge) => edge + }) + + expect(Option.isNone(result)).toBe(true) + }) }) describe("floydWarshall", () => { @@ -2615,6 +2703,26 @@ describe("Graph", () => { expect(() => Graph.floydWarshall(graph, (edge) => edge)).toThrow("Negative cycle detected") }) + + it("should traverse undirected edges in reverse storage direction", () => { + const graph = makeReversedUndirectedPath() + + const result = Graph.floydWarshall(graph, (edge) => edge) + + expect(result.distances.get(0)?.get(2)).toBe(2) + expect(result.paths.get(0)?.get(2)).toEqual([0, 1, 2]) + expect(result.costs.get(0)?.get(2)).toEqual([1, 1]) + }) + + it("should treat negative undirected edges as negative cycles", () => { + const graph = Graph.undirected((mutable) => { + const a = Graph.addNode(mutable, "A") + const b = Graph.addNode(mutable, "B") + Graph.addEdge(mutable, a, b, -1) + }) + + expect(() => Graph.floydWarshall(graph, (edge) => edge)).toThrow("Negative cycle detected") + }) }) describe("Iterator Base Methods", () => { @@ -2719,6 +2827,12 @@ describe("Graph", () => { expect(() => Graph.topo(cyclicGraph)).toThrow("Cannot perform topological sort on cyclic graph") }) + it("should throw for undirected graphs", () => { + const graph = makeReversedUndirectedPath() + + expect(() => Graph.topo(graph)).toThrow("Cannot perform topological sort on undirected graph") + }) + it("should handle corrupted graph state during topological sort", () => { const graph = Graph.directed((mutable) => { const a = Graph.addNode(mutable, "A") @@ -2780,6 +2894,15 @@ describe("Graph", () => { expect(entries).toEqual([[2, "C"], [1, "B"], [0, "A"]]) // Postorder: children before parents }) + + it("should traverse undirected edges in reverse storage direction", () => { + const graph = makeReversedUndirectedPath() + + expect(Array.from(Graph.indices(Graph.dfs(graph, { start: [0] })))).toEqual([0, 1, 2]) + expect(Array.from(Graph.indices(Graph.dfs(graph, { start: [0], direction: "incoming" })))).toEqual([0, 1, 2]) + expect(Array.from(Graph.indices(Graph.bfs(graph, { start: [0] })))).toEqual([0, 1, 2]) + expect(Array.from(Graph.indices(Graph.dfsPostOrder(graph, { start: [0] })))).toEqual([2, 1, 0]) + }) }) describe("DfsPostOrder Iterator", () => {