Add new plantri spkg

[nvcleemp]

Adds a new spkg containing the plantri program.

To use it in Sage, run the following in a terminal:

sage -i plantri

The tarball is located at ​http://www.nvcleemp.be/plantri-4.5.tar.bz2

Another ticket will handle the implementation of the methods calling plantri. See #17344.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​023aba4

Adding new plantri spkg

[ncohen]

Yo !

I think you should do both at the same time, otherwise it is hard to test the program you add. Plus it is weird to have a code that Sage cannot use, even for a time. As adding a new spkg does not take a lot of code there should be no problem doing both here !

Thank you by the way, it will be nice to have it around.

Nathann

[nvcleemp]

Ok, I'm making the changes today, but they will take a bit longer than just adding the spkg. Anyway, it's just pushing to a difference branch, so for me it's all the same.

[nvcleemp]

When the code to interact with plantri is added, then this will need the changes from #16972.

[nvcleemp]

Oops, this is certainly not yet ready for review.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​cedb881	Trac #16972: Moved code to read planar code to separate method
​8d0c0d5	Trac #16972: Extended documentation of _read_planar_code
​bb9be9e	Trac #16972: Use the new _read_planar_code method in fullerenes and fusenes.
​1975236	Merge changes of #16972 into this branch
​592a03a	#16970: Adding method to generate plane graphs

[nvcleemp]

I still have a lot of work on this ticket, but I thought I'd already push some changes, so that if somebody has some comments, then I can already take them into account.

Cheers, Nico

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​15c0bf3	#16970: Adding method to generate plane triangulations
​7328de5	#16970: Changed formation of command
​8808de2	#16970: Added method to generate quadrangulations

[ncohen]

Hellooooooooo !

Several comments about this branch:

• def plane_graphs -> def planar_graphs ?

• Return a generator which creates general plane graphs using the plantri generator (see [plantri]_) --> An iterator over planar graphs using the plantri generator ? It makes it more clear that they are not just 'some planar graphs' but 'all planar graphs' corresponding to the constraints. Also we try to keep the first line of doc in a function rather short, so the reference can appear later.

• You should add the new functions to the documentation at the head of graph_generators.py to let everybody know that they exist.

• There seems to be a lot of copy/paste in the three functions. Is there any reason why they should not all call planar_graphs which would do the parsing ?

• Could you mention that plantri is an optional package and that it has to be installed first if it is to work at all ? Actually, you can even try to check first if the package is installed, and raise a useful exception otherwise "i.e. --> install it"

​http://www.sagemath.org/doc/reference/misc/sage/misc/package.html

Thanks for this work ! It will be useful to many people I hope :-)

Nathann

[ncohen]

(oh, and the lines of the new spkg file you add should be 80 characters long)

[nvcleemp]

Hi

Thanks for the feedback.

The documentation is currently being extended, but I'm travelling at the moment so I didn't get as far with this ticket as I hoped to have.

I thought about making it just one function, but thought that it would make the meaning of the arguments less obvious to users. Of course, you could just add an argument triangulations and quadrangulations, but I think that having separate methods is a cleaner solution. I could add a helper function to which the three functions dispatch to factor out the common code.

Normally the code already checks that the package is installed. I will add that the package is optional.

Concerning the name. Since also graphs with a connectivity lower than 3 can be generated, this function really generates plane graphs, i.e., planar graphs together with an embedding, and not just planar graphs. The graphs are generated up to isomorphism of the plane graph.

[ncohen]

Helloooooo !

The documentation is currently being extended, but I'm travelling at the moment so I didn't get as far with this ticket as I hoped to have.

Oh I see I see, I had not even noticed that the ticket was not in needs_review. Sorry for interrupting.

I thought about making it just one function, but thought that it would make the meaning of the arguments less obvious to users. Of course, you could just add an argument triangulations and quadrangulations, but I think that having separate methods is a cleaner solution. I could add a helper function to which the three functions dispatch to factor out the common code.

Nononnono I agree with that, the four functions are fine ! I was only talking about the code: why wouldn't the first function call the other ones so that there is no duplication of code ?

Concerning the name. Since also graphs with a connectivity lower than 3 can be generated, this function really generates plane graphs, i.e., planar graphs together with an embedding, and not just planar graphs. The graphs are generated up to isomorphism of the plane graph.

Oh, do you mean that the function will output all different embeddings of non-3-connected planar graphs ? If so that's worth a note/warning in the doc !

I am still a bit troubled by this distinction between "plane graphs" and "planar graphs". Especially when the website of plantri/fullgen reads "planar graph" everywhere.

​http://cs.anu.edu.au/~bdm/plantri/

Nathann

[nvcleemp]

Hi

Oh I see I see, I had not even noticed that the ticket was not in needs_review. Sorry for interrupting.

Not a problem. It's always good to have some comments along the way.

Nononnono I agree with that, the four functions are fine ! I was only talking about the code: why wouldn't the first function call the other ones so that there is no duplication of code ?

That's true. I'll have a look at fixing that.

Oh, do you mean that the function will output all different embeddings of non-3-connected planar graphs ? If so that's worth a note/warning in the doc !

Yes, it will do that.

I am still a bit troubled by this distinction between "plane graphs" and "planar graphs". Especially when the website of plantri/fullgen reads "planar graph" everywhere.

​http://cs.anu.edu.au/~bdm/plantri/

Yes, I know. I actually don't know which notation Brendan uses, but I at least know that Gunnar is quite adamant about the distinction. I think they just never got around to changing it from the original wording.

But I can live with planar_graphs if it is noted clearly that for non-3-connected graphs each distinct planar embedding will be output.

At the moment I'm too jet-lagged to risk writing any code. (Unless you want some extra work as a reviewer ;-)) I'll continue work on this ticket soon.

Nico

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​99f6345	#16970: Renamed plane graphs to planar graphs
​09f9a83	#16970: Removed duplicate references
​3b6b494	#16970: Added new functions to documentation

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​a22f64d	#16970: Correct handling of case where order is equal to 1
​50e8b33	#16970: Extended documentation
​b9c7e38	#16970: Extended documentation
​738406d	#16970: Updated the description of the output
​556250e	#16970: Replaced plane by planar
​fcff91a	#16970: Extended documentation for triangulations
​30b0098	#16970: Correct handling of order that is too small for plantri (triangulations)
​bacafc2	#16970: Correct handling of order that is too small for plantri (planar graphs)
​90a09b7	#16970: Correct handling of order that is too small for plantri (quadrangulations)

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​d9367c5

#16970: Extended documentation for quadrangulations

[nvcleemp]

I had a look at factoring out common code. The problem is that although it looks like there is a lot of common code, the exact values that determine a valid input are different for each case. Internally plantri also uses separate logics for each case. If somebody sees a way to factor out all this logic, then I am curious to hear it! :-)

[ncohen]

I had a look at factoring out common code. The problem is that although it looks like there is a lot of common code, the exact values that determine a valid input are different for each case. Internally plantri also uses separate logics for each case. If somebody sees a way to factor out all this logic, then I am curious to hear it! :-)

Well I gave it a try because I really did not like to see the same lines in different functions (like n<64, ...) but after having begun to do the job, I am convinced that having all the logic in one function does not make the code much clearer. Soooooo Well, you are right, let us keep it like that !

Do you have anything left to do with the code or is it in needs_review ?

Nathann

[nvcleemp]

Yeah, I'm also not a big fan of those repetitions, but I also tried different approaches and always ended up with something that was less clear.

I had another look at the code, and right now there is nothing I want to add, so I guess it's in needs_review. If I missed something I'm sure you or someone else will (politely) point me to it. ;-)

Nico

[ncohen]

Helloooooooooo !

Okay, I just spent some time re-re-re-reading your code, to get to the very obvious conclusion that it is very very clean.

I added a small commit on top of yours that does simple things:

• Length of the lines in the SPKG.txt file
• All functions should begin with a short line describing their job
• Refactored several 'if'
• I never know what "between 1 and 5" means (strict or not ?) so I replaced it with \geq and \leq.
• Added some missing # optional - plantri that would have made it break on machines on which the package is not installed.

Very good work. Thank a lot !

Nathann

P.S. : If you agree with the new commit you can set the ticket to positive_review

---

New commits:

​b572274	trac #16970: Merged with beta4
​aa60bf0	trac #16970: review

[nvcleemp]

Hi

I had a look at your commits. Everything still works and I learned some new style guide lines for Sage. ;-)

Set to positive_review.

Looking forward to have this in Sage (without having to pull this branch). :-)

Nico