Merge pull request #226 from refactorfirst/use-ClassRelationshipDTO-in-package-cycles

Use class relationship dto in package cycles.  Feedback arc selections are consistent across repeated runs

Author: jimbethancourt

.refactorfirst/refactor-first.json

@@ -2,7 +2,6 @@
 
 import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
 import lombok.extern.slf4j.Slf4j;
 import org.hjug.feedback.SuperTypeToken;
 import org.jgrapht.Graph;
@@ -98,11 +97,49 @@ private E processStronglyConnectedComponent(Graph<V, E> graph, Set<V> scc) {
         // Run PageRank on line digraph
         Map<LineVertex<V, E>, Double> pageRankScores = computePageRank(lineDigraph);
 
-        // Find the edge (line vertex) with highest PageRank score
-        return pageRankScores.entrySet().parallelStream()
-                .max(Map.Entry.comparingByValue())
-                .map(entry -> entry.getKey().getOriginalEdge())
-                .orElse(null);
+        // Deterministically select the edge (line vertex) with the highest PageRank score
+        return selectHighestScoredEdge(pageRankScores);
+    }
+
+    /**
+     * Canonical, run-stable total order over line vertices: by the string form of the source
+     * vertex, then of the target vertex. Because the analyzed graphs never contain parallel
+     * edges, {@code (source, target)} uniquely identifies an edge, making this a total order
+     * over line vertices. Every ordering-sensitive step of the algorithm (PageRank accumulation,
+     * max-score selection, line-digraph DFS start) follows this order so that repeated runs on
+     * the same input graph — e.g. by the htmlReport and the jsonReport — produce identical
+     * feedback arc sets.
+     */
+    private Comparator<LineVertex<V, E>> lineVertexOrder() {
+        return Comparator.comparing((LineVertex<V, E> lineVertex) -> String.valueOf(lineVertex.getSource()))
+                .thenComparing(lineVertex -> String.valueOf(lineVertex.getTarget()));
+    }
+
+    /**
+     * Deterministically selects the line vertex carrying the highest PageRank score. Ties —
+     * which are common in symmetric cycles where every candidate edge scores identically — are
+     * broken by the canonical line-vertex order so the selection cannot vary between runs.
+     *
+     * @param pageRankScores PageRank score per line vertex
+     * @return the original edge of the highest-scored line vertex, or {@code null} when empty
+     */
+    private E selectHighestScoredEdge(Map<LineVertex<V, E>, Double> pageRankScores) {
+        LineVertex<V, E> best = null;
+        double bestScore = Double.NEGATIVE_INFINITY;
+        for (Map.Entry<LineVertex<V, E>, Double> entry : pageRankScores.entrySet()) {
+            double score = entry.getValue();
+            boolean tiedWinner = score == bestScore && best != null && isCanonicallyBefore(entry.getKey(), best);
+            if (best == null || score > bestScore || tiedWinner) {
+                best = entry.getKey();
+                bestScore = score;
+            }
+        }
+        return best != null ? best.getOriginalEdge() : null;
+    }
+
+    /** Returns whether the candidate precedes the incumbent in canonical source/target order. */
+    private boolean isCanonicallyBefore(LineVertex<V, E> candidate, LineVertex<V, E> incumbent) {
+        return lineVertexOrder().compare(candidate, incumbent) < 0;
     }
 
     /**
@@ -138,10 +175,13 @@ private void createLineDigraphEdges(
             Graph<V, E> graph, LineDigraph<V, E> lineDigraph, Map<E, LineVertex<V, E>> edgeToLineVertex) {
         Set<V> visited = ConcurrentHashMap.newKeySet();
 
-        // Start DFS from a random vertex if graph is not empty
+        // Start DFS from the canonically smallest vertex so the traversal — and therefore the
+        // resulting line-digraph edge set — is identical on every run
         if (!graph.vertexSet().isEmpty()) {
-            V startVertex = graph.vertexSet().iterator().next();
-            createLineDigraphEdgesDFS(graph, lineDigraph, edgeToLineVertex, startVertex, null, visited);
+            graph.vertexSet().stream()
+                    .min(Comparator.comparing(String::valueOf))
+                    .ifPresent(startVertex -> createLineDigraphEdgesDFS(
+                            graph, lineDigraph, edgeToLineVertex, startVertex, null, visited));
         }
     }
 
@@ -158,8 +198,11 @@ private void createLineDigraphEdgesDFS(
             Set<V> visited) {
         visited.add(vertex);
 
-        // Get outgoing edges from current vertex
-        Set<E> outgoingEdges = graph.outgoingEdgesOf(vertex);
+        // Iterate outgoing edges in canonical (target-vertex) order rather than the graph's
+        // internal set order, so the traversal — and therefore the line-digraph edge set and
+        // the FAS selection — cannot vary with the order edges were added to the graph
+        List<E> outgoingEdges = new ArrayList<>(graph.outgoingEdgesOf(vertex));
+        outgoingEdges.sort(Comparator.comparing(edge -> String.valueOf(graph.getEdgeTarget(edge))));
 
         for (E edge : outgoingEdges) {
             V target = graph.getEdgeTarget(edge);
@@ -191,6 +234,10 @@ private void createLineDigraphEdgesDFS(
 
     /**
      * Compute PageRank scores on the line digraph (Algorithm 4 implementation)
+     * Accumulation is sequential and iterates vertices and their outgoing neighbors in the
+     * canonical line-vertex order, keeping the floating-point sums bit-identical across runs —
+     * a parallel merge would introduce run-dependent rounding and, through it, different
+     * feedback arc selections in different report executions.
      * @param lineDigraph The line digraph
      * @return Map of line vertices to their PageRank scores
      */
@@ -200,58 +247,56 @@ private Map<LineVertex<V, E>, Double> computePageRank(LineDigraph<V, E> lineDigr
 
         if (numVertices == 0) return new HashMap<>();
 
+        // Canonical iteration order for vertices and adjacency keeps every run identical
+        List<LineVertex<V, E>> orderedVertices = new ArrayList<>(vertices);
+        orderedVertices.sort(lineVertexOrder());
+        Map<LineVertex<V, E>, List<LineVertex<V, E>>> orderedOutgoing = new HashMap<>();
+        for (LineVertex<V, E> v : orderedVertices) {
+            List<LineVertex<V, E>> outgoing = new ArrayList<>(lineDigraph.getOutgoingNeighbors(v));
+            outgoing.sort(lineVertexOrder());
+            orderedOutgoing.put(v, outgoing);
+        }
+
         // Initialize PageRank scores
-        Map<LineVertex<V, E>, Double> currentScores =
-                new ConcurrentHashMap<>(Math.max(16, (int) (numVertices / 0.75f) + 1));
+        Map<LineVertex<V, E>, Double> currentScores = new HashMap<>(Math.max(16, (int) (numVertices / 0.75f) + 1));
 
         final double initialScore = 1.0 / numVertices;
-        // No lambdas here, so nothing captures a non-final variable
-        for (LineVertex<V, E> v : vertices) {
+        for (LineVertex<V, E> v : orderedVertices) {
             currentScores.put(v, initialScore);
         }
 
         // Run PageRank iterations
         for (int iteration = 0; iteration < pageRankIterations; iteration++) {
             // Fresh map each iteration; pre-seed zeros so all vertices exist in the map
-            ConcurrentMap<LineVertex<V, E>, Double> newScores = new ConcurrentHashMap<>(currentScores.size());
+            Map<LineVertex<V, E>, Double> newScores = new HashMap<>(currentScores.size());
 
-            for (LineVertex<V, E> v : vertices) {
+            for (LineVertex<V, E> v : orderedVertices) {
                 newScores.put(v, 0.0);
             }
 
-            // Do one iteration in parallel; lambdas only see method parameters (effectively final)
-            applyOneIteration(vertices, lineDigraph, currentScores, newScores);
+            // One sequential iteration in canonical order; lambdas only see method parameters
+            for (LineVertex<V, E> v : orderedVertices) {
+                double score = currentScores.get(v);
+                List<LineVertex<V, E>> outgoing = orderedOutgoing.get(v);
+
+                if (outgoing.isEmpty()) {
+                    // Sink: keep score on itself
+                    newScores.merge(v, score, Double::sum);
+                } else {
+                    double scorePerEdge = score / outgoing.size();
+                    for (LineVertex<V, E> target : outgoing) {
+                        newScores.merge(target, scorePerEdge, Double::sum);
+                    }
+                }
+            }
 
-            // Swap for next iteration (this reassigns local variables, not captured by lambdas)
+            // Swap for next iteration
             currentScores = newScores;
         }
 
         return currentScores;
     }
 
-    private void applyOneIteration(
-            Set<LineVertex<V, E>> vertices,
-            LineDigraph<V, E> lineDigraph,
-            Map<LineVertex<V, E>, Double> currentScores,
-            ConcurrentMap<LineVertex<V, E>, Double> newScores) {
-
-        vertices.parallelStream().forEach(vertex -> {
-            double score = currentScores.get(vertex);
-            Set<LineVertex<V, E>> outgoing = lineDigraph.getOutgoingNeighbors(vertex);
-
-            if (outgoing.isEmpty()) {
-                // Sink: keep score on itself
-                newScores.merge(vertex, score, Double::sum);
-            } else {
-                double scorePerEdge = score / outgoing.size();
-                // Inner loop kept sequential: nested parallel often hurts more than it helps
-                for (LineVertex<V, E> target : outgoing) {
-                    newScores.merge(target, scorePerEdge, Double::sum);
-                }
-            }
-        });
-    }
-
     /**
      * Find strongly connected components using Kosaraju's algorithm
      */

graph-algorithms/src/main/java/org/hjug/feedback/arc/pageRank/PageRankFAS.java

@@ -0,0 +1,189 @@
+package org.hjug.feedback.arc.pageRank;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.TreeSet;
+import org.hjug.feedback.SuperTypeToken;
+import org.jgrapht.Graph;
+import org.jgrapht.alg.cycle.CycleDetector;
+import org.jgrapht.graph.DefaultDirectedGraph;
+import org.jgrapht.graph.DefaultEdge;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Regression tests pinning the run-to-run determinism of {@link PageRankFAS#computeFeedbackArcSet()}.
+ *
+ * <p>Both the htmlReport and the jsonReport invoke the same cycle-removal pipeline on the same
+ * package graph, so a non-deterministic feedback arc selection makes the two reports disagree on
+ * which package relationships (and, transitively, which class relationships) to remove. This
+ * suite reproduces the exact package-SCC shape that caused the htmlReport / jsonReport
+ * discrepancy between the {@code org.hjug.feedback} and
+ * {@code org.hjug.feedback.vertex.kernelized} packages.</p>
+ */
+class PageRankFASDeterminismTest {
+
+    private static final String FEEDBACK = "org.hjug.feedback";
+    private static final String PAGE_RANK = "org.hjug.feedback.arc.pageRank";
+    private static final String KERNELIZED = "org.hjug.feedback.vertex.kernelized";
+
+    /** Builds the package-SCC shape observed in the RefactorFirst report output. */
+    private Graph<String, DefaultEdge> createFeedbackPackageScc() {
+        Graph<String, DefaultEdge> graph = new DefaultDirectedGraph<>(DefaultEdge.class);
+        graph.addVertex(FEEDBACK);
+        graph.addVertex(PAGE_RANK);
+        graph.addVertex(KERNELIZED);
+        graph.addEdge(FEEDBACK, PAGE_RANK);
+        graph.addEdge(PAGE_RANK, FEEDBACK);
+        graph.addEdge(FEEDBACK, KERNELIZED);
+        graph.addEdge(KERNELIZED, FEEDBACK);
+        return graph;
+    }
+
+    /** Copies the graph topology with fresh edges so each run can remove edges independently. */
+    private Graph<String, DefaultEdge> copyGraph(Graph<String, DefaultEdge> original) {
+        Graph<String, DefaultEdge> copy = new DefaultDirectedGraph<>(DefaultEdge.class);
+        original.vertexSet().forEach(copy::addVertex);
+        original.edgeSet()
+                .forEach(edge ->
+                        copy.addEdge(original.getEdgeSource(edge), original.getEdgeTarget(edge), new DefaultEdge()));
+        return copy;
+    }
+
+    /** Converts edges to sorted source/target keys for comparison across graph copies. */
+    private Set<String> edgeKeys(Graph<String, DefaultEdge> graph, Set<DefaultEdge> edges) {
+        Set<String> keys = new TreeSet<>();
+        for (DefaultEdge edge : edges) {
+            keys.add(graph.getEdgeSource(edge) + " -> " + graph.getEdgeTarget(edge));
+        }
+        return keys;
+    }
+
+    /** Verifies repeated runs select the same edges and break both cycles in the package SCC. */
+    @DisplayName("computeFeedbackArcSet selects the same edges on every run for the feedback package SCC")
+    @Test
+    void computeFeedbackArcSet_isDeterministicAcrossRuns() {
+        // Given: the package SCC from the RefactorFirst report whose tied PageRank scores made
+        // htmlReport and jsonReport pick different package edges to remove
+        Graph<String, DefaultEdge> template = createFeedbackPackageScc();
+
+        // When: the same algorithm runs many times on fresh copies of the same graph
+        Set<String> firstRun = null;
+        for (int i = 0; i < 300; i++) {
+            Graph<String, DefaultEdge> graph = copyGraph(template);
+            PageRankFAS<String, DefaultEdge> pageRankFAS = new PageRankFAS<>(graph, new SuperTypeToken<>() {});
+            Set<String> run = edgeKeys(graph, pageRankFAS.computeFeedbackArcSet());
+            if (firstRun == null) {
+                firstRun = run;
+            } else {
+                assertEquals(
+                        firstRun,
+                        run,
+                        "computeFeedbackArcSet must select identical edges across runs; run " + i + " differed");
+            }
+        }
+
+        // Then: the selection breaks both 2-cycles, so the graph is acyclic after removal
+        Graph<String, DefaultEdge> graph = copyGraph(template);
+        PageRankFAS<String, DefaultEdge> pageRankFAS = new PageRankFAS<>(graph, new SuperTypeToken<>() {});
+        Set<DefaultEdge> feedbackArcSet = pageRankFAS.computeFeedbackArcSet();
+        assertEquals(2, feedbackArcSet.size(), "Both 2-cycles require exactly one removed edge each");
+        feedbackArcSet.forEach(graph::removeEdge);
+        assertTrue(
+                !new CycleDetector<>(graph).detectCycles(),
+                "Removing the feedback arc set must leave the graph acyclic");
+    }
+
+    /** Verifies repeatable feedback arc selection and cycle removal in a larger symmetric graph. */
+    @DisplayName("computeFeedbackArcSet is deterministic for a graph with parallel tie candidates")
+    @Test
+    void computeFeedbackArcSet_isDeterministicForLargerTiedGraph() {
+        // Given: a larger symmetric SCC where many edges tie on PageRank score
+        Graph<String, DefaultEdge> template = new DefaultDirectedGraph<>(DefaultEdge.class);
+        for (int i = 0; i < 6; i++) {
+            template.addVertex("P" + i);
+        }
+        for (int i = 0; i < 6; i++) {
+            template.addEdge("P" + i, "P" + (i + 1) % 6);
+            template.addEdge("P" + i, "P" + (i + 2) % 6);
+        }
+
+        // When: the algorithm runs repeatedly on fresh copies
+        Set<String> firstRun = null;
+        for (int i = 0; i < 100; i++) {
+            Graph<String, DefaultEdge> graph = copyGraph(template);
+            PageRankFAS<String, DefaultEdge> pageRankFAS = new PageRankFAS<>(graph, new SuperTypeToken<>() {});
+            Set<String> run = edgeKeys(graph, pageRankFAS.computeFeedbackArcSet());
+            if (firstRun == null) {
+                firstRun = run;
+            } else {
+                assertEquals(
+                        firstRun,
+                        run,
+                        "computeFeedbackArcSet must select identical edges across runs; run " + i + " differed");
+            }
+        }
+
+        // Then: the selection is a valid feedback arc set
+        Graph<String, DefaultEdge> graph = copyGraph(template);
+        PageRankFAS<String, DefaultEdge> pageRankFAS = new PageRankFAS<>(graph, new SuperTypeToken<>() {});
+        Set<DefaultEdge> feedbackArcSet = pageRankFAS.computeFeedbackArcSet();
+        Set<DefaultEdge> removed = new HashSet<>(feedbackArcSet);
+        removed.forEach(graph::removeEdge);
+        assertTrue(
+                !new CycleDetector<>(graph).detectCycles(),
+                "Removing the feedback arc set must leave the graph acyclic");
+    }
+
+    /** Verifies edge insertion order does not change feedback arc selection for overlapping cycles. */
+    @DisplayName("computeFeedbackArcSet is independent of the order edges were added to the graph")
+    @Test
+    void computeFeedbackArcSet_isDeterministicAcrossEdgeInsertionOrders() {
+        // Given: one logical SCC expressing multiple overlapping cycles
+        String[][] logicalEdges = {
+            {"A", "B"},
+            {"B", "C"},
+            {"C", "A"}, // triangle 1
+            {"A", "D"},
+            {"D", "E"},
+            {"E", "A"}, // triangle 2
+            {"B", "E"},
+            {"D", "B"}, // cross edges
+        };
+
+        // And: several different edge insertion orders of the same logical edges
+        int[][] permutations = {
+            {0, 1, 2, 3, 4, 5, 6, 7}, // as declared
+            {7, 6, 5, 4, 3, 2, 1, 0}, // reversed
+            {3, 0, 6, 2, 5, 1, 7, 4}, // interleaved
+            {5, 2, 7, 1, 4, 6, 0, 3}, // another interleave
+        };
+
+        // When: the feedback arc set is computed for each insertion order
+        Set<String> firstOrder = null;
+        for (int[] permutation : permutations) {
+            Graph<String, DefaultEdge> graph = new DefaultDirectedGraph<>(DefaultEdge.class);
+            for (int edgeIndex : permutation) {
+                String[] logicalEdge = logicalEdges[edgeIndex];
+                graph.addVertex(logicalEdge[0]);
+                graph.addVertex(logicalEdge[1]);
+                graph.addEdge(logicalEdge[0], logicalEdge[1]);
+            }
+            PageRankFAS<String, DefaultEdge> pageRankFAS = new PageRankFAS<>(graph, new SuperTypeToken<>() {});
+            Set<String> fas = edgeKeys(graph, pageRankFAS.computeFeedbackArcSet());
+
+            // Then: every insertion order selects the identical feedback arc set
+            if (firstOrder == null) {
+                firstOrder = fas;
+            } else {
+                assertEquals(
+                        firstOrder,
+                        fas,
+                        "computeFeedbackArcSet must not depend on the order edges were added to the graph");
+            }
+        }
+    }
+}