#3943: [with patch, needs work] block designs patch
---------------------------+------------------------------------------------
Reporter: wdj | Owner: wdj
Type: enhancement | Status: new
Priority: major | Milestone: sage-3.1.3
Component: combinatorics | Resolution:
Keywords: |
---------------------------+------------------------------------------------
Comment (by rlm):
David,
Thank you for putting up with such a tedious review process. This new
patch is looking good. Here are a few comments I have on the new patch:
- I only recently learned this. When you have a block comment at the top
of the file, the first line is used as a subject heading for the reference
manual. Whatever that line is, is used for the title of that section in
the manual. So, something like `A module to help with constructions and
computations of block designs and other incidence structures.` as you have
it in block_design.py is better off as something more like `Block
Designs`.
- I still think that the class `IncidenceStructure` belongs somewhere else
than in `block_design.py`, although I'm not sure where it should go. I had
once thought that `sage.graphs` should be something like
`sage.incidence_structures`, and from there we would have designs, graphs,
hypergraphs, etc. Although, the naming is ugly, and this is probably not
the right approach. Any thoughts you have on this are welcome.
- There cannot be any new functions in any patch to be merged without
doctests, including auxiliary functions used only internally. In
particular, `tdesign_params`, `IncidenceStructure.__init__`,
`IncidenceStructure.__repr__`, `IncidenceStructure.blocks`,
`IncidenceStructure.blocks_from_gap`,
`IncidenceStructure._get_block_sizes`, and `IncidenceStructure.points` all
need doctests.
- Is there any particular reason you define `_get_incidence_matrix` and
`incidence_matrix`? The latter seems good enough, and there are a couple
functions that start with `_get`, and I don't see why.
- You have commented out the check in `BlockDesign` to see whether the
input satisfies the definition. I think this is a good idea, and should be
put back in. As it is right now, you can create an IncidenceStructure
which does not satisfy the definitions, and if you call `is_block_design`,
it will blindly return True, i.e. all IncidenceStrucures are taken to be
block designs. This is bad. Further, the code in the commented block calls
GAP, when in fact the axioms of a block design are easily checked, and
should be done in Sage. Also, would there be in the future any functions
which are particular to the block design assumption? If so you might
consider making `BlockDesign` a separate class which inherits from
`IncidenceStructure`.
- Have you run these doctests with `sage -t -optional`? I don't think you
have, since for example `gap.eval("IsBinaryBlockDesign("+gD+")")=="true"`
will surely fail because of the quotation marks.
- I'd really like to see this patch's dependence on GAP decrease more. One
task which would go a very far way in doing this would be to implement
#1323, which shouldn't really be all that hard, since it would ultimately
just be an exercise in linear algebra.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/3943#comment:21>
Sage <http://sagemath.org/>
Sage - Open Source Mathematical Software: Building the Car Instead of
Reinventing the Wheel
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en
-~----------~----~----~----~------~----~------~--~---
