Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

document and doctest constructing polyhedra over number fields #26077

Closed
dimpase opened this issue Aug 17, 2018 · 17 comments
Closed

document and doctest constructing polyhedra over number fields #26077

dimpase opened this issue Aug 17, 2018 · 17 comments

Comments

@dimpase
Copy link
Member

dimpase commented Aug 17, 2018

Currently, the only example given in Polyhedron docstring is

    When the input contains elements of a Number Field, they require an
    embedding::

        sage: K = NumberField(x^2-2,'s')
        sage: s = K.0
        sage: L = NumberField(x^3-2,'t')
        sage: t = L.0
        sage: P = Polyhedron(vertices = [[0,s],[t,0]])
        Traceback (most recent call last):
        ...
        ValueError: invalid base ring

This is clearly insufficient from online documentation point of view. One has to look either at the html/pdf docs ot at the header of the file sage/geometry/polyhedron/constructor.py for more examples with embeddings specified.

CC: @jplab @mkoeppe @mo271

Component: geometry

Author: Dima Pasechnik

Branch/Commit: a962a00

Reviewer: Jean-Philippe Labbé

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

@dimpase dimpase added this to the sage-8.4 milestone Aug 17, 2018
@dimpase
Copy link
Member Author

dimpase commented Aug 17, 2018

comment:1

Could please someone provide a working example? I'll do the rest...

@dimpase
Copy link
Member Author

dimpase commented Aug 17, 2018

comment:2

here is a small working example

R0.<r0>=QQ[]
R1.<r1>=NumberField(r0^2-2, embedding=1.414213562373095)
s = Polyhedron(vertices = [[0,1, 1], [1,r1, -1], [2,-1, 1], [7,-1, -1]], base_ring=R1)

I'd also have something more interesting, e.g. a regular icosahedron.

@dimpase

This comment has been minimized.

@dimpase
Copy link
Member Author

dimpase commented Aug 17, 2018

Author: Dima Pasechnik

@jplab
Copy link

jplab commented Aug 18, 2018

comment:4

Have a look at #17197 (which this present ticket might qualify as a duplicate).

Yes, this is true that polyhedron over number field is now well-documented. You can have a look at

#22420: Meta-ticket: Polyhedron: new features and known bugs

and

https://wiki.sagemath.org/OptiPolyGeom

for a roadmap about things on polyhedra. This issue is on the list for a long time. My priority was to offer a wide breath tutorial on polyhedra and use it to then complete the docstring. Perhaps not the right way around, but the tutorial is now available:

http://doc.sagemath.org/html/en/thematic_tutorials/geometry/polyhedra_tutorial.html

thanks to

#22572: Add a thematic tutorial on the polyhedron class

from which you can pick many examples that work under Sage 8.3.

I am sorry if the documentation string still has not been adapted yet.

@dimpase
Copy link
Member Author

dimpase commented Aug 18, 2018

Commit: 1a0c188

@dimpase
Copy link
Member Author

dimpase commented Aug 18, 2018

Branch: u/dimpase/geometry/poly_NF_doc

@dimpase
Copy link
Member Author

dimpase commented Aug 18, 2018

comment:5

Here is a short fix for this. I deliberately constructed an example from scratch,
rather than picking one from polytopes.*.

I wonder how one can make pointers to the tutorial in the reference manual without
mentioning the full URL.


New commits:

1a0c188add doc for #26077

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 18, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d9d1220add doc for #26077

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 18, 2018

Changed commit from 1a0c188 to d9d1220

@dimpase
Copy link
Member Author

dimpase commented Aug 18, 2018

comment:7

some typos fixed; now the links look proper.

@jplab
Copy link

jplab commented Aug 30, 2018

Changed branch from u/dimpase/geometry/poly_NF_doc to public/26077

@jplab
Copy link

jplab commented Aug 30, 2018

Changed commit from d9d1220 to a962a00

@jplab
Copy link

jplab commented Aug 30, 2018

Reviewer: Jean-Philippe Labbé

@jplab
Copy link

jplab commented Aug 30, 2018

comment:8

Hi,

This looks good to me. I added the spacing convention to the example.

You can put it as positive review on my behalf if you accept the changes.


New commits:

a962a00Added spacing convention

@dimpase
Copy link
Member Author

dimpase commented Aug 30, 2018

comment:9

Thanks.

@vbraun
Copy link
Member

vbraun commented Sep 1, 2018

Changed branch from public/26077 to a962a00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants