Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#4859 closed enhancement (fixed)

[with patch, positive review] basic covering design module

Reported by: dgordon Owned by: mhansen
Priority: major Milestone: sage-3.3
Component: combinatorics Keywords:
Cc: sage-combinat Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

I maintain a database of covering designs (k-subsets of a v-set such that all t-sets are in at least one of the k-sets) at http://www.ccrwest.org/cover.html. This patch implements covering designs in Sage using the IncidenceStructure? class, and allows a user to get coverings from the website.

Attachments (2)

11113.patch (12.1 KB) - added by dgordon 11 years ago.
trac_4859-covering-designs2.patch (14.2 KB) - added by dgordon 11 years ago.
fix to problems in first patch pointed out by the reviewers

Download all attachments as: .zip

Change History (10)

Changed 11 years ago by dgordon

comment:1 Changed 11 years ago by mabshoff

  • Milestone set to sage-3.4
  • Summary changed from basic covering design module to [with patch, needs review] basic covering design module

comment:2 Changed 11 years ago by wdj

Two minor comments:

(1) I'm confused by this docstring

A $(v,k,t)$ covering design $C$ is an incidence structure ...

combined with this behaviour:

sage: C = best_known_covering_design_www(7, 3, 2); C
<class 'sage.combinat.designs.covering_design.CoveringDesign'>
sage: D = C.incidence_structure
sage: type(D)
<class 'sage.combinat.designs.incidence_structures.IncidenceStructure'>

D is an incidence structure but the way of creating D seems slightly nonstandard. Why not

sage: D = C.incidence_structure()

instead? I'm not saying change this, just wondering why it is the way it is.

(2) For future reference, the patch naming is slightly non-standard (trac_4859-covering-designs.patch or something like that is more typical).

comment:3 Changed 11 years ago by wdj

  • Summary changed from [with patch, needs review] basic covering design module to [with patch, needs work] basic covering design module

Got an email from Dan that he plans to go some minor modifications, after while I'll give this a positive review.

comment:4 Changed 11 years ago by was

  • Milestone changed from sage-3.4.1 to sage-3.3

REFEREE REPORT:

The overall style of the code looks really good.

  • This worries me:
            82	    bound = 1.0 
     	83	    for i in range(t-1,-1,-1): 
     	84	        bound = (bound*RDF(v-i)/RDF(k-i)).ceiling() 
     	85	 
     	86	    return bound 
    

I'm worried about overflow. Maybe using interval arithmetic would be better, i.e., the real interval field. You could make a comment about why precision isn't an issue, but I sort of wonder.

  • I think it might be nice to avoid confusion if you make all the attributes private (i.e., put an underscore (or two) in the beginning of their names), then have methods to access them, e.g.,
    This is bad:
    sage:  C=CoveringDesign(7,3,2,7,range(7),[[0, 1, 2], [0, 3, 4], [0, 5, 6], [1, 3, 5], [1, 4, 6], [2, 3, 6], [2, 4, 5]],0, 'Projective Plane')
    sage: C.method
    'Projective Plane'
    sage: C.method = 'foo bar'
    sage: C.method
    'foo bar'
    
    This is better:
    sage: C.method()
    'Projective Plane'
    sage: C.method?
    lots of nice documentation about what the C.__method *means* and how to use it. 
    

Do the same with all the other attributes. This is for consistency with the rest of Sage, and because it is easier for users.

  • You defined a show method. This is reserved for graphical display. Instead call that method _repr_, so it gets automatically picked up by the print and str(...) methods. Also _repr_ should return a string instead of using the print command.
  • change requires internet, optional to optional -- requires internet. Doing that isn't documented anywhere, but it's the "new way" now that I wrote an optional doctesting framework that really singles out various components.

With the above issues addressed, this code should go into sage.

Changed 11 years ago by dgordon

fix to problems in first patch pointed out by the reviewers

comment:5 Changed 11 years ago by wdj

  • Summary changed from [with patch, needs work] basic covering design module to [with patch, positive review] basic covering design module

Both patches (in order) apply cleanly to 3.3.alpha1. They pass sage -t and sage -t -optional.

Looks good to me.

comment:6 Changed 11 years ago by was

MABSHOFF -- when you apply this patch, make sure to add Dan Gordon as an official Sage developer somehow to the list.

comment:7 Changed 11 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged both patches into Sage 3.3.alpha3.

Cheers,

Michael

comment:8 Changed 11 years ago by nthiery

  • Cc sage-combinat added
Note: See TracTickets for help on using tickets.