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

generic latte_int interface to integrate #22522

Closed
mforets mannequin opened this issue Mar 5, 2017 · 41 comments
Closed

generic latte_int interface to integrate #22522

mforets mannequin opened this issue Mar 5, 2017 · 41 comments

Comments

@mforets
Copy link
Mannequin

mforets mannequin commented Mar 5, 2017

Provide a generic interface to integrate a polynomial over a polytope.

See also #18232.

Depends on #22497

CC: @videlec @mkoeppe @jplab

Component: packages: optional

Keywords: days84

Author: Marcelo Forets

Branch/Commit: 6a474f9

Reviewer: Matthias Koeppe

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

@mforets mforets mannequin added this to the sage-7.6 milestone Mar 5, 2017
@mforets
Copy link
Mannequin Author

mforets mannequin commented Mar 5, 2017

Dependencies: #22497

@videlec

This comment has been minimized.

@videlec
Copy link
Contributor

videlec commented Mar 6, 2017

Changed keywords from none to days84

@mforets
Copy link
Mannequin Author

mforets mannequin commented Mar 6, 2017

Branch: u/mforets/22497

@mforets
Copy link
Mannequin Author

mforets mannequin commented Mar 6, 2017

Commit: 96e4099

@mforets
Copy link
Mannequin Author

mforets mannequin commented Mar 6, 2017

New commits:

d5ff15422497: generic interface to LattE integrale count
96e4099Integral of a polynomial over a polytope.

@mforets mforets mannequin added the s: needs review label Mar 6, 2017
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 6, 2017

Changed commit from 96e4099 to d3c9589

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 6, 2017

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

d3c9589Add volume function to generic latte_int interface.

@videlec
Copy link
Contributor

videlec commented Mar 6, 2017

comment:6

Why your commit d3c9589 is here!? Shouldn't it belong to another ticket?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 6, 2017

Changed commit from d3c9589 to 000bf8b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 6, 2017

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

000bf8bRestructured the script, with an integrate function accepting different valuations.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2017

Changed commit from 000bf8b to 5aa6695

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2017

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

5aa6695Added test for helper function _to_latte_polynomial.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2017

Changed commit from 5aa6695 to 72d03a1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2017

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

72d03a1Minor changes to helper function and integrate.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2017

comment:10

Replying to @videlec:

Why your commit d3c9589 is here!? Shouldn't it belong to another ticket?

The branch is on top of the branch of #22497.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2017

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2017

comment:11

Looks good to me.

Now follow-up tickets with corresponding methods of Polyhedron would be good.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2017

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

6a474f9fixed a typo in the docstring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2017

Changed commit from 72d03a1 to 6a474f9

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 7, 2017

comment:13

What does

``[1,[2,2,2]]``

do in the doctest?

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 8, 2017

comment:16

Learn about :: in the doc formatting. This will solve your problem.
Also build and verify the html doc.

@tscrim
Copy link
Collaborator

tscrim commented Mar 8, 2017

comment:17

There are also a number of doc issues with integrate in base.py. The doc should start with

    def integrate(self, polynomial, **kwds):
        r"""
        Return the integral of a polynomial over a polytope.

        INPUT:

        - ``polynomial`` -- a multivariate polynomial or a valid LattE description string
          for a polynomial

        - ``**kwds`` -- additional keyword arguments that are passed to the engine

        OUTPUT:

        The integral of the polynomial over the polytope.

        .. NOTE::

            The polytope triangulation algorithm is used. This function depends on
            LattE (i.e., the ``latte_int`` optional package).

The code in the EXAMPLES:: block also needs to be indented (as per comment:16).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 9, 2017

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

91d885eFix the docstrings, and enhance integrate method.
507aea5Add the new volume engine Polyhedron.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 9, 2017

Changed commit from 17911f7 to 507aea5

@mforets
Copy link
Mannequin Author

mforets mannequin commented Mar 9, 2017

comment:19

done! and thanks both for the help. from my side this ticket and #20887 are ok.

in the proposed solution, the new volume and integrate methods can also receive polyhedra in RDF, and each vertex is cast to QQ with an iterator over the vertices.

@videlec
Copy link
Contributor

videlec commented Mar 9, 2017

comment:20

Replying to @mforets:

done! and thanks both for the help. from my side this ticket and #20887 are ok.

Nice work!

in the proposed solution, the new volume and integrate methods can also receive polyhedra in RDF, and each vertex is cast to QQ with an iterator over the vertices.

Is the answer of .integrate() will be a RDF? Otherwise I think it is a bad idea.

@videlec
Copy link
Contributor

videlec commented Mar 9, 2017

comment:21

Replying to @sagetrac-git:

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

91d885eFix the docstrings, and enhance integrate method.
507aea5Add the new volume engine Polyhedron.

I don't understand why these commits are here... this ticket is about generic latte_int interface to integrate. If you change the purpose of the ticket you need to update the description.

@mforets
Copy link
Mannequin Author

mforets mannequin commented Mar 9, 2017

comment:23

Replying to @videlec:

Replying to @mforets:

done! and thanks both for the help. from my side this ticket and #20887 are ok.

Nice work!

in the proposed solution, the new volume and integrate methods can also receive polyhedra in RDF, and each vertex is cast to QQ with an iterator over the vertices.

Is the answer of .integrate() will be a RDF? Otherwise I think it is a bad idea.

But why? It will be a QQ.

@videlec
Copy link
Contributor

videlec commented Mar 9, 2017

comment:24

Replying to @mforets:

Replying to @videlec:

Replying to @mforets:

done! and thanks both for the help. from my side this ticket and #20887 are ok.

Nice work!

in the proposed solution, the new volume and integrate methods can also receive polyhedra in RDF, and each vertex is cast to QQ with an iterator over the vertices.

Is the answer of .integrate() will be a RDF? Otherwise I think it is a bad idea.

But why? It will be a QQ.

It does not make any sense. If I have a Polyhedron defined over RDF (= non exact field) its volume can not defined as an element of QQ (= exact field).

@mforets
Copy link
Mannequin Author

mforets mannequin commented Mar 10, 2017

comment:25

Hmm.. Matrices have a change_ring feature that comes very handy for conversion. Shall we add a similar one for Polyhedron?

And following your recommendation, make integrate and volume via latte raise a value error when instantiated with, say, P = 1.4142*polytopes.cube().

With the change ring, we could still integrate integrate a polynomial over P with the slightly more complicated P.change_ring().integrate(x<sup>2*y</sup>2*z^2), instead of the ugly

sage: P_QQ = Polyhedron(vertices = [[QQ(vi) for vi in v] for v in P.vertices_generator()])
sage: P_QQ.integrate(x^2*y^2*z^2)

If there are alternative recommended options, I'd really like to learn; but I can post it as a ask.sagemath one if i'm off-topic :)

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 10, 2017

comment:26

Integration over an RDF polyhedron via QQ polyhedron (latte) could make sense as long as the method converts the answer back to RDF to indicate that the answer is inexact.

As to change_ring, this could be useful. I had related discussions with JP.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 10, 2017

comment:27

(In addition to change_ring, there could also be change_backend.)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 10, 2017

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

a563192fix a bug in _volume_latte

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 10, 2017

Changed commit from 507aea5 to a563192

@mforets
Copy link
Mannequin Author

mforets mannequin commented Mar 10, 2017

Changed branch from u/mforets/22497 to u/mforets/22522

@mforets
Copy link
Mannequin Author

mforets mannequin commented Mar 10, 2017

Changed commit from a563192 to 6a474f9

@vbraun
Copy link
Member

vbraun commented Mar 27, 2017

Changed branch from u/mforets/22522 to 6a474f9

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

4 participants