DoubleStarSnark, FosterGraph, GrayGraph and HarriesGraph

[ncohen]

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

[kini]

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.

[kini]

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

[kini]

apply to $SAGE_ROOT/devel/sage

[kini]

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

[kini]

Patchbot: apply trac_12952.patch trac_12952.review.patch

[kini]

Patchbot gives this a green circle - or would, if it wasn't still only approving test results from 4.8...

[ncohen]

Helloooooooooo !!!

Oh, what about the order ? I thought I had them sorted in alphabetical order O_o

Nathann

[ncohen]

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 ^^;

[kini]

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

[ncohen]

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.

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

[kini]

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!

[ncohen]

Yepyep, no prob. Thank you very much for your reviews ! :-)

Nathann

[kini]

Sure, thank you for your patches! :D