ehrhart_polynomial should be made available for polytopes defined over QQ

[mkoeppe]

... because they can happen to be lattice polytopes. An exception should be raised when they are not.

See ​http://trac.sagemath.org/ticket/18812#comment:33

[jipilab]

This should also be done through the backend normaliz with the version coming after 3.5.3. The call to get the ehrhart_polynomial in normaliz will be facilitated and would be nice to have too.

The upgrade to normaliz 3.5.3 is here: #22984.

[Winfried]

Normaliz and PyNormaliz? have the computation goal EhrhartQuasipolynomial? since Normaliz version 3.6.0.

[selia]

New commits:

​49a6107	first step for ehrhart polynomial
​d35b4e1	first changes

[git]

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

​c378edb

more changes

[git]

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

​50a6441

fixed typos

[git]

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

​f65b2d9

fixed compactness if statement

[jipilab]

Let's start with ZZ.

In base_ZZ.py (structure looks great):

• You can correct the doctests later (this might save time)
• ehrhart_polynomial: empty line after INPUT, and period at the end of engine description.
• Eventually replace the third input by the description in the _latte method.
• the current check if self.is_empty(): located at the beginning of _ehrhart_polynomial_latte could be moved to the public ehrhart_polynomial.

In backend_normaliz.py:

• looks good!

• I am tempted to simply make the definition of the two _ehrhart_polynomial_normaliz as alias:

    _ehrhart_polynomial_normaliz = _ehrhart_quasi_polynomial_normaliz

• The check if self.is_empty(): of _ehrhart_quasi_normaliz can be safely removed since the function should not be reached if it is empty since it is dealt with in the front function. I believe the same could also be checked for compactness in the front function (in base_ZZ and base_QQ).

[git]

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

​fec936b

adapt base_ZZ

[git]

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

​a561fe6

remove quasipolynomial for lattice polytopes

[git]

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

​1a5aa83

adapting base_QQ.py

[git]

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

​bc646b0

changes in base_QQ

[git]

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

​9bb89f8

changed error message

[git]

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

​de9cd45	docstring base_ZZ
​d3dc7aa	fix py3 in base.py with optional packages
​c69d74f	moved the import statement
​e5ecbcf	pyflakes
​846069d	pyflakes-bis
​4a9a35e	Merge branch 'public/28235' of trac.sagemath.org:sage into 18957

[git]

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

​0460583

more docstring changes for base_ZZ.py

[git]

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

​77c9ec8

initial docstring changes for base_QQ.py

[git]

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

​5e97d90

took out a period

[git]

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

​2c214b4

working on the variable

[git]

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

​6af8861

revert to fixed variable

[jipilab]

Ok, I give up, it is not nice to try to make latte have the option to specify the variable. It involves changing the interface, and perhaps not in the scope of this ticket.

I reverted the changes involving that around the _latte functions. It should be back to normal. So, I suggest that the variable= be only a normaliz engine option and not a latte one. Normaliz returns the data without the use of a variable, this is why it makes it easy to provide the option for the user to choose it since we have to choose one anyway.

[jipilab]

... if one really wants to make it also possible for latte, the option to change the base ring variable is then a valid choice. But I would add a comment next to it saying:

# TO DO: replace this change of variable by creating the appropriate polynomial ring in the latte interface.

[git]

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

​0f245b3

base_QQ doc work

[git]

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

​c7cb6d6

spacing

[git]

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

​be46cbc

added one more example to base_QQ

[jipilab]

Here are some small edits to be done:

• You can forget my previous comment abour the heading space added.

• No spaces are needed between equality in the definition of a function:

- def _ehrhart_quasipolynomial_normaliz(self,variable = 't'):
+ def _ehrhart_quasipolynomial_normaliz(self,variable='t'):

• The output should be related to the data structure, without much details of what they are. The text with details should then be moved after the one-line description of the function to explain the output.

        OUTPUT:

-        If it is a polynomial, returns the polynomial. Otherwise, returns a
-        tuple of rational polynomials whose length is the quasi-period of the
-        quasi-polynomial and each rational polynomial describes a residue class.

+        A polynomial or tuple of polynomials.

• Most examples in doctests with text explanations should be moved to the public function without underscore so that they are accessible to the users. Then the examples of the private functions should show that they work as needed and that raising Errors are done appropriately.

• Some formating of variables are missing (see ​http://doc.sagemath.org/html/en/developer/coding_basics.html):

- * None (default): When no input is given the Ehrhart polynomial
+ * ``None`` (default): When no input is given the Ehrhart polynomial

• it is conventional to use lower cases for objects:

- sage: Line_Seg = Polyhedron(vertices=[[0],[1/2]],backend='normaliz')
+ sage: line_seg = Polyhedron(vertices=[[0],[1/2]],backend='normaliz')

[git]

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

​d08171b

fixing formatting to fit conventions

[chapoton]

ready for review ?

[chapoton]

The branch is red, and the required rebase on the latest develop branch is not trivial.

[jipilab]

I forgot that we could have put it to needs review to get some bot feedback already, but it was still quite under constant development.

Once the merge conflict is resolved, it could benefit from some bot-runs before another round of review.

As this function is a good example of interfacing of normaliz, I would say it is worth to make it a robust and clean model for other eventual methods using normaliz. It is in quite good shape, once it is on needs review, I would check again if we covered all the intricate cases of backends and base rings...

[git]

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

​08a5d67

added to-dos for changing polynomial via new polynomial ring

[jipilab]

Could you merge the latest sage version into your branch? There is a conflict...

Then, I guess it would ready for review.

[git]

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

​db84f51

first step for ehrhart polynomial

[jipilab]

Okay!

From the patchbot:

• 2 failing doctests in backend_normaliz
• 2 functions are missing doctests in base_ZZ
• pyflakes error in base_ZZ
• the documentation of base_QQ does not build:

OSError: /home/worker/sage-patchbot/local/lib/python2.7/site-packages/sage/geometry/polyhedron/base_QQ.py:docstring of sage.geometry.polyhedron.base_QQ.Polyhedron_QQ.ehrhart_quasipolynomial:69: WARNING: Inline literal start-string without end-string.

[jipilab]

One more thing: we should consider making some of these methods cached so as to not recompute everything every time...

[chapoton]

empty TESTS section should be removed:

+        TESTS::
+        """

[git]

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

​6e34f90

fixing documentation

[git]

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

​e7e5ca6

pyflakes

[chapoton]

doc still does not build:

sage.geometry.polyhedron.base_QQ.Polyhedron_QQ.ehrhart_quasipolynomial:110: WARNING: Inline literal start-string without end-string

maybe because of a missing empty line after .. WARNING::

[jipilab]

And there is one superfluous import, see the pyflake error.

There is also the coverage to take care of ...

[git]

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

​57dbd85

added empty line after warning

[tscrim]

You might also still have some doc problems

         -  ``variable`` -- string (default: 't'); The variable in which the
-              Ehrhart polynomial should be expressed.
+           Ehrhart polynomial should be expressed
 
         - ``engine`` -- string; The backend to use. Allowed values are:
 
-            * ``None`` (default); When no input is given the Ehrhart polynomial
-              is computed using LattE Integrale (optional)
+          * ``None`` (default); When no input is given the Ehrhart polynomial
+            is computed using LattE Integrale (optional)

and similar. My understanding is sphinx likes things to be aligned with the first non-whitespace character after the - (or *) in a list.

[chapoton]

The following is not good:

TypeError: The backend of ``self`` should be 'normaliz'

There should be no such formatting inside doctests themselves.

[git]

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

​c589388

spacing and coverage fix

[git]

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

​d8886fc

removed backticks

[chapoton]

and some failing doctests, one missing optional and some misplaced optional

[chapoton]

and doc still does not build. The offending line seems to be

``backend``='normaliz'::

in line 490, which should rather be

``backend='normaliz'``::

[git]

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

​60c2e8a

moving backticks

[chapoton]

There remains the same backtick problem in the other file base_ZZ.py in the line 400

``engine``='latte' and then with ``engine``='normaliz'.

and still the pyflakes warnings: do not write engine is 'latte' but engine == 'latte'. See patchbot pyflakes plugins for the 4 exact lines where you should fix that.

and still the failing doctests due to missing or misplaced # optional tags.

[git]

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

​125f822	Merge branch 'public/18957' in 8.9
​f809935	trac 18957 fixing some details

[chapoton]

I have fixed my own comments. There remains only one failing doctest in

src/sage/geometry/polyhedron/base_ZZ.py

, that you should take care of.

[chapoton]

one last effort is required..

[jipilab]

We're on it! :) The semester starts today, and we'll have more time to look into sage.

[git]

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

​b152001	Merge branch 'public/18957' of trac.sagemath.org:sage into sage8.9beta0
​4786032	removed a superfluous indirect test

[jipilab]

Needs review again. It should be good to go now.

The failing doctest due to a missing optional package was a superfluous indirect doctest, so I removed it. I don't think it needed testing there. If you disagree, I could put it back with the optional tag.

[chapoton]

typo "integeral"

sinon, c'est bon : positive review

[git]

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

​b373a3b

typo