Opened 8 years ago

Closed 8 years ago

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

Attachments (4)

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

Download all attachments as: .zip

Change History (23)

Changed 8 years ago by kini

apply to $SAGE_ROOT/devel/sage

comment:1 Changed 8 years ago by ncohen

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

Nathann

comment:2 Changed 8 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 8 years ago by kini (previous) (diff)

comment:3 Changed 8 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 8 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 8 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 8 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: Changed 8 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 8 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 8 years ago by kini

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

comment:10 Changed 8 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 8 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?

comment:12 Changed 8 years ago by kini

Oh, never mind, you uploaded the refresh .2.patch too. Let's see...

comment:13 Changed 8 years ago by kini

Yes, you forgot to update the doctests you added, since I changed the ValueError? messages in .2.patch :)

comment:14 Changed 8 years ago by kini

I'll just upload my patches, I've tested that everything is working.

Changed 8 years ago by kini

apply to $SAGE_ROOT/devel/sage

Changed 8 years ago by kini

apply to $SAGE_ROOT/devel/sage

Changed 8 years ago by kini

apply to $SAGE_ROOT/devel/sage

comment:15 Changed 8 years ago by kini

OK, confirmed again that tests pass. Positive review?

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

comment:16 Changed 8 years ago by ncohen

  • Status changed from needs_review to positive_review

Yep ! All tests pass, the doc is nice, thank you for your help ! :-)

Nathann

comment:17 Changed 8 years ago by kini

  • Reviewers set to Nathann Cohen, Keshav Kini

comment:18 Changed 8 years ago by kini

And thank you for your help too :D

comment:19 Changed 8 years ago by jdemeyer

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