The lca package #597

AlexandruValeanu · 2018-06-12T22:01:53Z

I have finished the lca package. The jgrapth-dev topic relevant to this PR is here.

Add small comments to explain harder parts
Write better documentation for all implementations

This should close #453.

…o lca

# Conflicts: # jgrapht-core/src/main/java/org/jgrapht/GraphTests.java # jgrapht-core/src/main/java/org/jgrapht/generate/BarabasiAlbertForestGenerator.java # jgrapht-core/src/test/java/org/jgrapht/generate/BarabasiAlbertForestGeneratorTest.java

# Conflicts: # jgrapht-core/src/main/java/org/jgrapht/alg/decomposition/HeavyPathDecomposition.java # jgrapht-core/src/main/java/org/jgrapht/alg/decomposition/package-info.java # jgrapht-core/src/test/java/org/jgrapht/alg/decomposition/HeavyPathDecompositionTest.java

d-michail

Nice contribution. A few mostly stylistic and documentation comments.

jgrapht-core/src/main/java/org/jgrapht/alg/NaiveLcaFinder.java

 */
-public class NaiveLcaFinder<V, E>
+@Deprecated public class NaiveLcaFinder<V, E>


jgrapht-core/src/main/java/org/jgrapht/alg/decomposition/HeavyPathDecomposition.java

+         * @return internal parent array
+         */
+        public int[] getParentArray(){
+            return parent.clone();


jgrapht-core/src/main/java/org/jgrapht/alg/lca/NaiveLCAFinder.java

+    @SuppressWarnings("unchecked")
+    public Set<V> getAllLCAs(V a, V b)
+    {
+        Set<V>[] visitedSets = new Set[2];


jgrapht-core/src/main/java/org/jgrapht/alg/lca/NaiveLCAFinder.java

+     * has been visited from both a and b, it is no longer expanded in our search (we know that its
+     * ancestors won't be part of the SLCA(x, y) set).
+     */
+    @SuppressWarnings("unchecked")


jgrapht-core/src/main/java/org/jgrapht/alg/lca/TarjanLCAFinder.java

+ * </p>
+ *
+ * Preprocessing Time complexity: $O(1)$
+ * Preprocessing Memory complexity:  $O(1)$


jgrapht-core/src/main/java/org/jgrapht/alg/lca/EulerTourRMQLCAFinder.java

+public class EulerTourRMQLCAFinder<V, E> implements LCAAlgorithm<V> {
+    private final Graph<V, E> graph;
+    private final Set<V> roots;
+    private final int MAX_LEVEL;


d-michail · 2018-08-02T10:43:42Z

jgrapht-core/src/main/java/org/jgrapht/alg/lca/EulerTourRMQLCAFinder.java

+        }
+    }
+
+    private void dfs(int u, int parent, int lvl) {


Could you convert this to iterative for better scalability?

jgrapht-core/src/main/java/org/jgrapht/alg/lca/EulerTourRMQLCAFinder.java

+        for (int i = 0; i < sizeTour; i++)
+            rmq[0][i] = i;
+
+        for (int i = 1; (1 << i) <= sizeTour; i++)


jgrapht-core/src/main/java/org/jgrapht/alg/lca/HeavyPathLCAFinder.java

+ * Algorithm for computing lowest common ancestors in rooted trees and forests based on {@link HeavyPathDecomposition}.
+ *
+ * Preprocessing Time complexity: $O(|V|)$
+ * Preprocessing Memory complexity: $O(|V|)$


d-michail · 2018-08-02T10:46:33Z

jgrapht-core/src/main/java/org/jgrapht/alg/lca/HeavyPathLCAFinder.java

+
+/**
+ * Algorithm for computing lowest common ancestors in rooted trees and forests based on {@link HeavyPathDecomposition}.
+ *


Add some reference to appropriate paper which introduced the technique. If this is not explicit to a particular paper, add a reference to the dynamic trees paper of Sleator and Tarjan which made all these possible.

I've added on in the HeavyPathDecomposition. Thank you for the remainder.

jkinable

Can you post a link to the place where you put the performance tests results? Can't seem to find it in my mailbox.

This PR introduces a variety of ways to compute LCAs.
Is there a clear winner? Is there a clear looser? Are there specific trade-offs the user should consider when using a specific LCA algorithm? Are there certain types of trees/forests which are solved better by a specific LCA implementation? Your typical LCA user might just want the "best" algorithm, without wasting time on performing a performance comparison.
I would add a paragraph of javadoc to each of the LCAs where you discuss the above questions, and crossreference to other LCAs to make it as simple as possible for the user to select the best LCA algorithm for his application.

Are there LCA algorithms or related problems that are not included in this PR but which we might want to consider including somewhere in the future?

jkinable · 2018-08-03T04:56:30Z

jgrapht-core/src/main/java/org/jgrapht/alg/lca/BinaryLiftingLCAFinder.java

+import static org.jgrapht.util.MathUtil.log2;
+
+/**
+ * Algorithm for computing lowest common ancestors in rooted trees and forests using the binary lifting method.


Can you add a reference for this technique?

I am not sure a formal reference (i.e. academic paper) exists. I've added a link to the original article on Topcoder. That's the best I can think of.

It actually looks like a "brute-force" approach, using the lookup table.

This reference might be related, but it needs further investigation.

Bender, Michael A., and Martın Farach-Colton. "The level ancestor problem simplified." Theoretical Computer Science 321.1 (2004): 5-12.

It is indeed a technique from that paper (or some older one). See Demaine's notes page 7, Algorithm B.

"The basic idea is to use jump pointers. These are pointers at a node which reference one of the node’s ancestors. For each node create jump pointers to ancestors at levels 1, 2, 4, . . . , 2k. Queries are answered by repeatedly jumping from node to node, each time jumping more than half of the remaining levels between the current ancestor and goal ancestor. So worst-case number of jumps is O(log n). Preprocessing is done by filling in jump pointers using dynamic programming."

No one is actually using the term "binary lifting". I think this was coined up by people in topcoder or some other programming competition.

While keeping the name is fine, I think the Javadocs need a reference to an academic paper using this technique. Moreover, they need at least a paragraph explaining the lookup table with the jump pointers, and how you answer the LCA query in O(logn) time.

Do you think we need more than the current explanation?

No, it looks fine. @jkinable ?

I think it's fine if we add the reference (+quoted text) pointed out by @d-michail

Isn't it already added? See

jgrapht/jgrapht-core/src/main/java/org/jgrapht/alg/lca/BinaryLiftingLCAFinder.java

Line 33 in 7ca0aeb

* The method appears in <i>Bender, Michael A., and Martın Farach-Colton. "The level ancestor problem

jgrapht-core/src/main/java/org/jgrapht/alg/lca/NaiveLCAFinder.java

+ * and b. Of course we may have to wait longer if the path to a is of length n, but the path to
+ * b&gt;n. at the first loop we have a path of 0 length from the nodes we are considering as LCA to
+ * their respective children which we wish to find the LCA for.
+ * 


jkinable · 2018-08-15T03:25:18Z

jgrapht-core/src/main/java/org/jgrapht/alg/lca/TarjanLCAFinder.java

+ *
+ * <p>
+ * See the article on
+ * <a href="https://en.wikipedia.org/wiki/Tarjan%27s_off-line_lowest_common_ancestors_algorithm">wikipedia</a> for more


This comment was marked as resolved but I dont see any changes?

There are. See

jgrapht/jgrapht-core/src/main/java/org/jgrapht/alg/lca/TarjanLCAFinder.java

Line 39 in 7ca0aeb

* The original algorithm can be found in <i>Gabow, H. N.; Tarjan, R. E. (1983),

Am I the only one that sees them? :)

AlexandruValeanu · 2018-08-04T18:47:09Z

@jkinable

Can you post a link to the place where you put the performance tests results? Can't seem to find it in my mailbox.

You can get some performance results by running perf.lca.LowestCommonAncestorAlgorithmPerformanceTest

This PR introduces a variety of ways to compute LCAs.
Is there a clear winner? Is there a clear looser? Are there specific trade-offs the user should consider when using a specific LCA algorithm? Are there certain types of trees/forests which are solved better by a specific LCA implementation? Your typical LCA user might just want the "best" algorithm, without wasting time on performing a performance comparison.
I would add a paragraph of javadoc to each of the LCAs where you discuss the above questions, and crossreference to other LCAs to make it as simple as possible for the user to select the best LCA algorithm for his application.

Tarjan's algorithm appears to be a clear looser but it uses the smaller amount of extra memory to perform a query and it doesn't need to preprocess the graph. The other three implementations are quite close and I guess it will depend on the input to see which is better. There doesn't appear to be a clear winner. I'll add some notes to each doc to try to guide the user.

Are there LCA algorithms or related problems that are not included in this PR but which we might want to consider including somewhere in the future?

I do not think there exist popular algorithms (for computing LCAs in trees) that are not included in this package. The RMQ part in EulerTourRMQLCAFinder can be done in linear time but it is more complicated and I am not sure how well it behaves in practice. Another LCA-related problem will be to compute LCAs in dynamic trees (trees where you can add/remove a vertex). Tarjan and Sleator have introduced link-cut trees that support these operations.

jsichi · 2018-08-07T20:29:12Z

jgrapht-core/src/main/java/org/jgrapht/alg/interfaces/LCAAlgorithm.java

+ *
+ * @author Alexandru Valeanu
+ */
+public interface LCAAlgorithm<V> {


For the main interface name, let's spell it out as LowestCommonAncestorAlgorithm. Elsewhere (e.g. in the method names and in the implementation classes) we can keep the acronym.

jsichi · 2018-08-07T20:37:25Z

jgrapht-core/src/main/java/org/jgrapht/alg/lca/HeavyPathLCAFinder.java

+            return null;
+
+        /*
+         * Idea: Get a anb b on the same vertex path by 'jumping' from one path to another


jsichi · 2018-08-07T20:39:43Z

jgrapht-core/src/main/java/org/jgrapht/alg/interfaces/LCAAlgorithm.java

+     * @return the LCA of a and b, or null if there is no LCA.
+     */
+    V getLCA(V a, V b);
+


Shouldn't we expose getAllLCAs at the interface level as well? Algorithms are free to throw UnsupportedOperationException if they don't support it.

I don't think so. That method is only added to classes that can implement the function more efficiently than the default one (only Tarjan so far) and to make it easier for the user to get LCAs for a batch of queries.

I'm referring to the getAllLCAs in NaiveLCAFinder, which finds all the lowest common ancestors (in the case where there are multiple) instead of just returning one chosen arbitrarily.

Sorry, I completely misread it. We can expose it and make all implementations except NaiveLCAFinder return the result of getLCA. What do you think?

That works, but then it shouldn't have "all" in the name since it's not guaranteed to return a comprehensive set (and as with isomorphism inspectors, each alg doc should specify its own behavior). How about

getLCA(V, V) : returns one
getLCASet(V, V) : returns many
getBatchLCA(List): batch version, each query returns one
getBatchLCASet(List): batch version, each query returns many

I agree. I assume that the batch version is implemented in terms of the non-batch one. And in this case, getLCASet is not supported for all tree-based implementations.

jsichi · 2018-08-13T05:15:16Z

jgrapht-core/src/main/java/org/jgrapht/alg/lca/BinaryLiftingLCAFinder.java

+    private int[] component;
+
+    /**
+     * Construct a new instance of the algorithm..<br>


Why two dots? Also, use p instead of br.

I have no idea where the two dots come from. Will change to <p>

jgrapht-core/src/main/java/org/jgrapht/alg/lca/BinaryLiftingLCAFinder.java

+        }
+
+        V vertexU = indexList.get(u);
+        for (E edge: graph.edgesOf(vertexU)){


jgrapht-core/src/main/java/org/jgrapht/alg/lca/EulerTourRMQLCAFinder.java

+    private int[] representative;
+
+    private int[][] rmq;
+    private int[] log2;


jsichi · 2018-08-13T05:31:42Z

jgrapht-core/src/test/java/org/jgrapht/GraphTestsTest.java

@@ -555,34 +556,6 @@
        Graphs.addEdgeWithVertices(graph, 1, 3);
        assertTrue(GraphTests.isWeaklyChordal(graph));
    }
-


Why are these tests being eliminated?

They are not. My editor auto-converted @Test public void testIsWeaklyChordal() to

@Test public void testIsWeaklyChordal()

I'm referring to failRequireIsWeightedOnUnweightedGraph and failRequireIsWeightedOnNull

I have no idea what happened. I've put the three tests back.

jsichi · 2018-08-13T05:34:07Z

jgrapht-core/src/test/java/org/jgrapht/alg/lca/LCATestBase.java

+ *
+ * @author Alexandru Valeanu
+ */
+public abstract class LCATestBase {


Since this class only provides non-DAG cases, it should be named LCATreeTestBase

jsichi · 2018-08-13T05:36:58Z

jgrapht-core/src/test/java/org/jgrapht/alg/lca/NaiveLCAFinderTest.java

+ * @author Barak Naveh
+ * @author Alexandru Valeanu
+ */
+public class NaiveLCAFinderTest {


Then it's probably worth splitting to distinguish the small cases from the large ones; either that or use an explicit suite to select them.

jgrapht-core/src/test/java/org/jgrapht/alg/lca/LCATestBase.java

+        Assert.assertEquals("b", createSolver(g, Collections.singleton("b")).getLCA("h", "b"));
+    }
+
+    // TODO: keep?


jgrapht-core/src/main/java/org/jgrapht/alg/lca/EulerTourRMQLCAFinder.java

+    }
+
+    private void dfsIterative(int u, int startLevel){
+        // set of vertices for which the the part of the if has been performed


jkinable · 2018-08-15T03:35:10Z

~~@AlexandruValeanu can you add reference given by @d-michail to BinaryLiftingLCAFinder.java? Also, for TarjanLCAFinder: do you have a better reference than the wiki url?~~

@d-michail if you and John's comments have been addressed, can you go ahead and merge this one?

AlexandruValeanu · 2018-08-15T18:47:15Z

@jkinable I have added both references (see

jgrapht/jgrapht-core/src/main/java/org/jgrapht/alg/lca/BinaryLiftingLCAFinder.java

Line 33 in 7ca0aeb

    
            * The method appears in <i>Bender, Michael A., and Martın Farach-Colton. "The level ancestor problem

for Binary... and

jgrapht/jgrapht-core/src/main/java/org/jgrapht/alg/lca/TarjanLCAFinder.java

Line 39 in 7ca0aeb

    
            *     The original algorithm can be found in <i>Gabow, H. N.; Tarjan, R. E. (1983),

for Tarjan...)

d-michail · 2018-08-18T10:45:17Z

Thanks!

AlexandruValeanu added 21 commits May 23, 2018 21:55

Add interface

1a449c3

Add interface

4fe4eaf

Update old implementations

9e26b03

Add EulerTourRMQLCAFinder and tests for all implementations

edf9900

Fix bug with disconnected graphs.

091dc67

Add more tests

176e867

Fix Java related issues

c6b36be

Add support for multiple roots

3b842e9

Add Heavy Path Decomposition

88b67ed

Add forest generator

2336275

Add performance tests

861b239

Small fixes

b3b3eb0

docs + opt-heavy-path

b1765b0

Update docs

da2e4ab

Update heavy-path tests

0586b4d

Refactor heavy-path decomposition

6c66f66

Fix checkstyle errors

1dda979

Fix checkstyle errors

3b91111

Merge branch 'lca' of https://github.com/AlexandruValeanu/jgrapht int…

e4b14a0

…o lca

Fix checkstyle errors

21aabb1

Update docs

fefe40e

AlexandruValeanu closed this Jun 19, 2018

AlexandruValeanu added 8 commits June 21, 2018 20:37

Merge remote-tracking branch 'remotes/upstream/master' into lca

756c6d2

Update docs

3a4295f

Fix imports

3c36ba6

Fix imports

1fabf8b

Update docs + tests

72d4292

Fix doc

b00ec07

AlexandruValeanu added 2 commits July 18, 2018 21:20

Add docs and reorganize lca package

c238f0e

AlexandruValeanu reopened this Jul 21, 2018

AlexandruValeanu changed the title ~~The lca package + Heavy-path decomposition~~ The lca package Jul 21, 2018

d-michail self-requested a review July 24, 2018 10:33

d-michail reviewed Aug 2, 2018

View reviewed changes

jkinable reviewed Aug 3, 2018

View reviewed changes

AlexandruValeanu added 4 commits August 4, 2018 11:57

Merge branch 'master' of https://github.com/jgrapht/jgrapht into lca

06d917f

Refactor lca package

50002cd

Fix broken javadoc

736f4cf

Update documentation of BinaryLifting

71d4dc2

jsichi reviewed Aug 7, 2018

View reviewed changes

AlexandruValeanu added 2 commits August 7, 2018 22:14

Rename interface to LowestCommonAncestorAlgorithm

61cb2a4

Update method names

751066e

jsichi reviewed Aug 13, 2018

View reviewed changes

jgrapht-core/src/main/java/org/jgrapht/alg/lca/EulerTourRMQLCAFinder.java Outdated

}

private void dfsIterative(int u, int startLevel){

// set of vertices for which the the part of the if has been performed

This comment was marked as resolved.

Sign in to view

AlexandruValeanu added 2 commits August 13, 2018 23:03

Small fixes + documentation update

8a89b81

Restore vanished tests

7ca0aeb

d-michail merged commit 048bf34 into jgrapht:master Aug 18, 2018

AlexandruValeanu deleted the lca branch September 2, 2018 08:19

syoon2 mentioned this pull request Apr 12, 2024

Add Missing License Headers #1215

Open

7 tasks

The lca package #597

The lca package #597

Conversation

AlexandruValeanu commented Jun 12, 2018 • edited

d-michail left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

AlexandruValeanu Aug 4, 2018 • edited

Choose a reason for hiding this comment

jkinable left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandruValeanu commented Aug 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandruValeanu Aug 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandruValeanu Aug 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandruValeanu Aug 13, 2018 • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

AlexandruValeanu Aug 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

jkinable commented Aug 15, 2018 • edited

AlexandruValeanu commented Aug 15, 2018 • edited

d-michail commented Aug 18, 2018

AlexandruValeanu commented Jun 12, 2018 •

edited

AlexandruValeanu Aug 4, 2018 •

edited

jkinable left a comment •

edited

AlexandruValeanu Aug 7, 2018 •

edited

AlexandruValeanu Aug 7, 2018 •

edited

AlexandruValeanu Aug 13, 2018 •

edited

AlexandruValeanu Aug 13, 2018 •

edited

jkinable commented Aug 15, 2018 •

edited

AlexandruValeanu commented Aug 15, 2018 •

edited