Add new plantri spkg - Githubissues

nvcleemp commented 10 years ago

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.

Depends on #16972

CC: @slel

Component: packages: optional

Keywords: plantri

Author: Nico Van Cleemput

Branch: aa60bf0

Reviewer: Nathann Cohen

Issue created by migration from https://trac.sagemath.org/ticket/16970

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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

023aba4 Adding new plantri spkg

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Commit: 023aba4

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

comment:2

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 commented 10 years ago

comment:3

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 commented 10 years ago

comment:4

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

nvcleemp commented 10 years ago

Dependencies: #16972

nvcleemp commented 10 years ago

comment:5

Oops, this is certainly not yet ready for review.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 023aba4 to 592a03a

nvcleemp commented 10 years ago

comment:7

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 592a03a to 8808de2

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

comment:9

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

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

comment:10

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

nvcleemp commented 10 years ago

comment:11

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.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

comment:12

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 commented 10 years ago

comment:13

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 8808de2 to 3b6b494

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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)`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 3b6b494 to 90a09b7

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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

d9367c5 #16970: Extended documentation for quadrangulations

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Changed commit from 90a09b7 to d9367c5

nvcleemp commented 10 years ago

comment:17

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! :-)

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

comment:18

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 commented 10 years ago

comment:19

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

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

Changed commit from d9367c5 to aa60bf0

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

Reviewer: Nathann Cohen

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

Changed branch from u/nvcleemp/plantri-spkg to public/16970

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 10 years ago

comment:20

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 commented 10 years ago

comment:21

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

vbraun commented 10 years ago

Changed branch from public/16970 to aa60bf0

slel commented 6 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,12 @@
-Adds a new spkg containing the plantri program. Another ticket will handle the implementation of the methods calling plantri.
+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.
+

slel commented 6 years ago

Changed commit from aa60bf0 to none

slel commented 6 years ago

Changed keywords from none to plantri

sagemath / sage

Add new plantri spkg #16970