Opened 10 years ago
Closed 10 years ago
#12952 closed enhancement (fixed)
DoubleStarSnark, FosterGraph, GrayGraph and HarriesGraph
Reported by: | ncohen | Owned by: | jason, ncohen, rlm |
---|---|---|---|
Priority: | major | Milestone: | sage-5.1 |
Component: | graph theory | Keywords: | |
Cc: | wdj, kini | Merged in: | sage-5.1.beta2 |
Authors: | Nathann Cohen | Reviewers: | Keshav Kini |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12942, #12945 | Stopgaps: |
Description
This patch adds 4 new graph constructors, that is DoubleStarSnar?, FosterGraph?, GrayGraph? and HarriesGraph?.
I am really having fun trying to build good drawings, but these graphs really get complicated sometimes !!! :-)
Nathann
Attachments (2)
Change History (18)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Cc kini added
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
- Dependencies set to #12942, #12945
comment:3 Changed 10 years ago by
- Reviewers set to Keshav Kini
comment:4 Changed 10 years ago by
Doing some refactoring... why are the graphs in this file in such a strange order... Nathann, are you working on any more graphs now? After you're done with your graph creation spree please let me know so I can make a ticket to reorder the graphs in this file :)
comment:5 follow-up: ↓ 10 Changed 10 years ago by
Here is a review patch! What I changed is explained in the commit message. Many of the changes are pretty trivial... please let me know if my messing around with your code so much is bothering you :)
comment:6 Changed 10 years ago by
Patchbot: apply trac_12952.patch trac_12952.review.patch
comment:7 Changed 10 years ago by
- Summary changed from DoubleStarSnar, FosterGraph, GrayGraph and HarriesGraph to DoubleStarSnark, FosterGraph, GrayGraph and HarriesGraph
comment:8 Changed 10 years ago by
Patchbot gives this a green circle - or would, if it wasn't still only approving test results from 4.8...
comment:9 Changed 10 years ago by
Helloooooooooo !!!
Oh, what about the order ? I thought I had them sorted in alphabetical order O_o
Nathann
comment:10 in reply to: ↑ 5 Changed 10 years ago by
Here is a review patch! What I changed is explained in the commit message. Many of the changes are pretty trivial... please let me know if my messing around with your code so much is bothering you :)
Thank you for your patch ! I have to admit I do not like to see lines begin with a "," but most of your changes are pretty sound and I actually should have made them myself before sending the patch ^^;
comment:11 follow-up: ↓ 12 Changed 10 years ago by
Oh, the multiline list syntax? That is something I borrowed from Haskell style - IMO it is nice for a couple of reasons, 1) there is a visual line down the left side of the list which clearly shows where it begins and ends, and 2) adding a new element to the end of the list will only cause a diff of +1 line, not +2 lines / -1 lines like with the usual Python style. But I'll get rid of it if you want :)
About the order, I didn't mean about your patch specifically (though your patch is also not following alphabetic order - D comes after C! :P ) - I mean the whole file. I mean, why is HarriesGraph
at the top of the file but DoubleStarSnark
is under "Named Graphs"? It seems like the top of the file was originally for small graphs that are commonly used as components in other graphs, or something like that, but I guess HarriesGraph
probably doesn't have that property, right? Etc. etc.
I think it makes more sense to separate graphs generators into ones that define families of graphs and one that define single graphs. All four of the graphs in this patch would fall into the second category.
By the way, PEP 8 recommends putting no spaces around "=" when defining default arguments in function definitions:
def f(x, y, z=42):
not
def f(x, y, z = 42):
comment:12 in reply to: ↑ 11 Changed 10 years ago by
About the order, I didn't mean about your patch specifically (though your patch is also not following alphabetic order - D comes after C! :P ) - I mean the whole file. I mean, why is
HarriesGraph
at the top of the file butDoubleStarSnark
is under "Named Graphs"? It seems like the top of the file was originally for small graphs that are commonly used as components in other graphs, or something like that, but I guessHarriesGraph
probably doesn't have that property, right? Etc. etc.
Oh, right... When I want to add a function I usually do a search thrpugh the file to find the definition of the previous function, and I add mine afterwards. But honestly I do not think that not having them sorted really is a problem. Python sure does not mine and you find the line you are looking for through a text search, and not by scrolling through the file sooooo ^^;
By the way, PEP 8 recommends putting no spaces around "=" when defining default arguments in function definitions:
Ahah. Lawyers. Well, on my side I do not mind as long as the code compiles and is somehow readable, I will try to conform a bit more to this fanciful "PEP8" when what it says makes some sense, and you are free to change my code however you like -- you may follow these weird rules but your code is nice in the end, so why not ? :-)
Nathann
comment:13 Changed 10 years ago by
- Status changed from needs_review to positive_review
Just saying, Sage code is supposed to conform to PEP 8.
So I guess this means you are OK with my review patch? Positive review, then!
comment:14 Changed 10 years ago by
Yepyep, no prob. Thank you very much for your reviews ! :-)
Nathann
comment:15 Changed 10 years ago by
Sure, thank you for your patches! :D
comment:16 Changed 10 years ago by
- Merged in set to sage-5.1.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
Let's keep this in a queue with #12942 and #12945. At least #12942 is definitely needed as a dependency, and #12945 modified
_circle_embedding
as well.