Opened 7 years ago
Closed 3 years ago
#18957 closed enhancement (fixed)
ehrhart_polynomial should be made available for polytopes defined over QQ
Reported by:  Matthias Köppe  Owned by:  

Priority:  minor  Milestone:  sage9.0 
Component:  geometry  Keywords:  latte, days100 
Cc:  JeanPhilippe Labbé, Sophia Elia  Merged in:  
Authors:  Sophia Elia  Reviewers:  JeanPhilippe Labbé, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  b373a3b (Commits, GitHub, GitLab)  Commit:  b373a3b3447f3c17ef8caaecb0aa349e60d1e091 
Dependencies:  #28235  Stopgaps: 
Description
... because they can happen to be lattice polytopes. An exception should be raised when they are not.
Change History (64)
comment:1 Changed 5 years ago by
Cc:  JeanPhilippe Labbé added 

comment:2 Changed 5 years ago by
comment:3 Changed 4 years ago by
Normaliz and PyNormaliz? have the computation goal EhrhartQuasipolynomial? since Normaliz version 3.6.0.
comment:4 Changed 3 years ago by
Cc:  Sophia Elia added 

comment:5 Changed 3 years ago by
Branch:  → public/18957 

Commit:  → d35b4e120adb96361aacd7632587060583588d5c 
Keywords:  days100 added 
comment:6 Changed 3 years ago by
Commit:  d35b4e120adb96361aacd7632587060583588d5c → c378edbbee41aece5b618ed5b78dbfe24c85db17 

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

comment:7 Changed 3 years ago by
Commit:  c378edbbee41aece5b618ed5b78dbfe24c85db17 → 50a644145c216f1c531be01f82f43724ef574bb3 

Branch pushed to git repo; I updated commit sha1. New commits:
50a6441  fixed typos

comment:8 Changed 3 years ago by
Commit:  50a644145c216f1c531be01f82f43724ef574bb3 → f65b2d9f92d8a69be35cb748c5c708adc70bef5e 

Branch pushed to git repo; I updated commit sha1. New commits:
f65b2d9  fixed compactness if statement

comment:9 Changed 3 years ago by
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 publicehrhart_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 (inbase_ZZ
andbase_QQ
).
comment:10 Changed 3 years ago by
Commit:  f65b2d9f92d8a69be35cb748c5c708adc70bef5e → fec936b7fdd83cb3f803a08ed7dc09923543dc26 

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

comment:11 Changed 3 years ago by
Commit:  fec936b7fdd83cb3f803a08ed7dc09923543dc26 → a561fe629585bd8f53e9dce479acb449966ad928 

Branch pushed to git repo; I updated commit sha1. New commits:
a561fe6  remove quasipolynomial for lattice polytopes

comment:12 Changed 3 years ago by
Commit:  a561fe629585bd8f53e9dce479acb449966ad928 → 1a5aa83c815cfce4655a2012158502c75b92489b 

Branch pushed to git repo; I updated commit sha1. New commits:
1a5aa83  adapting base_QQ.py

comment:13 Changed 3 years ago by
Commit:  1a5aa83c815cfce4655a2012158502c75b92489b → bc646b0490bfd4fb3aa3a32f84367c1071b6288a 

Branch pushed to git repo; I updated commit sha1. New commits:
bc646b0  changes in base_QQ

comment:14 Changed 3 years ago by
Commit:  bc646b0490bfd4fb3aa3a32f84367c1071b6288a → 9bb89f80b85a564ad01475a344ab76cd6713cca1 

Branch pushed to git repo; I updated commit sha1. New commits:
9bb89f8  changed error message

comment:15 Changed 3 years ago by
Dependencies:  → #28235 

comment:16 Changed 3 years ago by
Commit:  9bb89f80b85a564ad01475a344ab76cd6713cca1 → 4a9a35eedd34c3632048dad3d41b610788b27bf3 

comment:17 Changed 3 years ago by
Commit:  4a9a35eedd34c3632048dad3d41b610788b27bf3 → 0460583fa008fbfe59e4a85836b3dfbe18fc06da 

Branch pushed to git repo; I updated commit sha1. New commits:
0460583  more docstring changes for base_ZZ.py

comment:18 Changed 3 years ago by
Milestone:  sage6.9 → sage8.9 

comment:19 Changed 3 years ago by
Commit:  0460583fa008fbfe59e4a85836b3dfbe18fc06da → 77c9ec8e3f83baba4e8ece88003aaccfdd25338c 

Branch pushed to git repo; I updated commit sha1. New commits:
77c9ec8  initial docstring changes for base_QQ.py

comment:20 Changed 3 years ago by
Commit:  77c9ec8e3f83baba4e8ece88003aaccfdd25338c → 5e97d90f3da4f24ffca482eede5141966db9a9ef 

Branch pushed to git repo; I updated commit sha1. New commits:
5e97d90  took out a period

comment:21 Changed 3 years ago by
Commit:  5e97d90f3da4f24ffca482eede5141966db9a9ef → 2c214b422079560d30d620d68dd85fc55d08c0e6 

Branch pushed to git repo; I updated commit sha1. New commits:
2c214b4  working on the variable

comment:22 Changed 3 years ago by
Commit:  2c214b422079560d30d620d68dd85fc55d08c0e6 → 6af886110bd323070b020b82289ea48c69c7d07f 

Branch pushed to git repo; I updated commit sha1. New commits:
6af8861  revert to fixed variable

comment:23 Changed 3 years ago by
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 3 years ago by
... 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 3 years ago by
Commit:  6af886110bd323070b020b82289ea48c69c7d07f → 0f245b394d3514494bcce58beed09edbca447070 

Branch pushed to git repo; I updated commit sha1. New commits:
0f245b3  base_QQ doc work

comment:26 Changed 3 years ago by
Commit:  0f245b394d3514494bcce58beed09edbca447070 → c7cb6d648e4611f96b0206e99b6e3c24638f7d09 

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

comment:27 Changed 3 years ago by
Commit:  c7cb6d648e4611f96b0206e99b6e3c24638f7d09 → be46cbc8eddcb8fdfc24e23aebf74667195acd66 

Branch pushed to git repo; I updated commit sha1. New commits:
be46cbc  added one more example to base_QQ

comment:28 Changed 3 years ago by
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 oneline 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 quasiperiod of the  quasipolynomial 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')
comment:29 Changed 3 years ago by
Commit:  be46cbc8eddcb8fdfc24e23aebf74667195acd66 → d08171b3aa70ebc0637547ddc0d72b9345255d1a 

Branch pushed to git repo; I updated commit sha1. New commits:
d08171b  fixing formatting to fit conventions

comment:31 Changed 3 years ago by
The branch is red, and the required rebase on the latest develop branch is not trivial.
comment:32 Changed 3 years ago by
Authors:  → 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 botruns 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 3 years ago by
Commit:  d08171b3aa70ebc0637547ddc0d72b9345255d1a → 08a5d67ec1115d0a9f901b65837a7c29a26e0c39 

Branch pushed to git repo; I updated commit sha1. New commits:
08a5d67  added todos for changing polynomial via new polynomial ring

comment:34 Changed 3 years ago by
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 3 years ago by
Commit:  08a5d67ec1115d0a9f901b65837a7c29a26e0c39 → db84f51bcdf50fb92ad616bb18cc2626f0aff782 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
db84f51  first step for ehrhart polynomial

comment:36 Changed 3 years ago by
Reviewers:  → JeanPhilippe Labbé 

Status:  new → needs_review 
comment:37 Changed 3 years ago by
Status:  needs_review → 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/sagepatchbot/local/lib/python2.7/sitepackages/sage/geometry/polyhedron/base_QQ.py:docstring of sage.geometry.polyhedron.base_QQ.Polyhedron_QQ.ehrhart_quasipolynomial:69: WARNING: Inline literal startstring without endstring.
comment:38 Changed 3 years ago by
One more thing: we should consider making some of these methods cached so as to not recompute everything every time...
comment:40 Changed 3 years ago by
Commit:  db84f51bcdf50fb92ad616bb18cc2626f0aff782 → 6e34f903561eff2025b3403b77838f7449ce6836 

Branch pushed to git repo; I updated commit sha1. New commits:
6e34f90  fixing documentation

comment:41 Changed 3 years ago by
Commit:  6e34f903561eff2025b3403b77838f7449ce6836 → e7e5ca66920654e97f20c9697c63435a7d667aa1 

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

comment:42 Changed 3 years ago by
Status:  needs_work → needs_review 

comment:43 Changed 3 years ago by
doc still does not build:
sage.geometry.polyhedron.base_QQ.Polyhedron_QQ.ehrhart_quasipolynomial:110: WARNING: Inline literal startstring without endstring
maybe because of a missing empty line after .. WARNING::
comment:44 Changed 3 years ago by
And there is one superfluous import, see the pyflake error.
There is also the coverage to take care of ...
comment:45 Changed 3 years ago by
Commit:  e7e5ca66920654e97f20c9697c63435a7d667aa1 → 57dbd85a39c6e7ef9d48dd0705ba0edf01a82ee9 

Branch pushed to git repo; I updated commit sha1. New commits:
57dbd85  added empty line after warning

comment:46 Changed 3 years ago by
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 nonwhitespace character after the 
(or *
) in a list.
comment:47 Changed 3 years ago by
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 3 years ago by
Commit:  57dbd85a39c6e7ef9d48dd0705ba0edf01a82ee9 → c589388078516b0f2933004bef9b908c70e065f6 

Branch pushed to git repo; I updated commit sha1. New commits:
c589388  spacing and coverage fix

comment:49 Changed 3 years ago by
Commit:  c589388078516b0f2933004bef9b908c70e065f6 → d8886fc473a59172a8f6885653b6465a916b74df 

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

comment:50 Changed 3 years ago by
and some failing doctests, one missing optional and some misplaced optional
comment:51 Changed 3 years ago by
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 3 years ago by
Commit:  d8886fc473a59172a8f6885653b6465a916b74df → 60c2e8a7388e78907194724bf7de94a879bbb877 

Branch pushed to git repo; I updated commit sha1. New commits:
60c2e8a  moving backticks

comment:53 Changed 3 years ago by
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 3 years ago by
Commit:  60c2e8a7388e78907194724bf7de94a879bbb877 → f809935738a710718021557ebd15da2844ebaa01 

comment:55 Changed 3 years ago by
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.
comment:57 Changed 3 years ago by
We're on it! :) The semester starts today, and we'll have more time to look into sage.
comment:58 Changed 3 years ago by
Commit:  f809935738a710718021557ebd15da2844ebaa01 → 4786032cd5a0338006b25a3143b6ce67c3f64fbf 

comment:59 Changed 3 years ago by
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:61 Changed 3 years ago by
Commit:  4786032cd5a0338006b25a3143b6ce67c3f64fbf → b373a3b3447f3c17ef8caaecb0aa349e60d1e091 

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

comment:62 Changed 3 years ago by
Reviewers:  JeanPhilippe Labbé → JeanPhilippe Labbé, Frédéric Chapoton 

Status:  needs_review → positive_review 
comment:63 Changed 3 years ago by
Milestone:  sage8.9 → sage9.0 

comment:64 Changed 3 years ago by
Branch:  public/18957 → b373a3b3447f3c17ef8caaecb0aa349e60d1e091 

Resolution:  → fixed 
Status:  positive_review → closed 
This should also be done through the backend
normaliz
with the version coming after 3.5.3. The call to get theehrhart_polynomial
in normaliz will be facilitated and would be nice to have too.The upgrade to normaliz 3.5.3 is here: #22984.