#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, GitHub, GitLab) | Commit: | |
Dependencies: | #16972 | Stopgaps: |
Description (last modified by )
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 8 years ago by
- Commit set to 023aba46ca0bbda773fff78625c5127430aeeeac
comment:2 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
- 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 8 years ago by
- Status changed from needs_review to needs_work
Oops, this is certainly not yet ready for review.
comment:6 Changed 8 years ago by
- Commit changed from 023aba46ca0bbda773fff78625c5127430aeeeac to 592a03afb913501e0a1d095f4b458f772afde152
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
|
comment:7 Changed 8 years ago by
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 8 years ago by
- Commit changed from 592a03afb913501e0a1d095f4b458f772afde152 to 8808de28c104ef9bd620c86be02bd9e20a2b4469
comment:9 Changed 8 years ago by
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 8 years ago by
(oh, and the lines of the new spkg file you add should be 80 characters long)
comment:11 follow-up: ↓ 12 Changed 8 years ago by
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: ↓ 13 Changed 8 years ago by
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
andquadrangulations
, 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 8 years ago by
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.
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 8 years ago by
- Commit changed from 8808de28c104ef9bd620c86be02bd9e20a2b4469 to 3b6b49435278d58f0667c47b6f47c4111bc2af2d
comment:15 Changed 8 years ago by
- 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 8 years ago by
- 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: ↓ 18 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
- 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 8 years ago by
- 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:
b572274 | trac #16970: Merged with beta4
|
aa60bf0 | trac #16970: review
|
comment:21 Changed 8 years ago by
- 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 8 years ago by
- Branch changed from public/16970 to aa60bf0d85ff499085c2bd3062356579b24457a9
- Resolution set to fixed
- Status changed from positive_review to closed
comment:23 Changed 4 years ago by
- Cc slelievre added
- Commit aa60bf0d85ff499085c2bd3062356579b24457a9 deleted
- Description modified (diff)
- Keywords plantri added
Branch pushed to git repo; I updated commit sha1. New commits:
Adding new plantri spkg