Opened 10 years ago

Closed 10 years ago

#6823 closed enhancement (fixed)

[with patch, positive review] Kneser Graph in graph_generators

Reported by: ncohen Owned by: rlm
Priority: major Milestone: sage-4.2
Component: graph theory Keywords: graph generators kneser
Cc: Merged in: sage-4.2.alpha0
Authors: Nathann Cohen Reviewers: Rob Beezer
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Kneser graphs for graph_generators ( http://en.wikipedia.org/wiki/Kneser_graph )

I just define the new function graphs.KneserGraph?()

Attachments (3)

knesergraph.patch (1.8 KB) - added by ncohen 10 years ago.
trac_6823.patch (4.5 KB) - added by ncohen 10 years ago.
trac_6823_reviewer.patch (767 bytes) - added by rbeezer 10 years ago.
Reviewer patch to set odd graph name

Download all attachments as: .zip

Change History (9)

Changed 10 years ago by ncohen

comment:1 Changed 10 years ago by rbeezer

  • Keywords graph generators kneser added
  • Summary changed from [with patch, needs review] Kneser Graph in graph_generators to [with patch, needs work] Kneser Graph in graph_generators

Hi Nathann,

This will be a nice addition to the graph generators. Some suggestions:

  1. How about giving the graph a name with the parameters in the string, like "Kneser graph with parameters 5 and 2"?
  1. The patch just seems to be a diff file, so it really should be a Mercurial patch with your name/email and a one-line comment. Patch files now seem to uniformly have filenames that begin with "trac_xxxx_" and you should put the trac number in the one-line summary of the Mercurial patch.
  1. I'd really like to see more robust handling of the inputs (and doctests to see that they work). For example, a negative n will bomb in the subset code. How about checking that n >= 0 and then that 0 <= k <= n?
  1. English is very good, but I'd suggest "adjacent" or "joined" instead of "linked" and "with parameters" instead of "of parameters" (three places).

With this completed, it'll be easy to add the Odd graphs - just Kneser graphs with n=2k+1.

This passes all tests in sage/graphs and the documentation builds fine.

Rob

comment:2 Changed 10 years ago by ncohen

  • Summary changed from [with patch, needs work] Kneser Graph in graph_generators to [with patch, needs review] Kneser Graph in graph_generators

New patch. Odds graphs are added, and with some luck each one of your remarks will find an answer in this new version. Hope you'll like it ! :-)

Nathann

Changed 10 years ago by ncohen

comment:3 Changed 10 years ago by ncohen

New patch taking into account the comments from #6828

Changed 10 years ago by rbeezer

Reviewer patch to set odd graph name

comment:4 Changed 10 years ago by rbeezer

  • Authors set to Nathann Cohen
  • Reviewers set to Rob Beezer

Nathann,

Looks very good, builds on 4.1.2.alpha2, passes all tests, etc.

Right now the name of an odd graph reports the Kneser graph parameters, etc. I'd expect this to confuse someone who builds an odd graph, yet does not know the connection to the Kneser graphs. I've attached a small patch that just sets the name on the odd graph routine. If you agree with the change, then you can mark the ticket as positive review. In other words, you can review my additional patch, and we'll be done.

Thanks, Rob

comment:5 Changed 10 years ago by ncohen

  • Summary changed from [with patch, needs review] Kneser Graph in graph_generators to [with patch, positive review] Kneser Graph in graph_generators

Good thinking ! ;-)

Nathann

comment:6 Changed 10 years ago by mhansen

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