#13282: Access to GRDB Fano polytopes
----------------------------------+-----------------------------------------
Reporter: sjg10 | Owner: mhampton
Type: enhancement | Status: new
Priority: major | Milestone: sage-5.3
Component: geometry | Resolution:
Keywords: | Work issues:
Report Upstream: N/A | Reviewers:
Authors: Samuel Gonshaw | Merged in:
Dependencies: | Stopgaps:
----------------------------------+-----------------------------------------
Comment (by novoselt):
A few global comments:
1. Why are there separate functions for different dimensions of the same
stuff? Why not have a single one that will take dimension as another
argument? If you really want to have functions with trailing dimension,
you can have one "actual function" and then a bunch of shortcuts that will
just add this trailing dimension to the list of parameters. I am, however,
against such name clutter - instead of 16 methods it seems to be possible
to have only 5 showing up in TAB-completion.
2. I think it would actually make sense to package everything into a
factory class with an instance, say `grdb_Fano_polytopes`, and access like
`grdb_Fano_polytopes.terminal(dim, n)`. (See e.g. Volker's toric variety
library for how it can be done.) Sage is not matching Magma or any other
system in naming conventions, and I think that such an approach would be
more in line with current constructors of different objects.
3. There is packing/unpacking of input in every function - it would be
better to have some helper functions doing it instead of duplicating the
code again and again.
4. There should be no commented functions. If you suppress vertex
computation, construction of a `LatticePolytope` does not invoke PALP and
so should work fine. It should then be possible to convert it to
`Polyhedron` even now and still do something. So all dimensions should be
allowed and the documentation may mention the limitations.
5. There is a big list of numbers in the patch - can it be read from the
package of polytopes instead of being hard-coded?
Small picks at docstring formatting, e.g. at
{{{
r"""
Returns ``n``-th smooth Fano polytope from the database
of 4-dimensional smooth Fano polytopes.
``n`` must be an integer or list of integers each of
which is in the range 1 to 124
See :func:`sage.geometry.lattice_polytope.PolytopeSmoothFano`
EXAMPLES:
::
sage: S = lattice_polytope.PolytopeSmoothFanoDim4(7) # optional -
GRDB_polytopes
"""
}}}
1. The starting "one-liner" should be a one line if at all possible. It
can be shortened here to "Return the ``n``-th smooth Fano polytope." With
clarification of the used order in the rest of the documentation.
2. There is no INPUT/OUPUT block.
3. Missing punctuation to make complete sentences.
4. I think that reference to the other (main) function should be expanded
to indicate that there is a description of the database there.
5. If you write `EXAMPLES::`, there is no need in dangling `::`.
6. What does this example demonstrate? Just that the line has finished?
For understanding of what was returned and testing that the returned value
is correct, would be nice to have the output of the function printed,
together with the vertices. And if there are two input formats, then I
think that both have to be tested. Note that this work is easier, if there
are no functions with trailing dimensions ;-)
7. In places where there are INPUT blocks, the formatting looks
suspicious. Did you try to compile the documentation and check that
everything looks OK?
It would be nice to also have definitions of all these polytopes in the
documentation of the functions, i.e. not just that it is the n-th polytope
from that database, but what does it mean mathematically that it is
terminal etc.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13282#comment:1>
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 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.
