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

Bug in IncidenceStructure.is_block_design #15664

Closed
nathanncohen mannequin opened this issue Jan 11, 2014 · 14 comments
Closed

Bug in IncidenceStructure.is_block_design #15664

nathanncohen mannequin opened this issue Jan 11, 2014 · 14 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Jan 11, 2014

This bug has been noticed by Frederic in #15285 :

sage: IncidenceStructure([0,1,2],[[0,1],[0,2],[1,2]]*15).is_block_design()
(True, [1, 3, 2, 15])
sage: IncidenceStructure([0,1,2],[[0,1],[0,2],[1,2]]*15).parameters()     
(2, 3, 2, 15)

Turns out that the code of is_block_design was rather unclear, and rather... Costly. Here is a simpler, faster and more correct version in which the bug in solved.

Before:

sage: D = IncidenceStructure(range(44),designs.steiner_quadruple_system(44))
sage: time D.is_block_design()                                              
CPU times: user 99.26 s, sys: 0.06 s, total: 99.32 s
Wall time: 99.40 s
(True, [3, 44, 4, 1])

After:

sage: D = IncidenceStructure(range(44),designs.steiner_quadruple_system(44))
sage: time D.is_block_design()                                              
CPU times: user 0.06 s, sys: 0.00 s, total: 0.06 s
Wall time: 0.06 s
(True, [3, 44, 4, 1])

Secondly, IncidenceStructure.parameter has a very tricky behaviour :

sage: D = IncidenceStructure(range(44),designs.steiner_quadruple_system(44))
sage: D.is_block_design()                                                   
(True, [3, 44, 4, 1])
sage: D.parameters()                                                        
(2, 44, 4, 21)

This is because parameters() take t as a parameter, and sets it to 2 by default. That's.... surprising :-P

This patch adds a deprecation warning when this parameter is not set, and we will make it mandatory... in a while.

Here it is !

Nathann

(this ticket took roughly 3hours of solid work)

Depends on #15285

CC: @fchapoton @sagetrac-Stefan @wdjoyner

Component: combinatorics

Author: Nathann Cohen

Branch/Commit: u/ncohen/15664 @ 2aa7e18

Reviewer: Frédéric Chapoton

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

@nathanncohen nathanncohen mannequin added this to the sage-6.1 milestone Jan 11, 2014
@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jan 11, 2014

Branch: u/ncohen/15664

@nathanncohen

This comment has been minimized.

@nathanncohen

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 11, 2014

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

ee6d412Projective Plane designs constructor
363badbtrac 15107 -- reviewer's comments
9fcfb13Rename the method from ProjectivePlaneDesign to DesarguesianProjectivePlaneDesign
cf71d58Rebase on 5.13.beta0
2ec76c7Bug in AffineGeometryDesign
2750b79trac 15285: rebase on 6.1.beta4
e2935feTrac #15107: Rebase on 6.1.beta3
6f247f6trac #15107: back to the first name with a new argument
8468131trac #15285: Rebase on updated #15107
6ff6a06trac #15285: Doc of AffineGeometryDesign

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 11, 2014

Commit: e84f4ee

@fchapoton
Copy link
Contributor

comment:7

Hello,

Nice job !

Do you still need to use self.blocks() instead of self.blcks ? You have replaced some of them, but there remains one.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 12, 2014

Changed commit from e84f4ee to 77c7d44

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 12, 2014

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

ee6d412Projective Plane designs constructor
363badbtrac 15107 -- reviewer's comments
9fcfb13Rename the method from ProjectivePlaneDesign to DesarguesianProjectivePlaneDesign
cf71d58Rebase on 5.13.beta0
2ec76c7Bug in AffineGeometryDesign
2750b79trac 15285: rebase on 6.1.beta4
e2935feTrac #15107: Rebase on 6.1.beta3
6f247f6trac #15107: back to the first name with a new argument
8468131trac #15285: Rebase on updated #15107
6ff6a06trac #15285: Doc of AffineGeometryDesign

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jan 12, 2014

comment:9

Fixed !

Nathann

@fchapoton
Copy link
Contributor

comment:10

Looks good to me.

One last detail : there lacks a space after "2" in "2is used when none is provided"

Then you can set a positive review.

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 14, 2014

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

ee6d412Projective Plane designs constructor
363badbtrac 15107 -- reviewer's comments
9fcfb13Rename the method from ProjectivePlaneDesign to DesarguesianProjectivePlaneDesign
cf71d58Rebase on 5.13.beta0
2ec76c7Bug in AffineGeometryDesign
2750b79trac 15285: rebase on 6.1.beta4
e2935feTrac #15107: Rebase on 6.1.beta3
6f247f6trac #15107: back to the first name with a new argument
8468131trac #15285: Rebase on updated #15107
6ff6a06trac #15285: Doc of AffineGeometryDesign

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 14, 2014

Changed commit from 77c7d44 to 2aa7e18

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Jan 14, 2014

comment:13

Done ! Thanks for the review ;-)

Nathann

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

2 participants