Access to GRDB Fano polytopes

[sjg10]

This patch introduces the following functions-

CanonicalFano                   SmoothFano
ReflexiveFano                   TerminalFano
PolytopeSmallPolygon            lReflexive
LDP

These read from the data stored in this spkg:​http://coates.ma.ic.ac.uk/grdb_polytopes-0.1.spkg by using the new private function _GRDBBlockReader to access classifications of the above Fano polytopes originally stored on the ​Graded Ring Database and return LatticePolytope instances.

The implementation is similar that of #13115 where we provide a module lattice_polytopes to act as a namespace for these functions which are situated elsewhere, giving the option to concentrate all future examples of lattice polytopes here.

Install the latest SPKG above and the latest patch below.

[sjg10]

Adds access to the GRDB

[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.

[tomc]

Replying to novoselt:

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?

This is of course possible. But we certainly want to retain the function

PolytopeSmoothFano?(n)

so that we can iterate over smooth Fano polytopes, regardless of dimension. So your suggestion would mean that this function would take two forms:

PolytopeSmoothFano(dim=3,id=n)

and:

PolytopeSmoothFano(id=m)

(Note that, for a given polytope, n and m are different here.) I think the this option is clunkier, but if you prefer it then we could do that instead.

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.

I disagree. One of the things that I find difficult in Sage as it stands is that you need to know (or guess) the names of object factories before you can find the object-construction functions using TAB completion. Hanging all of these functions off lattice_polytope seems logical; it is certainly the first place that I would look for them.

I concur with the rest of your comments.

[novoselt]

I personally find it a bit confusing to enumerate polytopes in all dimensions as a single sequence and for iterating though all of them it seems more natural to have functions that take only dimension and return the sequence of all polytopes of that dimension. If you really feel that your second one-argument form should work, I still prefer it to a bunch of functions with trailing dimensions.

As for naming conventions:

• Accessing stuff through module names or factories isn't much different from the point of view "need to know their names" to use them, factories actually tend to have shorter/more natural names.
• lattice_polytope.PolytopeSmoothFano repeats "polytope" twice and looks strange.
• I am not sure why do we have lattice_polytope imported in the global namespace at all. William has done it many years ago when I provided just the original module, not a patch. According to current situation, it does not seem to me that it should be imported itself - only some of its classes/functions directly.
• My original name suggestion was not very thought through, it should be rather Fano_polytopes_grdb or just polytopes_grdb (or with GRDB) and imported in the global name space so that it appears in TAB completion for Fano or polytope, just as there is now polytopes database. This is more logical than lattice_polytope for those who don't know module hierarchy.

Volker, what are your thoughts?

[novoselt]

On a related note, groups. discussion: ​https://groups.google.com/d/topic/sage-devel/yrCeYitwdxE/discussion

[sjg10]

Access to GRDB via module lattice.polytopes

[sjg10]

added recognition of MAGMA contribution

[tomc]

Moved patch to git; minor edits to code, mainly to make it more Pythonic; minor edits to docstrings, mainly to improve grammar; new SPKG with license details included and better install script; see ​http://coates.ma.ic.ac.uk/grdb_polytopes-0.1.spkg for the new SPKG.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​755c7e3

Moved patch to git; minor edits to code, mainly to make it more Pythonic; minor edits to docstrings, mainly to improve grammar; new SPKG with license details included and better install script; see [http://coates.ma.ic.ac.uk/grdb_polytopes-0.1.spkg] for the new SPKG.

[rws]

Patchbot fails:

--2014-05-04 08:27:25--  http://coates.ma.ic.ac.uk/grdb_polytopes-0.1.spkg
Resolving coates.ma.ic.ac.uk (coates.ma.ic.ac.uk)... 155.198.35.88
Connecting to coates.ma.ic.ac.uk (coates.ma.ic.ac.uk)|155.198.35.88|:80... conn
ected.
HTTP request sent, awaiting response... 200 OK
Length: 74478 (73K) [text/plain]
Saving to: ‘/tmp/tmpKQrDny/grdb_polytopes-0.1.spkg’

     0K .                                                    100%  109K=0.7s

2014-05-04 08:27:26 (109 KB/s) - ‘/tmp/tmpKQrDny/grdb_polytopes-0.1.spkg’ saved
 [74478/74478]

abort: couldn't find mercurial libraries in [/usr/bin ...

[novoselt]

Note also that doctests will have to be adjusted due to #15240.

I'll try to finally get this reviewed during June Sage Days, if nobody beats me, of course.

[chapoton]

I have made the code pep8 compliant.

---

New commits:

​5db16ff	Merge branch 'u/tomc/ticket/13282' of ssh://trac.sagemath.org:22/sage into 13282
​37c562c	trac #13282 code is now pep8 compliant

[chapoton]

I got that:

tar: This does not look like a tar archive

gzip: stdin: not in gzip format
tar: Child returned status 1
tar: Error is not recoverable: exiting now
Error extracting ’canonical3’ database

[chapoton]

and moreover, when running sage after pulling the branch:

sage/geometry/lattice_polytopes_backend.py:20: DeprecationWarning: 
Importing SAGE_SHARE from here is deprecated. If you need to use it, please import it directly from sage.env
See http://trac.sagemath.org/17460 for details.
  GRDB_location = os.path.join(SAGE_SHARE, 'grdb_polytopes')

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​f93a39b	Merge branch 'public/ticket/13282' into 6.10.b0
​606bf78	trac #13282 fixing the import of SAGE_SHARE

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​b942f89

Merge branch 'public/ticket/13282' in 8.0.b5

[slelievre]

There is interest for this:

• ​Ask Sage question 53526: How to install spkg for GRDB Fano polytopes