Add IsPlanarDigraph and related #156

james-d-mitchell · 2018-12-11T14:09:50Z

This PR adds functionality to Digraphs to check whether a digraph is planar, and some related functionality. This is essentially just a wrapper around the C library edge-addition-planarity-suite, plus a little bit more.

It is currently missing:

some tests
all doc
valgrind

Things to keep in mind:

This makes Digraphs dependent on an external C-library (which appears to work well as an autohell subpackage). A copy of the library is included in this PR. I'm not completely sure this is desirable.
It might be the case that at some point in the future edge-addition-planarity-suite stops working on some platforms or is no longer maintained. At that point, we can introduce a configure flag --disable-edge-addition-planarity-suite to disable these features. I don't really want to add the flag now, because then we have to test with and without the flag and everything gets more complex.

wilfwilson · 2018-12-11T14:39:39Z

Thanks, I'll review once there are tests and documentation and the CI passes.

james-d-mitchell · 2018-12-11T14:49:54Z

Interestingly, this almost works ;)

james-d-mitchell · 2018-12-11T14:51:34Z

Also I will split the PR into 2 commits, one adding all the files in the subpackage and the other with the changes to Digraphs itself

james-d-mitchell · 2018-12-11T14:53:30Z

Also @wilfwilson any chance you can merge stable-0.14 into master? So I can rebase on that?

wilfwilson · 2018-12-11T15:01:29Z

Merged!

james-d-mitchell · 2018-12-11T15:08:41Z

Super thanks!

james-d-mitchell · 2018-12-14T15:47:47Z

I think this is more or less good to go. I'm just going to valgrind, and if the CI passes, then this is ready for review.

james-d-mitchell · 2018-12-14T15:57:58Z

Oh wait I forgot, I don't know how to valgrind GAP, I committed some files that are generated by mistake will remove them now.

james-d-mitchell · 2018-12-14T16:04:47Z

Ready to go

wilfwilson

Hi @james-d-mitchell, thanks for doing this. In my review I've asked a few questions, and otherwise my comments basically amount to some tweaks to the documentation.

wilfwilson · 2018-12-14T19:35:30Z

doc/planar.xml

+  <Description>
+    An <E>outer planar</E> digraph is a digraph that can be embedded in the
+    plane in such a way that its edges do not intersect, and all vertices
+    belong to the unbounded face of the embedding.  A digraph is planar if and


A digraph is planar or outer planar?

wilfwilson · 2018-12-14T19:35:59Z

doc/planar.xml

+  <Description>
+    <C>KuratowskiPlanarSubdigraph</C> returns the list of out-neighbours
+    of a (not necessarily induced) subdigraph of the digraph <A>digraph</A>
+    that witness the fact that <A>digraph</A> is not planar, or


witness -> witnesses

wilfwilson · 2018-12-14T19:37:35Z

doc/planar.xml

+    <K>fail</K> if <A>digraph</A> is planar.  In other words,
+    <C>KuratowskiPlanarSubdigraph</C> returns the out-neighbours of a
+    subdigraph of <A>digraph</A> that is homeomorphic to the complete graph
+    with four vertices, or to the complete bipartite graph with vertex sets of


Don't you mean K_5 and K_3,3 and not K_4 and K_2,3?

Also note that here you talk about the complete graph with four vertices whereas above you talk about the complete graph on <C>4</C> vertices - it's not vital but I'd recommend to use the same wording.

wilfwilson · 2018-12-14T19:40:08Z

doc/planar.xml

+    If <A>digraph</A> is a planar digraph, then 
+    <C>PlanarEmbedding</C> returns the list of out-neighbours
+    of a subdigraph of <A>digraph</A> such that each vertex's
+    neighbors are given in clockwise order. If <A>digraph</A> is not planar,


Use neighbours for consistency with the rest of the documentation/sentence please.

wilfwilson · 2018-12-14T19:42:36Z

doc/planar.xml

+  <Attr Name="PlanarEmbedding" Arg="digraph"/>
+  <Returns>A list or <K>fail</K>.</Returns>
+  <Description>
+    If <A>digraph</A> is a planar digraph, then 


I'm not sure this documentation properly conveys what's happening here. The point is that if the vertices are drawn as suggested by the returned list, then you can fill in the edges without edges crossing. I feel like the connection between this clockwise property and planarity could be elucidated a bit more.

Perhaps this could be clearer, I have not been able to find any description of the return value of this method (from the edge-addition-planarity-suite), so I basically copied what is written here from the corresponding manual entry from Sage see here.

Okay, I appreciate the explanation. To me, this bit of documentation is pretty much meaningless (as is the Sage entry) – I don't actually know whether the description "The point is that..." that I gave above is correct. If you think you can work out and write a better description, please do, but otherwise it won't stop me merging.

wilfwilson · 2018-12-14T19:47:31Z

doc/planar.xml

+    the complete bipartite graph with vertex sets of sizes <C>2</C> and
+    <C>3</C>; the complete bipartite graph with vertex sets of sizes <C>3</C>
+    and <C>3</C>; or the complete graph with <C>4</C> vertices. If
+    <A>digraph</A> has no such subdigraphs, then <K>fail</K> is returned. 


Maybe emphasise the point that a digraph is planar/outer planar if and only if it has no such sub digraph?

Why does SubdigraphHomeomorphicToK5 not exist?

I've added a link to the doc for IsPlanarDigraph and IsOuterPlanarDigraph, rather than repeating the doc here again.

Why does SubdigraphHomeomorphicToK5 not exist?

Because it isn't implemented in the edge-addition-planarity-suite, and it isn't clear to me how to do this (at least in any good way). I did have a version of this PR where I had a function called SubdigraphHomeomorphicTo(digraph, subdigraph) but I abandoned it as too complicated in the first instance (it would only have worked for digraphs homeomorphic to K_4, K_{2, 3}, and K_{3, 3}, anyway.

Fair enough, I'm surprised that it doesn't contain a K_5 thing, but that it does contain the other three, but then again I have no idea what the suite is doing!

wilfwilson · 2018-12-14T19:48:46Z

gap/attr.gi

@@ -384,14 +384,10 @@ InstallMethod(OutNeighbours, "for a digraph",
 [IsDigraph],
 function(digraph)
  local out;
-  if IsBound(digraph!.adj) then
-    return digraph!.adj;


Why is this change included here? And what is the explanation for its deletion?

Didn't we remove the component adj from all digraphs at some point in the not too distant past? This was originally used to hold the OutNeighbours for access in the C code, but if I recall this was unnecessary, and I think you removed it. I believe this code is a remnant of the older code, and I noticed it when writing tests for MaximalAntiSymmetricSubdigraph (it was not covered by any tests, and removing it seems to have no effect). Perhaps it should be in another PR, but it is in a separate commit at least.

Ah right, no problem. I'm not sure if I did the changes, but yes this the adj component is removed, and certainly if these lines are not being covered by tests then it's not needed. I'm not normally against a PR changing unrelated things, but my default assumption when reviewing a PR is that the changes are related, and so I was confused at this point because I couldn't work out what the relation was here.

wilfwilson · 2018-12-14T19:49:53Z

gap/attr.gi

+  n := DigraphNrVertices(D);
+  if n <= 1
+      or (HasIsAntisymmetricDigraph(D) and IsAntisymmetricDigraph(D)) then
+    return D;


If n <= 1 or the digraph is already antisymmetric, it's possible that there are still multiple edges, which the documentation forbids!

Ooops, I swapped the clauses around here.

wilfwilson · 2018-12-14T19:50:42Z

gap/oper.gi

@@ -1005,13 +1005,13 @@ function(digraph, edge)
 end);

 InstallMethod(IsDigraphEdge, "for a digraph, pos int, pos int",


Change the string please to agree with the args

wilfwilson · 2018-12-14T19:51:14Z

gap/oper.gi

 function(digraph, u, v)
  local n;

  n := DigraphNrVertices(digraph);

-  if u > n or v > n then
+  if u > n or v > n or u < 0 or v < 0 then


Why do we want to allow 0? Vertices are currently restricted to [1..n]

Double sorry my bad. BTW the reason for this change was that I want to call IsDigraphEdge in the C code, and there, sometimes GAP integers don't seem belong to IsPosInt even though they are positive and would satisfy IsPosInt in GAP.

wilfwilson · 2018-12-14T19:54:05Z

I am also a bit wary of including a whole library. But if it seems to work, then let's do it. If it causes problems for us or other users in the future, then we can deal with this then.

james-d-mitchell · 2018-12-15T10:02:33Z

Thanks @wilfwilson I’ll try to update the pr today

james-d-mitchell · 2018-12-15T15:43:33Z

I agree with you @wilfwilson, on the other hand, we already rely on Bliss, so maybe it's not that bad either. I'll update the PR just now.

james-d-mitchell · 2018-12-15T15:45:53Z

Updated! Off to Paris now, will check in again tomorrow. Thanks for the review @wilfwilson !

wilfwilson · 2018-12-16T19:23:06Z

gap/prop.gd

@@ -22,6 +22,7 @@ DeclareProperty("IsReflexiveDigraph", IsDigraph);
 DeclareProperty("IsStronglyConnectedDigraph", IsDigraph);
 DeclareProperty("IsSymmetricDigraph", IsDigraph);
 DeclareProperty("IsAntisymmetricDigraph", IsDigraph);
+DeclareSynonym("IsAntiSymmetricDigraph", IsAntisymmetricDigraph);


I think this should be DeclareSynonymAttr.

wilfwilson · 2018-12-16T19:33:12Z

I hope you remembered your yellow vest @james-d-mitchell! ~~I'll merge this tomorrow, to give you the chance to make any additional tweaks.~~ See later comments, and please change DeclareSynonym to DeclareSynonymAttr.

wilfwilson · 2018-12-18T13:39:53Z

@james-d-mitchell I just thought about licensing issues. Is the license of digraphs compatible with the suite, and vice versa? Do we need to include a copy of their license or something?

wilfwilson · 2018-12-19T13:33:38Z

From https://github.com/graph-algorithms/edge-addition-planarity-suite/blob/master/LICENSE.TXT:

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are met:

* Redistributions of source code must retain the above copyright notice, this
  list of conditions and the following disclaimer.

* Redistributions in binary form must reproduce the above copyright notice,
  this list of conditions and the following disclaimer in the documentation
  and/or other materials provided with the distribution.

* Neither the name of The Edge Addition Planarity Suite nor the names of its
  contributors may be used to endorse or promote products derived from
  this software without specific prior written permission.

Fair enough. At the top of all the new files you've retained

/*
Copyright (c) 1997-2015, John M. Boyer
All rights reserved.
See the LICENSE.TXT file for licensing information.
*/

But I think it would be a good idea to actually include that LICENSE.TXT file as, for instance,
edge-addition-planarity-suite-Version_3.0.0.5/LICENSE.TXT.

I had to tweak it ever so slightly to get it to work, so this is not exactly version 3.0.0.5.

james-d-mitchell · 2019-01-07T13:04:11Z

hi @wilfwilson finally back at my keyboard. i thought I did include a copy of the LICENSE file, but you're correct, it looks like I didn't. I have just pushed a new version including the LICENSE file.

james-d-mitchell requested a review from wilfwilson December 11, 2018 14:09

james-d-mitchell added new-feature A label for new features. 0.15 do not merge A label for PRs that should not be merged for whatever reason. labels Dec 11, 2018

james-d-mitchell force-pushed the planar branch from 23fa2eb to 105a467 Compare December 11, 2018 15:11

james-d-mitchell added 3 commits December 12, 2018 15:17

attr: remove some legacy weirdness

379d0d6

prop: add synonym IsAntiSymmetricDigraph

6b651ca

io: fix typos in doc

918825d

james-d-mitchell force-pushed the planar branch from 105a467 to a45a3c0 Compare December 14, 2018 15:46

james-d-mitchell removed the do not merge A label for PRs that should not be merged for whatever reason. label Dec 14, 2018

james-d-mitchell force-pushed the planar branch from a45a3c0 to 6946673 Compare December 14, 2018 16:04

wilfwilson requested changes Dec 14, 2018

View reviewed changes

james-d-mitchell force-pushed the planar branch from 6946673 to ccc9e27 Compare December 15, 2018 15:45

wilfwilson reviewed Dec 16, 2018

View reviewed changes

wilfwilson approved these changes Dec 16, 2018

View reviewed changes

james-d-mitchell added 3 commits January 7, 2019 13:02

planar: add copy of edge-addition-planarity-suite

6d81ae8

I had to tweak it ever so slightly to get it to work, so this is not exactly version 3.0.0.5.

planar: add IsPlanarDigraph and related

3c0ab68

kernel: fix header includes

ba19a25

james-d-mitchell force-pushed the planar branch from ccc9e27 to ba19a25 Compare January 7, 2019 13:03

wilfwilson merged commit ef0169e into digraphs:master Jan 7, 2019

		@@ -1005,13 +1005,13 @@ function(digraph, edge)
		end);

		InstallMethod(IsDigraphEdge, "for a digraph, pos int, pos int",

Add IsPlanarDigraph and related #156

Add IsPlanarDigraph and related #156

Conversation

james-d-mitchell commented Dec 11, 2018 • edited Loading

wilfwilson commented Dec 11, 2018

james-d-mitchell commented Dec 11, 2018

james-d-mitchell commented Dec 11, 2018

james-d-mitchell commented Dec 11, 2018

wilfwilson commented Dec 11, 2018

james-d-mitchell commented Dec 11, 2018

james-d-mitchell commented Dec 14, 2018

james-d-mitchell commented Dec 14, 2018

james-d-mitchell commented Dec 14, 2018

wilfwilson 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilfwilson commented Dec 14, 2018

james-d-mitchell commented Dec 15, 2018

james-d-mitchell commented Dec 15, 2018

james-d-mitchell commented Dec 15, 2018

Choose a reason for hiding this comment

wilfwilson commented Dec 16, 2018 • edited Loading

wilfwilson commented Dec 18, 2018

wilfwilson commented Dec 19, 2018

james-d-mitchell commented Jan 7, 2019

james-d-mitchell commented Dec 11, 2018 •

edited

Loading

wilfwilson commented Dec 16, 2018 •

edited

Loading