Opened 8 years ago

Last modified 2 months ago

#13282 needs_work enhancement

Access to GRDB Fano polytopes

Reported by: sjg10 Owned by: mhampton
Priority: major Milestone: sage-6.10
Component: geometry Keywords: polytopes
Cc: tomc, vbraun, novoselt, rbeezer Merged in:
Authors: Samuel Gonshaw Reviewers:
Report Upstream: N/A Work issues: address compile failure
Branch: public/ticket/13282 (Commits) Commit: b942f89f1ce7fc5a1053183696689939dba28b65
Dependencies: Stopgaps:

Description (last modified by chapoton)

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.

Attachments (3)

trac_13282_GRDB_polytopes_access.patch (73.1 KB) - added by sjg10 8 years ago.
Adds access to the GRDB
trac13282_GRDB_polytopes_new_module2.patch (30.1 KB) - added by sjg10 8 years ago.
Access to GRDB via module lattice.polytopes
trac13282_GRDB_polytopes_new_module3.patch (30.3 KB) - added by sjg10 8 years ago.
added recognition of MAGMA contribution

Download all attachments as: .zip

Change History (30)

Changed 8 years ago by sjg10

Adds access to the GRDB

comment:1 follow-up: Changed 8 years ago 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.

comment:2 in reply to: ↑ 1 Changed 8 years ago by 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.

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

comment:3 Changed 8 years ago by 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?

comment:4 Changed 8 years ago by novoselt

comment:5 Changed 8 years ago by rbeezer

  • Cc rbeezer added

comment:6 Changed 8 years ago by sjg10

  • Description modified (diff)
  • Status changed from new to needs_review

Changed 8 years ago by sjg10

Access to GRDB via module lattice.polytopes

comment:7 Changed 8 years ago by sjg10

  • Description modified (diff)

Changed 8 years ago by sjg10

added recognition of MAGMA contribution

comment:8 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:9 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:10 Changed 7 years ago by tomc

  • Created changed from 07/23/12 09:11:30 to 07/23/12 09:11:30
  • Description modified (diff)
  • Modified changed from 01/30/14 21:20:52 to 01/30/14 21:20:52

comment:11 Changed 7 years ago by tomc

  • Branch set to u/tomc/ticket/13282

comment:12 Changed 7 years ago by tomc

  • Modified changed from 03/03/14 19:54:26 to 03/03/14 19:54:26

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.

comment:13 Changed 7 years ago by git

  • Commit set to 755c7e3fbd8af82930df2d11b4cb2f2ca9d3be02

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

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

comment:14 Changed 7 years ago by chapoton

  • Keywords polytopes added

comment:15 Changed 7 years ago by rws

  • Status changed from needs_review to needs_work
  • Work issues set to address compile failure

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

comment:16 Changed 7 years ago by 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.

comment:17 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:18 Changed 6 years ago by chapoton

  • Branch changed from u/tomc/ticket/13282 to public/ticket/13282
  • Commit changed from 755c7e3fbd8af82930df2d11b4cb2f2ca9d3be02 to 37c562cadfd39ad79f34cdda28a94ab0b98aaa4a

I have made the code pep8 compliant.


New commits:

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

comment:19 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:20 Changed 6 years ago by chapoton

  • Description modified (diff)

comment:21 Changed 5 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-6.8

comment:22 Changed 5 years ago by 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

comment:23 Changed 5 years ago by 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')

comment:24 Changed 5 years ago by git

  • Commit changed from 37c562cadfd39ad79f34cdda28a94ab0b98aaa4a to 606bf789bcfcedabc69e8bb0d6adedc53e32658f

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

f93a39bMerge branch 'public/ticket/13282' into 6.10.b0
606bf78trac #13282 fixing the import of SAGE_SHARE

comment:25 Changed 5 years ago by chapoton

  • Milestone changed from sage-6.8 to sage-6.10

comment:26 Changed 4 years ago by git

  • Commit changed from 606bf789bcfcedabc69e8bb0d6adedc53e32658f to b942f89f1ce7fc5a1053183696689939dba28b65

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

b942f89Merge branch 'public/ticket/13282' in 8.0.b5
Note: See TracTickets for help on using tickets.