Opened 6 years ago
Closed 6 years ago
#15664 closed defect (fixed)
Bug in IncidenceStructure.is_block_design
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.2 
Component:  combinatorics  Keywords:  
Cc:  chapoton, Stefan, wdj  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  u/ncohen/15664 (Commits)  Commit:  2aa7e1887472afaba33175bde385d45a0dd11cd8 
Dependencies:  #15285  Stopgaps: 
Description (last modified by )
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)
Change History (15)
comment:1 Changed 6 years ago by
 Cc chapoton Stefan wdj added
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Branch set to u/ncohen/15664
comment:3 Changed 6 years ago by
 Status changed from needs_review to needs_work
comment:4 Changed 6 years ago by
 Description modified (diff)
comment:5 Changed 6 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:6 Changed 6 years ago by
 Commit set to e84f4ee14f61d2715c2c7449480fbe430427061e
comment:7 Changed 6 years ago by
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.
comment:8 Changed 6 years ago by
 Commit changed from e84f4ee14f61d2715c2c7449480fbe430427061e to 77c7d4477051fa26ba38031f920a4f9809686ae4
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
ee6d412  Projective Plane designs constructor

363badb  trac 15107  reviewer's comments

9fcfb13  Rename the method from ProjectivePlaneDesign to DesarguesianProjectivePlaneDesign

cf71d58  Rebase on 5.13.beta0

2ec76c7  Bug in AffineGeometryDesign

2750b79  trac 15285: rebase on 6.1.beta4

e2935fe  Trac #15107: Rebase on 6.1.beta3

6f247f6  trac #15107: back to the first name with a new argument

8468131  trac #15285: Rebase on updated #15107

6ff6a06  trac #15285: Doc of AffineGeometryDesign

comment:9 Changed 6 years ago by
Fixed !
Nathann
comment:10 Changed 6 years ago by
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.
comment:11 Changed 6 years ago by
 Reviewers set to Frédéric Chapoton
comment:12 Changed 6 years ago by
 Commit changed from 77c7d4477051fa26ba38031f920a4f9809686ae4 to 2aa7e1887472afaba33175bde385d45a0dd11cd8
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
ee6d412  Projective Plane designs constructor

363badb  trac 15107  reviewer's comments

9fcfb13  Rename the method from ProjectivePlaneDesign to DesarguesianProjectivePlaneDesign

cf71d58  Rebase on 5.13.beta0

2ec76c7  Bug in AffineGeometryDesign

2750b79  trac 15285: rebase on 6.1.beta4

e2935fe  Trac #15107: Rebase on 6.1.beta3

6f247f6  trac #15107: back to the first name with a new argument

8468131  trac #15285: Rebase on updated #15107

6ff6a06  trac #15285: Doc of AffineGeometryDesign

comment:13 Changed 6 years ago by
 Status changed from needs_review to positive_review
Done ! Thanks for the review ;)
Nathann
comment:14 Changed 6 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:15 Changed 6 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
Projective Plane designs constructor
trac 15107  reviewer's comments
Rename the method from ProjectivePlaneDesign to DesarguesianProjectivePlaneDesign
Rebase on 5.13.beta0
Bug in AffineGeometryDesign
trac 15285: rebase on 6.1.beta4
Trac #15107: Rebase on 6.1.beta3
trac #15107: back to the first name with a new argument
trac #15285: Rebase on updated #15107
trac #15285: Doc of AffineGeometryDesign