Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Odd Girth #8952

Closed
nathanncohen mannequin opened this issue May 12, 2010 · 46 comments
Closed

Odd Girth #8952

nathanncohen mannequin opened this issue May 12, 2010 · 46 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented May 12, 2010

Add a function to compute odd girth of a graph G.

Apply:

  1. attachment: trac_8952_odd_girth_consolidated.patch
  2. attachment: trac_8952_odd_girth-bugfix.patch

CC: @rbeezer

Component: graph theory

Author: Jernej Azarija

Reviewer: Rob Beezer, Nathann Cohen

Merged: sage-5.6.beta0

Issue created by migration from https://trac.sagemath.org/ticket/8952

@nathanncohen nathanncohen mannequin added this to the sage-5.6 milestone May 12, 2010
@nathanncohen nathanncohen mannequin added the s: needs work label Jun 6, 2010
@kini
Copy link
Contributor

kini commented Sep 25, 2012

comment:2

What needs work here exactly? There's no patch on this ticket.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 25, 2012

comment:3

Well, nothing actually. I used to use the Trac server as my todo-list, a loooong time ago, so you will find many such tickets in this section ^^;

I am (desperately) looking for a flat in Paris right now, but I should stop sinking and start swimming in a couple of weeks.

I hope that all is well on you side !! :-)

Nathann

@kini
Copy link
Contributor

kini commented Sep 25, 2012

comment:4

Haha, OK :) I found this ticket because someone asked about it on IRC and wants to work on it. Good luck with your flat!

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Sep 25, 2012

Attachment: trac_8952_odd_girth.patch.gz

Patch performing the ticket request

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Sep 25, 2012

comment:5

Hello!

Attached is the patch that should perform the required task. odd_girth() computes the odd girth of a graph using a property of the characteristic polynomial of the adjacency matrix. Its (theoretic) computational complexity is optimal.

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Sep 26, 2012

comment:6

Azi,

I took a quick look and it looks promising, especially since characteristic polynomials are quite fast in Sage. I can look closer when I have a bit more time.

For now, documentation needs some work. For example:

Any complete graph on more than 2 vertices contains a triangle and has thus odd girth 3 

	            G = graphs.CompleteGraph(10) 
	            sage: G.odd_girth() 
	            3

needs a double colon after the lead-in sentence (all 3 of your verbatim blocks need this). And your line creating the complete graph needs a sage: preceding it.

Current documentation style needs "INPUT" and "OUTPUT" blocks - look around for examples. They will be pretty simple in this case.

You can catch some of these yourself:

sage -b
sage -docbuild reference html

will rebuild the documentation and you can view the html file for mess-ups with the doc string.

And

sage -t <source file>

will find broken tests.

Rob

@rbeezer rbeezer mannequin added s: needs work and removed s: needs review labels Sep 26, 2012
@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Sep 26, 2012

Second patch implementing the requested comments

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Sep 26, 2012

comment:8

Attachment: trac_8952_odd_girth_second.patch.gz

I have added a patch (hopefully valid) with the commends you requested. There is still one missing warning in building the documentation (Literal block expected; none found." is likely an indentiation problem and/or a problem with a double-colon) which I was not able to spot.

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Sep 26, 2012

comment:10

Dear Azi,

Your "literal block expected" error is caused by a double-colon after "EXAMPLES". It should be a single, just to be typeset as-is. A double-colon says verbatim (literal) stuff comes next.

The patch looks severely messed-up to me. ;-( I am not all sure how you got it to that state, but I think I can fix it. Are you using clones or Mercurial queues? Queues are much easier to deal with, and easier for iterative changes like this.

Don't try to fix the double-colon until I fix the patch (maybe later today).

Rob

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Sep 26, 2012

comment:11

The patch was produced using the following guide http://www.sagemath.org/doc/developer/walk_through.html#creating-a-change

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Sep 26, 2012

comment:12

Replying to @sagetrac-azi:

The patch was produced using the following guide http://www.sagemath.org/doc/developer/walk_through.html#creating-a-change

Well, I sort of hope not, since I wrote that guide. ;-) It looks like something wasn't done quite right.

Guide discusses two approaches: clones and queues. I cannot even begin to help if I don't know which approach you are taking. Let me know.

Rob

@sagetrac-azi

This comment has been minimized.

@rbeezer
Copy link
Mannequin

rbeezer mannequin commented Sep 30, 2012

comment:14

"consolidated" patch has previous work in one (proper) patch - still has azi's name on it, too.

See new "Apply" section in ticket description.

Azi -

  1. Start with a clean Sage installation (even if it means installing from source).
  2. Install a system version of Mercurial, or replace "hg" commands with "sage -hg"
  3. hg import (get URL from tiny download icon, not web-view of patch)
  4. hg qpush (to apply patch)
  5. Rebuild sage, edit source, make changes, etc, etc.
  6. hg qrefresh -m ""
  7. hg export qtip >
  8. hg qpop (to move changes out of the way, and get back to original Sage, rebuild, etc)

You can use "hg qnew " to initiate a new project with no interference from previous one. You can easily manage several activities with qpop/qpush, and "hg qpush --move" will allow applying patches out of order. "hg qapplied", "hg qunapplied" are informative. Hope this is helpful.

Rob

@rbeezer

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 1, 2012

comment:15

(same patch with the max 80 characters per line that Minh taught me to respect in the Graph files, some links between odd_girth and girth, and some modifications in the doc so that Sphinx gets what it should)

@nathanncohen

This comment has been minimized.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 2, 2012

comment:21

(just updated the patch so that is_odd_hole_free uses this function too !!!)

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Oct 27, 2012

comment:22

Attachment: trac_8952_odd_girth-bugfix.patch.gz

The patch looks good to me. Am I allowed to change the status of the ticket to reflect that?

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 27, 2012

comment:23

Well I guess you can if you think the patch is ready :-)

Nathann

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 5, 2012

comment:24

Ahem. Does everybody here agree that this patch should be merged ?

Nathann

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Nov 5, 2012

comment:25

Looks good to me!

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 5, 2012

comment:26

Ok, fine then... Could you set it to positive_review ? I'm the last one who added something to the ticket :-)

Nathann

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 17, 2012

comment:27

Okayyyyyyyyyyyyyyyyyy.................. :-P

Nathann

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Nov 17, 2012

comment:28

I am sorry. I completely overlooked the "set it to positive_review" part.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 17, 2012

comment:29

No problem. Could you add your name to the list of reviewers/authors ? And if possible to the list in "http://trac.sagemath.org/sage_trac/wiki" too :-)

Nathann

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Nov 17, 2012

comment:30

OK. I have added myself to the wiki link. But I can't find the list of reviewers/authors. Do you happen to know where do I find that?

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 17, 2012

comment:31

Ahahaah. It's on this page, on the "Modify Ticket" section. When a ticket is positively reviewed, the list of authors and reviewers has to be filled. I often forget that myself :-)

Nathann

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Nov 17, 2012

Reviewer: Jernej Azarija

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Nov 17, 2012

Author: Jernej Azarija

@sagetrac-azi
Copy link
Mannequin

sagetrac-azi mannequin commented Nov 17, 2012

comment:33

Oooh ok! I suppose you will add yourself to the list right?

@kini
Copy link
Contributor

kini commented Nov 17, 2012

Changed reviewer from Jernej Azarija to Nathann Cohen, Jernej Azarija

@kini
Copy link
Contributor

kini commented Nov 17, 2012

Changed author from Jernej Azarija to Jernej Azarija, Nathann Cohen

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 17, 2012

Changed author from Jernej Azarija, Nathann Cohen to Jernej Azarija

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 17, 2012

comment:35

Well, I'm not really an author of this ticket, and Rob did some of it too :-)

Nathann

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 17, 2012

Changed reviewer from Nathann Cohen, Jernej Azarija to Rob Beezer, Nathann Cohen, Jernej Azarija

@kini
Copy link
Contributor

kini commented Nov 17, 2012

comment:36

If azi is the only author, then he cannot be a reviewer...

@kini
Copy link
Contributor

kini commented Nov 17, 2012

Changed reviewer from Rob Beezer, Nathann Cohen, Jernej Azarija to Rob Beezer, Nathann Cohen

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Nov 17, 2012

comment:37

AHahah. If that's the law, then...:-)

Nathann

@jdemeyer
Copy link

Attachment: trac_8952_odd_girth_consolidated.patch.gz

@jdemeyer
Copy link

comment:38

Rebased to sage-5.5.rc0.

@jdemeyer
Copy link

Merged: sage-5.6.beta0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants