Boyer-Myrvold planarity testing algorithm #795

Toptachamann · 2019-06-16T13:30:03Z

Finally, implemented a linear time planarity testing and embedding algorithm with Kuratowski subdivision extraction option #730. The implemented algorithm is an iterative version of the Boyer-Myrvold algorithm. The implementation is tested on a large number of graph instances (10^5 triangulations and 10^5 random graphs). Here is a running time comparison with the left-right planarity testing algorithm:

From the plot above it is clear that the Boyer-Myrvold algorithm is slightly faster than the left-right algorithm, but it has a more complicated logic. In the case of the left-right algorithm, the testing phase consists of 2 dfs traversals, embedding phase consists of 1 more dfs traversal. The dfs traversals in the left-right algorithm were also made iterative.

A couple of questions:

In the algorithm I needed a list which gives references of this list nodes to the users. This works similar to the heaps in the jheaps project. Currently, this class is located in the planar package. Should I move somewhere?
The interface PlanarityTestingAlgorithm has 3 methods for testing whether a graph is a Kuratowski subdivision. Should I move these methods to the GraphTests class?

I read and understood https://github.com/jgrapht/jgrapht/wiki/Become-a-Contributor
I read and understood https://github.com/jgrapht/jgrapht/wiki/How-to-make-your-first-%28code%29-contribution
I added unit tests
I added documentation
I followed the Coding and Style Conventions
I have not modified HISTORY.md or CONTRIBUTORS.md
I ensured that the git commit message is a good one

jsichi · 2019-06-17T05:01:37Z

Regarding the list implementation, I suggest moving it to org.jgrapht.util, and name it DoublyLinkedList. As a followup pull request after this one gets merged, use it to replace other instances where something similar appears (WarnsdorffRuleKnightTourHeuristic, and your BlossomV if applicable).

Regarding the Kuratowski subdivision checking methods, yes, they can go in GraphTests.

d-michail

Great job! Some minor comments about typos and function names.

d-michail · 2019-06-17T07:20:33Z

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

+
+    /**
+     * Computes combinatorial embedding of the input {@code graph}. This method will return
+     * a valid result only if the {@code graph} is planar. For more information on the combinarotial


d-michail · 2019-06-17T07:22:19Z

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

+
+    /**
+     * Uses BFS to find all vertices of the {@code graph} which have a degree {@code degree}.
+     * This method doesn't advance to new nodes after the it finds a node with a degree {@code degree}


d-michail · 2019-06-17T07:26:26Z

jgrapht-core/src/main/java/org/jgrapht/alg/planar/ElementList.java

+ * @param <E> the list element type
+ * @author Timofey Chudakov
+ */
+public class ElementList<E> implements Iterable<E> {


If we are going to keep this, I suggest to add a modCount just like LinkedList and throw a ConcurrentModificationException when detected.

d-michail · 2019-06-17T07:27:19Z

jgrapht-core/src/main/java/org/jgrapht/alg/planar/ElementList.java

+     *
+     * @return the number of elements in this list
+     */
+    public int getSize() {


Better to use similar names like the List interface.

d-michail · 2019-06-17T07:29:00Z

jgrapht-core/src/main/java/org/jgrapht/alg/planar/ElementList.java

+     *
+     * @param list the list to prepend
+     */
+    public void prepend(ElementList<E> list) {


I think it would be better to use similar names like the ArrayDeque. This would make it easier for other Java users. For example "prepend" -> "addFirst" and "append" -> "addLast". Same comment applies in other names also.

d-michail · 2019-06-17T07:30:10Z

jgrapht-core/src/main/java/org/jgrapht/alg/planar/ElementList.java

+     * @param element the element of this list to search the list node of.
+     * @return the list node allocate for the {@code element}, or {@code null} if this list doesn't contain
+     * the {@code element}
+     */


Mention complexity of this method and the fact that equality checks uses method equals().

d-michail · 2019-06-17T07:33:32Z

jgrapht-core/src/main/java/org/jgrapht/alg/planar/BoyerMyrvoldPlanarityInspector.java

+    /**
+     * Performs sorting of the vertices by their lowpoints and adding them to the {@code separatedDfsChildList} lists
+     */
+    private void sortVetices() {


d-michail · 2019-06-17T07:34:47Z

jgrapht-core/src/main/java/org/jgrapht/alg/planar/BoyerMyrvoldPlanarityInspector.java

+                }
+            }
+        }
+//        for (Node node : nodes) {


Remove commented code.

Toptachamann · 2019-06-18T09:14:53Z

I haven't renamed the methods DoubleLinkedList.append(DoublyLinkedList) and DoubleLinkedList.prepend(DoublyLinkedList). @d-michail let me know whether they should also be renamed.

@jsichi that's a good idea. I think using the DoublyLinkedList in the Blossom algorithm can increase the memory usage a bit, but it's definitely worth giving it a try.

jsichi · 2019-06-24T19:28:02Z

(@Toptachamann we could also have an IntrusiveDoublyLinkedList to avoid the memory increase, like boost https://www.boost.org/doc/libs/1_64_0/doc/html/intrusive/list.html)

jkinable · 2019-10-09T18:36:53Z

@Toptachamann what's the status of this PR?

Toptachamann · 2019-10-10T19:25:16Z

The current issue is that I need to understand the idea about intrusive lists.

So, we need to implement something like

class IntrusiveListHook {
IntrusiveListHook prev, next;

public IntrusiveListHook next(){...}

public IntrusiveListHook prev(){...}

...
}

and then we might have

class Node extends IntrusiveListHook {
...
}

@jsichi Indeed, this is a good idea to try. The thing that I don't currently understand is how this can be used when we need to store the same node in multiple lists?

jsichi · 2019-10-10T19:58:44Z

Hmm, I forgot about the fact that C++ can deal with multiple inheritance instantiation of the same base class, whereas Java can't. (Likewise, the Boost member technique relies on being able to calculate the memory offset from the beginning of the object to the member, which isn't applicable to Java.) So there may be no easy+clean way to do it.

In any case, this PR itself should not be held up for intrusive lists; my suggestion was to address that in a followup PR.

d-michail · 2019-10-14T10:03:05Z

No further comments from me. This looks ready to be merged!

jsichi · 2019-10-15T18:13:44Z

I'll go ahead and merge this in a few days unless I see new comments from @jkinable

Finally we can update our "open tasks" page!

jkinable · 2019-10-21T06:15:34Z

I'm at a conference till Wednesday, I made a note to look at this on Thursday

…

On Tue, Oct 15, 2019, 11:13 AM John Sichi ***@***.***> wrote: I'll go ahead and merge this in a few days unless I see new comments from @jkinable <https://github.com/jkinable> Finally we can update our "open tasks" page! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#795?email_source=notifications&email_token=AAU5RSV652VOSKSS6V35TJ3QOYB5XA5CNFSM4HYRF2QKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBJW6WY#issuecomment-542338907>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAU5RSQNKJXD4XGE4DRDSBLQOYB5XANCNFSM4HYRF2QA> .

jkinable

Beautiful, absolutely well done @Toptachamann ! I'm super happy that we can finally take this of our wish list.

jkinable · 2019-10-26T19:07:56Z

jgrapht-core/src/main/java/org/jgrapht/util/DoublyLinkedList.java

+ * @param <E> the list element type
+ * @author Timofey Chudakov
+ */
+public class DoublyLinkedList<E> implements Iterable<E> {


For a future PR, it might be good to see whether we can also implement the List interface.

jgrapht-core/src/main/java/org/jgrapht/util/DoublyLinkedList.java

jkinable · 2019-10-26T19:12:38Z

jgrapht-core/src/main/java/org/jgrapht/util/DoublyLinkedList.java

+    /**
+     * Returns a reverse iterator over this list, which starts from the {@code element}.
+     * Note: the returned iterator will iterate over the all list, meaning it won't
+     * stop at the end of the list unless the {@code element} is the beginning of the list.


I'm not sure I understand this. If an element is the first element of the list, why wouldn't the reverseIterator wrap around and walk the list back to the beginning?

Good point, this is a bit incorrect. The reverse iterator will stop at the beginning of the list in the case the start node is the last element.

I still find the description of reverseIteratorFrom(.) confusing. What about the following description:

Returns an iterator over this list, which traverses the list in the reverse direction. The returned iterator will iterate over the <i>entire</i> list, that is, the list is treated as if it were cyclic. The iterator starts from {@code element}, and walks the list backwards, while wrapping around at the beginning of the list. For instance, for the list [1,2,3,4], {@code reverseIteratorFrom(3)} would return 3,2,1,4. This method throws {@link NoSuchElementException} in the case {@code element} doesn't belong to this list. This method invokes {@link #getNode(Object) getNode} to find the list node containing {@code element}.

jkinable · 2019-10-26T19:33:18Z

jgrapht-core/src/main/java/org/jgrapht/GraphTests.java

+    }
+
+    /**
+     * Checks whether the {@code graph} is a $K_5$ subdivision.


High level comment: $K_5$ subdivisions and K_{3,3}$ subdivision are quite specific. It might be a little too specific for for the general GraphTests class. For now let's leave it here, since I've no idea where else to put these.

jkinable · 2019-11-02T00:28:18Z

jgrapht-core/src/main/java/org/jgrapht/util/DoublyLinkedList.java

+
+    /**
+     * Finds and returns the list node allocate for the {@code element}. If this list doesn't contain
+     * the {@code element}, returns {@code null}. The timer complexity of this method is linear in the


typo: timer->time

jkinable · 2019-11-05T13:39:52Z

Awesome work @Toptachamann ! We can finally remove this from our todo list. Keep up the good work :)

Toptachamann added 4 commits June 16, 2019 16:21

Boyer-Myrvold

3489f63

Removed unnecessary changes in GraphTestsTest

6884841

Added package-info.java to the git index

032d01a

Added newline to the end of the package-info.java

bd422bd

d-michail reviewed Jun 17, 2019

View reviewed changes

Changed methods names, moved code and added tests

7af6b56

Removed unnecessary changes and fixed one more typo

6374ee1

jsichi mentioned this pull request Oct 15, 2019

Hopcroft Tarjan planarity test algorithm #416

Closed

jkinable reviewed Oct 26, 2019

View reviewed changes

Fixed docs in the DoublyLinkedList

87dd968

jkinable reviewed Nov 2, 2019

View reviewed changes

Improved Java doc

514b434

jkinable merged commit 5c607dc into jgrapht:master Nov 5, 2019

syoon2 mentioned this pull request Apr 12, 2024

Add Missing License Headers #1215

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Boyer-Myrvold planarity testing algorithm #795

Boyer-Myrvold planarity testing algorithm #795

Toptachamann commented Jun 16, 2019

jsichi commented Jun 17, 2019

d-michail left a comment

d-michail Jun 17, 2019

d-michail Jun 17, 2019

d-michail Jun 17, 2019

d-michail Jun 17, 2019

d-michail Jun 17, 2019

d-michail Jun 17, 2019

d-michail Jun 17, 2019

d-michail Jun 17, 2019

Toptachamann commented Jun 18, 2019

jsichi commented Jun 24, 2019

jkinable commented Oct 9, 2019

Toptachamann commented Oct 10, 2019

jsichi commented Oct 10, 2019

d-michail commented Oct 14, 2019

jsichi commented Oct 15, 2019

jkinable commented Oct 21, 2019 via email

jkinable left a comment

jkinable Oct 26, 2019

jkinable Oct 26, 2019

Toptachamann Nov 1, 2019

jkinable Nov 2, 2019

jkinable Oct 26, 2019

jkinable Nov 2, 2019

jkinable commented Nov 5, 2019

Boyer-Myrvold planarity testing algorithm #795

Boyer-Myrvold planarity testing algorithm #795

Conversation

Toptachamann commented Jun 16, 2019

jsichi commented Jun 17, 2019

d-michail left a 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

Choose a reason for hiding this comment

Toptachamann commented Jun 18, 2019

jsichi commented Jun 24, 2019

jkinable commented Oct 9, 2019

Toptachamann commented Oct 10, 2019

jsichi commented Oct 10, 2019

d-michail commented Oct 14, 2019

jsichi commented Oct 15, 2019

jkinable commented Oct 21, 2019 via email

jkinable left a 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

jkinable commented Nov 5, 2019