Opened 10 years ago
Last modified 23 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, GitHub, GitLab) | Commit: | b942f89f1ce7fc5a1053183696689939dba28b65 |
Dependencies: | Stopgaps: |
Description (last modified by )
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)
Change History (30)
Changed 10 years ago by
comment:1 follow-up: ↓ 2 Changed 10 years ago by
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 likegrdb_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 toPolyhedron
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.
comment:2 in reply to: ↑ 1 Changed 10 years ago by
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
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 likegrdb_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 10 years ago by
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 justpolytopes_grdb
(or withGRDB
) and imported in the global name space so that it appears in TAB completion forFano
orpolytope
, just as there is nowpolytopes
database. This is more logical thanlattice_polytope
for those who don't know module hierarchy.
Volker, what are your thoughts?
comment:4 Changed 10 years ago by
On a related note, groups.
discussion:
https://groups.google.com/d/topic/sage-devel/yrCeYitwdxE/discussion
comment:5 Changed 10 years ago by
- Cc rbeezer added
comment:6 Changed 10 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:7 Changed 10 years ago by
- Description modified (diff)
comment:8 Changed 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:9 Changed 9 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:10 Changed 8 years ago by
- 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 8 years ago by
- Branch set to u/tomc/ticket/13282
comment:12 Changed 8 years ago by
- 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 8 years ago by
- Commit set to 755c7e3fbd8af82930df2d11b4cb2f2ca9d3be02
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.
|
comment:14 Changed 8 years ago by
- Keywords polytopes added
comment:15 Changed 8 years ago by
- 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 8 years ago by
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 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:18 Changed 8 years ago by
- Branch changed from u/tomc/ticket/13282 to public/ticket/13282
- Commit changed from 755c7e3fbd8af82930df2d11b4cb2f2ca9d3be02 to 37c562cadfd39ad79f34cdda28a94ab0b98aaa4a
comment:19 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:20 Changed 7 years ago by
- Description modified (diff)
comment:21 Changed 7 years ago by
- Milestone changed from sage-6.4 to sage-6.8
comment:22 Changed 7 years ago by
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 7 years ago by
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 7 years ago by
- Commit changed from 37c562cadfd39ad79f34cdda28a94ab0b98aaa4a to 606bf789bcfcedabc69e8bb0d6adedc53e32658f
comment:25 Changed 7 years ago by
- Milestone changed from sage-6.8 to sage-6.10
comment:26 Changed 5 years ago by
- Commit changed from 606bf789bcfcedabc69e8bb0d6adedc53e32658f to b942f89f1ce7fc5a1053183696689939dba28b65
Branch pushed to git repo; I updated commit sha1. New commits:
b942f89 | Merge branch 'public/ticket/13282' in 8.0.b5
|
comment:27 Changed 23 months ago by
There is interest for this:
Adds access to the GRDB