#16534: Basic Block design methods
-------------------------------------+-------------------------------------
Reporter: brett | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.4
Component: combinatorial | Resolution:
designs | Merged in:
Keywords: Block Design, | Reviewers:
Incidence Structure, Residual, | Work issues:
Derived, Complement, Supplement, | Commit:
Union | 0c935a3fddca65fea1534110c62ace88bc0b7296
Authors: Brett Stevens | Stopgaps:
Report Upstream: N/A |
Branch: u/brett/design |
Dependencies: #16553 |
-------------------------------------+-------------------------------------
Changes (by ncohen):
* status: needs_review => needs_work
Comment:
Hellooooooooooo again,
Some remarks follow... It is a bit long, which is one of the reasons why
we try
to keep the patches 'focused'.. Sorry for that ^^;
- 'union' does not appear in the index of methods
- A 'the' is missing several times, e.g.: "Return complementary incidence
structure."
- Sage's source code is not the right place to ask questions. What about
turning
this 'todo' into a trac ticket?
- It is not very efficient to look for an element in a list. You could do
the
following replacement for a better runtime:
{{{
#!diff
- if delete:
- int_points = frozenset(int(x) for x in self._points if x not in
points)
- else:
- int_points = frozenset(int(x) for x in points)
+ if delete:
+ int_points = frozenset(self._points).difference(points)
+ else:
+ int_points = frozenset(int(x) for x in points)
}}}
The same pattern appears several times.
- In the sentence {{{Specify whether the objective function is to maximize
the
cardinality of blocks in the packing}}}, could you say "is to maximize
the
number of blocks" instead? I believe that this could avoid confusion.
- The indentation of {{{+ # Maximum number of blocks }}} is not correct by
one
space.
- The following replacement also makes the code simpler:
{{{
- p.set_objective(p.sum([b[i]*len(self._blocks[i]) for i in
range(self.num_blocks())]))
+ p.set_objective(p.sum([b[i]*len(bi) for i,bi in
enumerate(self._blocks)]))
}}}
- We try to keep the LaTeX readable both in html form and in text form
(for
those who query the help from the command line) whenever possible. Could
you
replace "\mid" by "|" in your formulas? The latter is much easier to
decrypt.
- In `derived_incidence_structure` a "\\neq" is wrongly displayed in the
html
version of the doc.
- The documentation of the derived incidence structure could be made
easier to
read as
{{{
At **a point**: Given a [...]
At **a block**: Given a [...]
}}}
- Isn't the derived incidence structure "at a block" equal to the trace of
the
incidence structure on that block? Is only 'B' missing?
- "Otherwise there is no guarantee that the derived incidence structure at
a
block is a block design": I do not think that this is actually
necessary,
though it is not very important.
- Could you specify in the INPUT section of `derived_incidence_structure`
that
exactly one of `at_point` and `at_block` must be defined?
- The documentation of a function is no place to ask questions on its
desired
behaviour. Please specify in the documentation how it behaves on
multiple
blocks if that troubles you. You can use a '.. NOTE' environment.
- If neither `at_point` nor `at_block` is specified, I would say that the
correct behaviour is an exception. You can use the following pattern:
{{{
if (at_point is None) + (at_block is None) != 1:
raise ValueError("Exactly one of at_block and at_point must be
specified")
if at_point:
<code>
else:
<code>
}}}
- {{{if not(at_block in self.blocks()):}}}: this can be rewritten as
`at_block
not in self`. It is less wasteful, as it does not build the list of
blocks
only to throw it away afterwards. In general `not(a in b)` is I believe
equivalent to `a not in b`.
- `residual_incidence_structure`: the formulas do not appear correctly in
the
html doc, probably because of the '\\'.
- Could you specify in `residual_incidence_structure` and in
`derived_incidence_structure` where those definitions are taken?
- `supplementary_incidence_structure`: again, testing containment in a
list is
slow. Could you use a set instead? Then, containment tests involve
equality
test: `a==b` is fast when `a,b` are integers but the points of an
incidence
structure may be arbitrary objects and their equality tests may be
slower. It
is best to work on the integers directly.
- `complementary_incidence_structure`: the iterators from itertools are
usually
faster than their 'combinat' counterpart. It is probably faster to use
them:
{{{
sage: list(combinations([1,2,3,4],2))
[(1, 2), (1, 3), (1, 4), (2, 3), (2, 4), (3, 4)]
}}}
Here again, it is better to work on integers directly.
- Do you think that it could be useful to add a 'simple' option to
'union'? It
would return the union of the two structures while ignoring duplicated
blocks.
- It feels a bit weird to carry the class' name in its methods' names.
What
about renaming 'supplementary_incidence_structure' to 'supplement',
"residual_incidence_structure" to "residual", etc?..
Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/16534#comment:39>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
