Opened 6 years ago

Closed 5 years ago

#15664 closed defect (fixed)

Bug in IncidenceStructure.is_block_design

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.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 ncohen)

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 ncohen

  • Cc chapoton Stefan wdj added
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by ncohen

  • Branch set to u/ncohen/15664

comment:3 Changed 6 years ago by ncohen

  • Status changed from needs_review to needs_work

comment:4 Changed 6 years ago by ncohen

  • Description modified (diff)

comment:5 Changed 6 years ago by ncohen

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:6 Changed 6 years ago by git

  • Commit set to e84f4ee14f61d2715c2c7449480fbe430427061e

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

comment:7 Changed 6 years ago by chapoton

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 git

  • Commit changed from e84f4ee14f61d2715c2c7449480fbe430427061e to 77c7d4477051fa26ba38031f920a4f9809686ae4

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

comment:9 Changed 6 years ago by ncohen

Fixed !

Nathann

comment:10 Changed 6 years ago by chapoton

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 chapoton

  • Reviewers set to Frédéric Chapoton

comment:12 Changed 6 years ago by git

  • Commit changed from 77c7d4477051fa26ba38031f920a4f9809686ae4 to 2aa7e1887472afaba33175bde385d45a0dd11cd8

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

comment:13 Changed 6 years ago by ncohen

  • Status changed from needs_review to positive_review

Done ! Thanks for the review ;-)

Nathann

comment:14 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:15 Changed 5 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.