Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#16970 closed enhancement (fixed)

Add new plantri spkg

Reported by: nvcleemp Owned by:
Priority: minor Milestone: sage-6.4
Component: packages: optional Keywords: plantri
Cc: slelievre Merged in:
Authors: Nico Van Cleemput Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: aa60bf0 (Commits) Commit:
Dependencies: #16972 Stopgaps:

Description (last modified by slelievre)

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.

Change History (23)

comment:1 Changed 6 years ago by git

  • Commit set to 023aba46ca0bbda773fff78625c5127430aeeeac

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

023aba4Adding new plantri spkg

comment:2 Changed 6 years ago by 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

comment:3 Changed 6 years ago by 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.

comment:4 Changed 6 years ago by nvcleemp

  • Dependencies set to #16972
  • Status changed from new to needs_review

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

comment:5 Changed 6 years ago by nvcleemp

  • Status changed from needs_review to needs_work

Oops, this is certainly not yet ready for review.

comment:6 Changed 6 years ago by git

  • Commit changed from 023aba46ca0bbda773fff78625c5127430aeeeac to 592a03afb913501e0a1d095f4b458f772afde152

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

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

comment:7 Changed 6 years ago by 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

comment:8 Changed 6 years ago by git

  • Commit changed from 592a03afb913501e0a1d095f4b458f772afde152 to 8808de28c104ef9bd620c86be02bd9e20a2b4469

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

comment:9 Changed 6 years ago by 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

comment:10 Changed 6 years ago by ncohen

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

comment:11 follow-up: Changed 6 years ago by 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.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 6 years ago by 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

comment:13 in reply to: ↑ 12 Changed 6 years ago by 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

comment:14 Changed 6 years ago by git

  • Commit changed from 8808de28c104ef9bd620c86be02bd9e20a2b4469 to 3b6b49435278d58f0667c47b6f47c4111bc2af2d

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

comment:15 Changed 6 years ago by git

  • Commit changed from 3b6b49435278d58f0667c47b6f47c4111bc2af2d to 90a09b73b729c24873b2422c513b838491cf57ea

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)

comment:16 Changed 6 years ago by git

  • Commit changed from 90a09b73b729c24873b2422c513b838491cf57ea to d9367c56bd0edc28f18bdaa93de51948a622c0b7

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

d9367c5#16970: Extended documentation for quadrangulations

comment:17 follow-up: Changed 6 years ago by 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! :-)

comment:18 in reply to: ↑ 17 Changed 6 years ago by 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

comment:19 Changed 6 years ago by nvcleemp

  • Status changed from needs_work to needs_review

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

comment:20 Changed 6 years ago by ncohen

  • Branch changed from u/nvcleemp/plantri-spkg to public/16970
  • Commit changed from d9367c56bd0edc28f18bdaa93de51948a622c0b7 to aa60bf0d85ff499085c2bd3062356579b24457a9
  • Reviewers set to Nathann Cohen

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:

b572274trac #16970: Merged with beta4
aa60bf0trac #16970: review

comment:21 Changed 6 years ago by nvcleemp

  • Status changed from needs_review to positive_review

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

comment:22 Changed 6 years ago by vbraun

  • Branch changed from public/16970 to aa60bf0d85ff499085c2bd3062356579b24457a9
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:23 Changed 3 years ago by slelievre

  • Cc slelievre added
  • Commit aa60bf0d85ff499085c2bd3062356579b24457a9 deleted
  • Description modified (diff)
  • Keywords plantri added
Note: See TracTickets for help on using tickets.