Opened 5 years ago

Closed 5 years ago

#16433 closed defect (fixed)

clique_with_loops.is_strongly_regular() is True

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.3
Component: graph theory Keywords:
Cc: vdelecroix Merged in:
Authors: Nathann Cohen Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 4e717d3 (Commits) Commit: 4e717d3bb11a562c260a34f6031a0def3bec162d
Dependencies: Stopgaps:

Description

In #16370 Vincent reported the following

sage: BIBD = designs.BalancedIncompleteBlockDesign(31,6)
sage: V = map(Set, BIBD.blocks())
sage: G = Graph([V, lambda x,y: x&y])
sage: G.is_strongly_regular(parameters=True)
(31, 32, 31, -1)

The parameters should always be positive, and this bug comes from the fact that G is a clique with loops. And well, strongly regular graphs are assumed to be simple, so let's keep Sage's code simple without trying to generalize definitions :-P

Nathann

Change History (12)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/16433
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to ea1f5b8888379d2a587b995c04b6bd34bb70ecf5

Branch pushed to git repo; I updated commit sha1. New commits:

ea1f5b8trac #16433: clique_with_loops.is_strongly_regular() is True

comment:3 follow-up: Changed 5 years ago by vdelecroix

Hi Nathann,

Nice fix! I added docs. See u/vdelecroix/16433. Have a look and put positive review if you like.

Vincent

comment:4 in reply to: ↑ 3 ; follow-up: Changed 5 years ago by ncohen

Yo !

Nice fix! I added docs. See u/vdelecroix/16433. Have a look and put positive review if you like.

Sounds a bit counter-productive to add such comments here while we implement OrthogonalArrayGraphs just near. Don't you prefer to have this patch depend on #16370 so that you can link to the doc of OrthogonalArrayGraph (with stuff like a seealso block) ? By the way we have a LOT of strongly reguar graphs in Sage (including families), so unless you want to make a list of them these comments do not make much sense to me... The doc about BIBD and OA graphs really belong to the doc of those functions, not to the doc of this method.

What we should do perhaps is begin to index the SRG we can build somewhere. But I don't know how to do this properly atm...

So it's up to you. If you insist on adding this I don't mind.

Nathann

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by vdelecroix

Hi,

Replying to ncohen:

Yo !

Nice fix! I added docs. See u/vdelecroix/16433. Have a look and put positive review if you like.

Sounds a bit counter-productive to add such comments here while we implement OrthogonalArrayGraphs just near. Don't you prefer to have this patch depend on #16370 so that you can link to the doc of OrthogonalArrayGraph (with stuff like a seealso block) ? By the way we have a LOT of strongly reguar graphs in Sage (including families), so unless you want to make a list of them these comments do not make much sense to me... The doc about BIBD and OA graphs really belong to the doc of those functions, not to the doc of this method.

I do know nothing about strongly regular graphs, so I was very enthusiastic of having this one. But you are right it does not make much sense.

What we should do perhaps is begin to index the SRG we can build somewhere. But I don't know how to do this properly atm...

Ok. Just keep this in mind.

So it's up to you. If you insist on adding this I don't mind.

Just add the doctest about the errors (there are at the very end of the doc in the commit). I do not mind if you do a copy paste and keep it in your branch.

Vincent

comment:6 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by ncohen

Yo !

I do know nothing about strongly regular graphs, so I was very enthusiastic of having this one. But you are right it does not make much sense.

What about your branch then ?... Do you update it ?

What we should do perhaps is begin to index the SRG we can build somewhere. But I don't know how to do this properly atm...

Ok. Just keep this in mind.

Of course I keep this in mind. I have been implementing families for this only... http://trac.sagemath.org/ticket/14631 http://trac.sagemath.org/ticket/16362

Just add the doctest about the errors (there are at the very end of the doc in the commit). I do not mind if you do a copy paste and keep it in your branch.

Okay Okay so I update your branch ...

Nathann

comment:7 in reply to: ↑ 6 Changed 5 years ago by ncohen

Okay, turns out I cannot add a reference to the OA graphs because the ticket may be closed as wontfix. So I do what I can.

Nathann

comment:8 Changed 5 years ago by git

  • Commit changed from ea1f5b8888379d2a587b995c04b6bd34bb70ecf5 to 4e717d3bb11a562c260a34f6031a0def3bec162d

Branch pushed to git repo; I updated commit sha1. New commits:

4e717d3trac #16433: more doc

comment:9 Changed 5 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

Perfect. Thanks.

comment:10 follow-up: Changed 5 years ago by vdelecroix

How did you manage to make me the author of the last commit ???

comment:11 in reply to: ↑ 10 Changed 5 years ago by ncohen

How did you manage to make me the author of the last commit ???

When you run git commit --amend instead of git commit you do not create a new commit but add your changes to the last commit. I just did that. I does not change the author's name.

You can also set the author manually when you do a commit btw.

Nathann

comment:12 Changed 5 years ago by vbraun

  • Branch changed from u/ncohen/16433 to 4e717d3bb11a562c260a34f6031a0def3bec162d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.