Opened 10 years ago
Closed 10 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: |
Description (last modified by )
Add the Balaban 11-cage to Sage's named graphs.
APPLY:
Attachments (4)
Change History (23)
Changed 10 years ago by
comment:1 Changed 10 years ago by
Wow ! Nice embedding ! How did you find that ? :-)
Nathann
comment:2 Changed 10 years ago by
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?
comment:3 Changed 10 years ago by
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
comment:5 Changed 10 years ago by
- 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
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
- 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
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
ppurka on IRC made a good point - we should probably test the ValueErrors? as well...
comment:10 Changed 10 years ago by
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
:( 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 10 years ago by
Oh, never mind, you uploaded the refresh .2.patch too. Let's see...
comment:13 Changed 10 years ago by
Yes, you forgot to update the doctests you added, since I changed the ValueError? messages in .2.patch :)
comment:14 Changed 10 years ago by
I'll just upload my patches, I've tested that everything is working.
comment:15 Changed 10 years ago by
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 10 years ago by
- 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 10 years ago by
- Reviewers set to Nathann Cohen, Keshav Kini
comment:18 Changed 10 years ago by
And thank you for your help too :D
comment:19 Changed 10 years ago by
- Merged in set to sage-5.1.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
apply to $SAGE_ROOT/devel/sage