Access to GRDB Fano polytopes

401471b9-1fa8-4bce-9462-4f5c713a368b commented 12 years ago

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.

CC: @sagetrac-tomc @vbraun @novoselt @rbeezer

Component: geometry

Keywords: polytopes

Work Issues: address compile failure

Author: Samuel Gonshaw

Branch/Commit: public/ticket/13282 @ b942f89

Issue created by migration from https://trac.sagemath.org/ticket/13282

401471b9-1fa8-4bce-9462-4f5c713a368b commented 12 years ago

Adds access to the GRDB

novoselt commented 12 years ago

comment:1

Attachment: trac_13282_GRDB_polytopes_access.patch.gz

A few global comments:

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

"""

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.
There is no INPUT/OUPUT block.
Missing punctuation to make complete sentences.
I think that reference to the other (main) function should be expanded to indicate that there is a description of the database there.
If you write EXAMPLES::, there is no need in dangling ::.
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 ;-)
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.

bb51d1f9-3114-42b3-bf15-d218ca7784e2 commented 12 years ago

comment:2

Replying to @novoselt:

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.

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 commented 12 years ago

comment:3

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 commented 12 years ago

comment:4

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

401471b9-1fa8-4bce-9462-4f5c713a368b commented 12 years ago

Description changed:

--- 
+++ 
@@ -1,20 +1,15 @@
-This patch introduces the following functions into `sage.geometry.lattice_polytope`:
+This patch introduces the following functions:

-PolytopeCanonicalFanoDim2 PolytopeSmoothFanoDim2 -PolytopeCanonicalFanoDim3 PolytopeSmoothFanoDim3 -PolytopeReflexiveFanoDim2 PolytopeSmoothFanoDim4 -PolytopeReflexiveFanoDim3 PolytopeSmoothFanoDim5 -PolytopeTerminalFanoDim2 PolytopeSmoothFanoDim6 -PolytopeTerminalFanoDim3 PolytopeSmoothFanoDim7 -PolytopeSmallPolygon PolytopeSmoothFanoDim8 -PolytopeSmoothFano PolytopelReflexiveDim2 +CanonicalFano SmoothFano +ReflexiveFano TerminalFano +PolytopeSmallPolygon PolytopelReflexive +LDP


-These read from the data stored in this spkg:[http://coates.ma.ic.ac.uk/GRDB_polytopes-20120719.spkg](http://coates.ma.ic.ac.uk/GRDB_polytopes-20120719.spkg)
+These read from the data stored in this spkg:[http://coates.ma.ic.ac.uk/GRDB_polytopes-20120806.spkg](http://coates.ma.ic.ac.uk/GRDB_polytopes-20120806.spkg)
 by using the new private function `_GRDBBlockReader` to access classifications of the above Fano polytopes originally stored on the [Graded Ring Database](http://grdb.lboro.ac.uk) and return `LatticePolytope` instances.
-The interface mirrors the implementation in MAGMA.

-* The functions `PolytopeSmoothFanoDim6`, `PolytopeSmoothFanoDim7`, `PolytopeSmoothFanoDim8`  are commented out and access to `PolytopeSmoothFano` is limited as smooth Fanos of dimension 6 and up have too many facets for the PALP backend to currently deal with.
+The implementation is similar that of [#13115](https://github.com/sagemath/sage-prod/issues/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.

-* the functions are not imported to the top-level at runtime, but are left accessible as `lattice_polytope.*` to reduce clutter
+Install the latest SPKG above and the latest patch.

401471b9-1fa8-4bce-9462-4f5c713a368b commented 12 years ago

Attachment: trac13282_GRDB_polytopes_new_module2.patch.gz

Access to GRDB via module lattice.polytopes

401471b9-1fa8-4bce-9462-4f5c713a368b commented 12 years ago

Description changed:

--- 
+++ 
@@ -3,7 +3,7 @@

CanonicalFano SmoothFano ReflexiveFano TerminalFano -PolytopeSmallPolygon PolytopelReflexive +PolytopeSmallPolygon lReflexive LDP


@@ -12,4 +12,4 @@

 The implementation is similar that of [#13115](https://github.com/sagemath/sage-prod/issues/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.
+Install the latest SPKG above and the latest patch below.

401471b9-1fa8-4bce-9462-4f5c713a368b commented 11 years ago

added recognition of MAGMA contribution

jdemeyer commented 11 years ago

comment:8

Attachment: trac13282_GRDB_polytopes_new_module3.patch.gz

bb51d1f9-3114-42b3-bf15-d218ca7784e2 commented 10 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-This patch introduces the following functions:
+This patch introduces the following functions-

CanonicalFano SmoothFano @@ -7,7 +7,7 @@ LDP


-These read from the data stored in this spkg:[http://coates.ma.ic.ac.uk/GRDB_polytopes-20120806.spkg](http://coates.ma.ic.ac.uk/GRDB_polytopes-20120806.spkg)
+These read from the data stored in this spkg:[http://coates.ma.ic.ac.uk/grdb_polytopes-0.1.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](http://grdb.lboro.ac.uk) and return `LatticePolytope` instances.

 The implementation is similar that of [#13115](https://github.com/sagemath/sage-prod/issues/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.

bb51d1f9-3114-42b3-bf15-d218ca7784e2 commented 10 years ago

Branch: u/tomc/ticket/13282

bb51d1f9-3114-42b3-bf15-d218ca7784e2 commented 10 years ago

comment:12

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

Commit: 755c7e3

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 10 years ago

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](http://coates.ma.ic.ac.uk/grdb_polytopes-0.1.spkg) for the new SPKG.

fchapoton commented 10 years ago

Changed keywords from none to polytopes

rwst commented 10 years ago

Work Issues: address compile failure

rwst commented 10 years ago

comment:15

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 commented 10 years ago

comment:16

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.

fchapoton commented 10 years ago

comment:18

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`

fchapoton commented 10 years ago

Changed branch from u/tomc/ticket/13282 to public/ticket/13282

fchapoton commented 10 years ago

Changed commit from 755c7e3 to 37c562c

fchapoton commented 9 years ago

Description changed:

--- 
+++ 
@@ -10,6 +10,6 @@
 These read from the data stored in this spkg:[http://coates.ma.ic.ac.uk/grdb_polytopes-0.1.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](http://grdb.lboro.ac.uk) and return `LatticePolytope` instances.

-The implementation is similar that of [#13115](https://github.com/sagemath/sage-prod/issues/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.
+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.

fchapoton commented 9 years ago

comment:22

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

fchapoton commented 9 years ago

comment:23

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')

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 37c562c to 606bf78

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 606bf78 to b942f89

slel commented 3 years ago

comment:27

There is interest for this:

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

sagemath / sage

Access to GRDB Fano polytopes #13282