Opened 5 years ago

Closed 6 months ago

#18957 closed enhancement (fixed)

ehrhart_polynomial should be made available for polytopes defined over QQ

Reported by: mkoeppe Owned by:
Priority: minor Milestone: sage-9.0
Component: geometry Keywords: latte, days100
Cc: jipilab, selia Merged in:
Authors: Sophia Elia Reviewers: Jean-Philippe Labbé, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: b373a3b (Commits) Commit: b373a3b3447f3c17ef8caaecb0aa349e60d1e091
Dependencies: #28235 Stopgaps:

Description

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

Change History (64)

comment:1 Changed 2 years ago by jipilab

  • Cc jipilab added

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

comment:3 Changed 21 months ago by Winfried

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

comment:4 Changed 9 months ago by jipilab

  • Cc selia added

comment:5 Changed 9 months ago by selia

  • Branch set to public/18957
  • Commit set to d35b4e120adb96361aacd7632587060583588d5c
  • Keywords days100 added

New commits:

49a6107first step for ehrhart polynomial
d35b4e1first changes

comment:6 Changed 9 months ago by git

  • Commit changed from d35b4e120adb96361aacd7632587060583588d5c to c378edbbee41aece5b618ed5b78dbfe24c85db17

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

c378edbmore changes

comment:7 Changed 9 months ago by git

  • Commit changed from c378edbbee41aece5b618ed5b78dbfe24c85db17 to 50a644145c216f1c531be01f82f43724ef574bb3

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

50a6441fixed typos

comment:8 Changed 9 months ago by git

  • Commit changed from 50a644145c216f1c531be01f82f43724ef574bb3 to f65b2d9f92d8a69be35cb748c5c708adc70bef5e

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

f65b2d9fixed compactness if statement

comment:9 Changed 9 months ago by 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).

comment:10 Changed 9 months ago by git

  • Commit changed from f65b2d9f92d8a69be35cb748c5c708adc70bef5e to fec936b7fdd83cb3f803a08ed7dc09923543dc26

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

fec936badapt base_ZZ

comment:11 Changed 9 months ago by git

  • Commit changed from fec936b7fdd83cb3f803a08ed7dc09923543dc26 to a561fe629585bd8f53e9dce479acb449966ad928

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

a561fe6remove quasipolynomial for lattice polytopes

comment:12 Changed 9 months ago by git

  • Commit changed from a561fe629585bd8f53e9dce479acb449966ad928 to 1a5aa83c815cfce4655a2012158502c75b92489b

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

1a5aa83adapting base_QQ.py

comment:13 Changed 9 months ago by git

  • Commit changed from 1a5aa83c815cfce4655a2012158502c75b92489b to bc646b0490bfd4fb3aa3a32f84367c1071b6288a

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

bc646b0changes in base_QQ

comment:14 Changed 9 months ago by git

  • Commit changed from bc646b0490bfd4fb3aa3a32f84367c1071b6288a to 9bb89f80b85a564ad01475a344ab76cd6713cca1

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

9bb89f8changed error message

comment:15 Changed 9 months ago by selia

  • Dependencies set to #28235

comment:16 Changed 9 months ago by git

  • Commit changed from 9bb89f80b85a564ad01475a344ab76cd6713cca1 to 4a9a35eedd34c3632048dad3d41b610788b27bf3

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

de9cd45docstring base_ZZ
d3dc7aafix py3 in base.py with optional packages
c69d74fmoved the import statement
e5ecbcfpyflakes
846069dpyflakes-bis
4a9a35eMerge branch 'public/28235' of trac.sagemath.org:sage into 18957

comment:17 Changed 9 months ago by git

  • Commit changed from 4a9a35eedd34c3632048dad3d41b610788b27bf3 to 0460583fa008fbfe59e4a85836b3dfbe18fc06da

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

0460583more docstring changes for base_ZZ.py

comment:18 Changed 9 months ago by vdelecroix

  • Milestone changed from sage-6.9 to sage-8.9

comment:19 Changed 9 months ago by git

  • Commit changed from 0460583fa008fbfe59e4a85836b3dfbe18fc06da to 77c9ec8e3f83baba4e8ece88003aaccfdd25338c

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

77c9ec8initial docstring changes for base_QQ.py

comment:20 Changed 9 months ago by git

  • Commit changed from 77c9ec8e3f83baba4e8ece88003aaccfdd25338c to 5e97d90f3da4f24ffca482eede5141966db9a9ef

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

5e97d90took out a period

comment:21 Changed 9 months ago by git

  • Commit changed from 5e97d90f3da4f24ffca482eede5141966db9a9ef to 2c214b422079560d30d620d68dd85fc55d08c0e6

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

2c214b4working on the variable

comment:22 Changed 9 months ago by git

  • Commit changed from 2c214b422079560d30d620d68dd85fc55d08c0e6 to 6af886110bd323070b020b82289ea48c69c7d07f

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

6af8861revert to fixed variable

comment:23 Changed 9 months ago by 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.

comment:24 Changed 9 months ago by 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.

comment:25 Changed 9 months ago by git

  • Commit changed from 6af886110bd323070b020b82289ea48c69c7d07f to 0f245b394d3514494bcce58beed09edbca447070

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

0f245b3base_QQ doc work

comment:26 Changed 9 months ago by git

  • Commit changed from 0f245b394d3514494bcce58beed09edbca447070 to c7cb6d648e4611f96b0206e99b6e3c24638f7d09

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

c7cb6d6spacing

comment:27 Changed 9 months ago by git

  • Commit changed from c7cb6d648e4611f96b0206e99b6e3c24638f7d09 to be46cbc8eddcb8fdfc24e23aebf74667195acd66

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

be46cbcadded one more example to base_QQ

comment:28 Changed 8 months ago by 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.
- * 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')
Last edited 8 months ago by jipilab (previous) (diff)

comment:29 Changed 8 months ago by git

  • Commit changed from be46cbc8eddcb8fdfc24e23aebf74667195acd66 to d08171b3aa70ebc0637547ddc0d72b9345255d1a

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

d08171bfixing formatting to fit conventions

comment:30 Changed 8 months ago by chapoton

ready for review ?

comment:31 Changed 8 months ago by chapoton

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

comment:32 Changed 8 months ago by jipilab

  • Authors set to Sophia Elia

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

comment:33 Changed 7 months ago by git

  • Commit changed from d08171b3aa70ebc0637547ddc0d72b9345255d1a to 08a5d67ec1115d0a9f901b65837a7c29a26e0c39

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

08a5d67added to-dos for changing polynomial via new polynomial ring

comment:34 Changed 7 months ago by jipilab

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

Then, I guess it would ready for review.

comment:35 Changed 7 months ago by git

  • Commit changed from 08a5d67ec1115d0a9f901b65837a7c29a26e0c39 to db84f51bcdf50fb92ad616bb18cc2626f0aff782

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

db84f51first step for ehrhart polynomial

comment:36 Changed 7 months ago by jipilab

  • Reviewers set to Jean-Philippe Labbé
  • Status changed from new to needs_review

comment:37 Changed 7 months ago by jipilab

  • Status changed from needs_review to needs_work

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.

comment:38 Changed 7 months ago by jipilab

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

comment:39 Changed 7 months ago by chapoton

empty TESTS section should be removed:

+        TESTS::
+        """

comment:40 Changed 7 months ago by git

  • Commit changed from db84f51bcdf50fb92ad616bb18cc2626f0aff782 to 6e34f903561eff2025b3403b77838f7449ce6836

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

6e34f90fixing documentation

comment:41 Changed 7 months ago by git

  • Commit changed from 6e34f903561eff2025b3403b77838f7449ce6836 to e7e5ca66920654e97f20c9697c63435a7d667aa1

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

e7e5ca6pyflakes

comment:42 Changed 7 months ago by selia

  • Status changed from needs_work to needs_review

comment:43 Changed 7 months ago by 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::

comment:44 Changed 7 months ago by jipilab

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

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

comment:45 Changed 7 months ago by git

  • Commit changed from e7e5ca66920654e97f20c9697c63435a7d667aa1 to 57dbd85a39c6e7ef9d48dd0705ba0edf01a82ee9

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

57dbd85added empty line after warning

comment:46 Changed 7 months ago by 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.

comment:47 Changed 7 months ago by chapoton

The following is not good:

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

There should be no such formatting inside doctests themselves.

comment:48 Changed 7 months ago by git

  • Commit changed from 57dbd85a39c6e7ef9d48dd0705ba0edf01a82ee9 to c589388078516b0f2933004bef9b908c70e065f6

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

c589388spacing and coverage fix

comment:49 Changed 7 months ago by git

  • Commit changed from c589388078516b0f2933004bef9b908c70e065f6 to d8886fc473a59172a8f6885653b6465a916b74df

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

d8886fcremoved backticks

comment:50 Changed 7 months ago by chapoton

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

comment:51 Changed 7 months ago by chapoton

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

``backend``='normaliz'::

in line 490, which should rather be

``backend='normaliz'``::

comment:52 Changed 7 months ago by git

  • Commit changed from d8886fc473a59172a8f6885653b6465a916b74df to 60c2e8a7388e78907194724bf7de94a879bbb877

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

60c2e8amoving backticks

comment:53 Changed 7 months ago by 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.

comment:54 Changed 6 months ago by git

  • Commit changed from 60c2e8a7388e78907194724bf7de94a879bbb877 to f809935738a710718021557ebd15da2844ebaa01

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

125f822Merge branch 'public/18957' in 8.9
f809935trac 18957 fixing some details

comment:55 Changed 6 months ago by 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.

Last edited 6 months ago by chapoton (previous) (diff)

comment:56 Changed 6 months ago by chapoton

one last effort is required..

comment:57 Changed 6 months ago by jipilab

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

comment:58 Changed 6 months ago by git

  • Commit changed from f809935738a710718021557ebd15da2844ebaa01 to 4786032cd5a0338006b25a3143b6ce67c3f64fbf

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

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

comment:59 Changed 6 months ago by 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.

comment:60 Changed 6 months ago by chapoton

typo "integeral"

sinon, c'est bon : positive review

comment:61 Changed 6 months ago by git

  • Commit changed from 4786032cd5a0338006b25a3143b6ce67c3f64fbf to b373a3b3447f3c17ef8caaecb0aa349e60d1e091

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

b373a3btypo

comment:62 Changed 6 months ago by jipilab

  • Reviewers changed from Jean-Philippe Labbé to Jean-Philippe Labbé, Frédéric Chapoton
  • Status changed from needs_review to positive_review

comment:63 Changed 6 months ago by chapoton

  • Milestone changed from sage-8.9 to sage-9.0

comment:64 Changed 6 months ago by vbraun

  • Branch changed from public/18957 to b373a3b3447f3c17ef8caaecb0aa349e60d1e091
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.