Context Navigation

← Previous Ticket
Next Ticket →

#12945 closed enhancement (fixed)

Balaban 11-cage

Reported by:	kini	Owned by:	jason, ncohen, rlm
Priority:	major	Milestone:	sage-5.1
Component:	graph theory	Keywords:
Cc:	ncohen	Merged in:	sage-5.1.beta1
Authors:	Keshav Kini, Nathann Cohen	Reviewers:	Nathann Cohen, Keshav Kini
Report Upstream:	N/A	Work issues:
Branch:		Commit:
Dependencies:	#12942	Stopgaps:

Status badges

Description (last modified by kini)

Add the Balaban 11-cage to Sage's named graphs.

APPLY:

Attachments (4)

trac_12945-balaban-11-cage.patch (4.6 KB) - added by kini 10 years ago.: apply to $SAGE_ROOT/devel/sage
trac_12945-balaban-11-cage.rebased.patch (11.0 KB) - added by kini 10 years ago.: apply to $SAGE_ROOT/devel/sage
trac_12945-newembedding.patch (11.6 KB) - added by kini 10 years ago.: apply to $SAGE_ROOT/devel/sage
trac_12945-balaban-11-cage.2.patch (6.4 KB) - added by kini 10 years ago.: apply to $SAGE_ROOT/devel/sage

Download all attachments as: .zip

Change History (23)

Changed 10 years ago by kini

Attachment trac_12945-balaban-11-cage.patch added

apply to $SAGE_ROOT/devel/sage

comment:1 Changed 10 years ago by ncohen

Wow ! Nice embedding ! How did you find that ? :-)

Nathann

comment:2 Changed 10 years ago by kini

Here is an old patch which applies to Sage 4.7.1.rc1. It should be updated to apply to 5.0.rc1, and probably to use the helper function ncohen wrote for #12942 (hence the dependencies of this ticket).

The current layout of the graph in the patch is based on the drawing on page 9 of the Graph-Drawing Contest Report from the Fifth Annual Graph Drawing Contest. The drawing was made by Petrus Abri Santoso and Andi Surjanto and won first prize in Category C of the contest :)

Is there a better layout we should use?

Last edited 10 years ago by kini (previous) (diff)

comment:3 Changed 10 years ago by ncohen

I don't think so -- I've been spending the last 3 hours trying to build one :-)

Nathann

comment:4 Changed 10 years ago by kini

The patch turned out to be trivial to rebase - here's the rebased version. I also moved the Balaban 10-cage from #12942 to the top of the list of Named graphs, since "Balaban10Cage" (and "Balaban11Cage") comes before "BidiakisCube" in alphabetic order... forgot to catch this on #12942 :(

comment:5 Changed 10 years ago by ncohen

Description modified (diff)
Status changed from new to needs_review

Helloooooooooooo !!!

Ok, your patch was missing some documentation and doctests so I added some in this patch, with the best embedding I was able to build... Of course yours remains the default, I was not able to do better :-)

This patch applies on top of yours, so if you agree with those changes... ;-)

Nathann

comment:6 Changed 10 years ago by kini

Awesome! I'll add another patch to fix up some minor stuff. After that I guess it would be best if a third person could review this since we are both basically authors - though I guess it's also possible that I review your part and you review my part, haha.

comment:7 follow-up: ↓ 8 Changed 10 years ago by kini

Authors set to Keshav Kini, Nathann Cohen
Description modified (diff)

Patchbot: apply trac_12945-balaban-11-cage.rebased.patch trac_12945-newembedding.patch trac_12945-balaban-11-cage.2.patch

OK, so one more patch is up now. If you're OK with saying that we are reviewing each other, then we can set this to positive review. Otherwise we can try to find a neutral third party to review this. (Maybe there are some people on IRC...)

comment:8 in reply to: ↑ 7 Changed 10 years ago by ncohen

OK, so one more patch is up now. If you're OK with saying that we are reviewing each other, then we can set this to positive review. Otherwise we can try to find a neutral third party to review this. (Maybe there are some people on IRC...)

Well, I would say that as long as all tests pass (which is the case) and the documentation is nicely built (which is the case too)... If someone is available on IRC it may be better, but otherwise I think this ticket can go :-)

Nathann

comment:9 Changed 10 years ago by kini

ppurka on IRC made a good point - we should probably test the ValueErrors? as well...

comment:10 Changed 10 years ago by ncohen

Done in the new version of my patch. I also refreshed yours so that it still applied on top of it.

Nathann

comment:11 Changed 10 years ago by kini

:( I just did this with my .2.patch ...

So what did you do with that patch? It won't apply on top of this now without breaking doctests. Can we just keep your old newembedding.patch and my new .2.patch?