#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)
Change History (10)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- 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
comment:3 Changed 11 years ago by
- 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
- 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
tooptional -- 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.
comment:5 Changed 11 years ago by
- 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
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
- 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
- Cc sage-combinat added
Two minor comments:
(1) I'm confused by this docstring
combined with this behaviour:
D is an incidence structure but the way of creating D seems slightly nonstandard. Why not
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).