Opened 8 years ago

Closed 8 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)

trac_12952.patch (10.2 KB) - added by ncohen 8 years ago.
trac_12952.review.patch (12.2 KB) - added by kini 8 years ago.
apply to $SAGE_ROOT/devel/sage

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by ncohen

comment:1 Changed 8 years ago by ncohen

  • Cc kini added
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by kini

  • Dependencies set to #12942, #12945

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.

comment:3 Changed 8 years ago by kini

  • Authors set to Nathann Cohen
  • Reviewers set to Keshav Kini

comment:4 Changed 8 years ago by 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 :)

Changed 8 years ago by kini

apply to $SAGE_ROOT/devel/sage

comment:5 follow-up: Changed 8 years ago by 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 :)

comment:6 Changed 8 years ago by kini

Patchbot: apply trac_12952.patch trac_12952.review.patch

comment:7 Changed 8 years ago by kini

  • Summary changed from DoubleStarSnar, FosterGraph, GrayGraph and HarriesGraph to DoubleStarSnark, FosterGraph, GrayGraph and HarriesGraph

comment:8 Changed 8 years ago by kini

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

comment:9 Changed 8 years ago by ncohen

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 8 years ago by 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 ^^;

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

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

comment:13 Changed 8 years ago by kini

  • 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 8 years ago by ncohen

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

Nathann

comment:15 Changed 8 years ago by kini

Sure, thank you for your patches! :D

comment:16 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.1.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.