Opened 6 years ago
Closed 2 years ago
#18957 closed enhancement (fixed)
ehrhart_polynomial should be made available for polytopes defined over QQ
Reported by:  mkoeppe  Owned by:  

Priority:  minor  Milestone:  sage9.0 
Component:  geometry  Keywords:  latte, days100 
Cc:  jipilab, selia  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 4 years ago by
 Cc jipilab added
comment:2 Changed 4 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 selia added
comment:5 Changed 3 years ago by
 Branch set to public/18957
 Commit set to d35b4e120adb96361aacd7632587060583588d5c
 Keywords days100 added
comment:6 Changed 3 years ago by
 Commit changed from d35b4e120adb96361aacd7632587060583588d5c to c378edbbee41aece5b618ed5b78dbfe24c85db17
Branch pushed to git repo; I updated commit sha1. New commits:
c378edb  more changes

comment:7 Changed 3 years ago by
 Commit changed from c378edbbee41aece5b618ed5b78dbfe24c85db17 to 50a644145c216f1c531be01f82f43724ef574bb3
Branch pushed to git repo; I updated commit sha1. New commits:
50a6441  fixed typos

comment:8 Changed 3 years ago by
 Commit changed from 50a644145c216f1c531be01f82f43724ef574bb3 to 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 changed from f65b2d9f92d8a69be35cb748c5c708adc70bef5e to fec936b7fdd83cb3f803a08ed7dc09923543dc26
Branch pushed to git repo; I updated commit sha1. New commits:
fec936b  adapt base_ZZ

comment:11 Changed 3 years ago by
 Commit changed from fec936b7fdd83cb3f803a08ed7dc09923543dc26 to 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 changed from a561fe629585bd8f53e9dce479acb449966ad928 to 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 changed from 1a5aa83c815cfce4655a2012158502c75b92489b to 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 changed from bc646b0490bfd4fb3aa3a32f84367c1071b6288a to 9bb89f80b85a564ad01475a344ab76cd6713cca1
Branch pushed to git repo; I updated commit sha1. New commits:
9bb89f8  changed error message

comment:15 Changed 3 years ago by
 Dependencies set to #28235
comment:16 Changed 3 years ago by
 Commit changed from 9bb89f80b85a564ad01475a344ab76cd6713cca1 to 4a9a35eedd34c3632048dad3d41b610788b27bf3
comment:17 Changed 3 years ago by
 Commit changed from 4a9a35eedd34c3632048dad3d41b610788b27bf3 to 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 changed from sage6.9 to sage8.9
comment:19 Changed 3 years ago by
 Commit changed from 0460583fa008fbfe59e4a85836b3dfbe18fc06da to 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 changed from 77c9ec8e3f83baba4e8ece88003aaccfdd25338c to 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 changed from 5e97d90f3da4f24ffca482eede5141966db9a9ef to 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 changed from 2c214b422079560d30d620d68dd85fc55d08c0e6 to 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 2 years ago by
 Commit changed from 6af886110bd323070b020b82289ea48c69c7d07f to 0f245b394d3514494bcce58beed09edbca447070
Branch pushed to git repo; I updated commit sha1. New commits:
0f245b3  base_QQ doc work

comment:26 Changed 2 years ago by
 Commit changed from 0f245b394d3514494bcce58beed09edbca447070 to c7cb6d648e4611f96b0206e99b6e3c24638f7d09
Branch pushed to git repo; I updated commit sha1. New commits:
c7cb6d6  spacing

comment:27 Changed 2 years ago by
 Commit changed from c7cb6d648e4611f96b0206e99b6e3c24638f7d09 to be46cbc8eddcb8fdfc24e23aebf74667195acd66
Branch pushed to git repo; I updated commit sha1. New commits:
be46cbc  added one more example to base_QQ

comment:28 Changed 2 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 2 years ago by
 Commit changed from be46cbc8eddcb8fdfc24e23aebf74667195acd66 to d08171b3aa70ebc0637547ddc0d72b9345255d1a
Branch pushed to git repo; I updated commit sha1. New commits:
d08171b  fixing formatting to fit conventions

comment:30 Changed 2 years ago by
ready for review ?
comment:31 Changed 2 years ago by
The branch is red, and the required rebase on the latest develop branch is not trivial.
comment:32 Changed 2 years ago by
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 2 years ago by
 Commit changed from d08171b3aa70ebc0637547ddc0d72b9345255d1a to 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 2 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 2 years ago by
 Commit changed from 08a5d67ec1115d0a9f901b65837a7c29a26e0c39 to 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 2 years ago by
 Reviewers set to JeanPhilippe Labbé
 Status changed from new to needs_review
comment:37 Changed 2 years ago by
 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/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 2 years ago by
One more thing: we should consider making some of these methods cached so as to not recompute everything every time...
comment:39 Changed 2 years ago by
empty TESTS section should be removed:
+ TESTS:: + """
comment:40 Changed 2 years ago by
 Commit changed from db84f51bcdf50fb92ad616bb18cc2626f0aff782 to 6e34f903561eff2025b3403b77838f7449ce6836
Branch pushed to git repo; I updated commit sha1. New commits:
6e34f90  fixing documentation

comment:41 Changed 2 years ago by
 Commit changed from 6e34f903561eff2025b3403b77838f7449ce6836 to e7e5ca66920654e97f20c9697c63435a7d667aa1
Branch pushed to git repo; I updated commit sha1. New commits:
e7e5ca6  pyflakes

comment:42 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:43 Changed 2 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 2 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 2 years ago by
 Commit changed from e7e5ca66920654e97f20c9697c63435a7d667aa1 to 57dbd85a39c6e7ef9d48dd0705ba0edf01a82ee9
Branch pushed to git repo; I updated commit sha1. New commits:
57dbd85  added empty line after warning

comment:46 Changed 2 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 2 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 2 years ago by
 Commit changed from 57dbd85a39c6e7ef9d48dd0705ba0edf01a82ee9 to c589388078516b0f2933004bef9b908c70e065f6
Branch pushed to git repo; I updated commit sha1. New commits:
c589388  spacing and coverage fix

comment:49 Changed 2 years ago by
 Commit changed from c589388078516b0f2933004bef9b908c70e065f6 to d8886fc473a59172a8f6885653b6465a916b74df
Branch pushed to git repo; I updated commit sha1. New commits:
d8886fc  removed backticks

comment:50 Changed 2 years ago by
and some failing doctests, one missing optional and some misplaced optional
comment:51 Changed 2 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 2 years ago by
 Commit changed from d8886fc473a59172a8f6885653b6465a916b74df to 60c2e8a7388e78907194724bf7de94a879bbb877
Branch pushed to git repo; I updated commit sha1. New commits:
60c2e8a  moving backticks

comment:53 Changed 2 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 2 years ago by
 Commit changed from 60c2e8a7388e78907194724bf7de94a879bbb877 to f809935738a710718021557ebd15da2844ebaa01
comment:55 Changed 2 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:56 Changed 2 years ago by
one last effort is required..
comment:57 Changed 2 years ago by
We're on it! :) The semester starts today, and we'll have more time to look into sage.
comment:58 Changed 2 years ago by
 Commit changed from f809935738a710718021557ebd15da2844ebaa01 to 4786032cd5a0338006b25a3143b6ce67c3f64fbf
comment:59 Changed 2 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:60 Changed 2 years ago by
typo "integeral"
sinon, c'est bon : positive review
comment:61 Changed 2 years ago by
 Commit changed from 4786032cd5a0338006b25a3143b6ce67c3f64fbf to b373a3b3447f3c17ef8caaecb0aa349e60d1e091
Branch pushed to git repo; I updated commit sha1. New commits:
b373a3b  typo

comment:62 Changed 2 years ago by
 Reviewers changed from JeanPhilippe Labbé to JeanPhilippe Labbé, Frédéric Chapoton
 Status changed from needs_review to positive_review
comment:63 Changed 2 years ago by
 Milestone changed from sage8.9 to sage9.0
comment:64 Changed 2 years ago by
 Branch changed from public/18957 to b373a3b3447f3c17ef8caaecb0aa349e60d1e091
 Resolution set to fixed
 Status changed from positive_review to 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.