Opened 6 years ago
Closed 17 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, 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 3 years ago by
- Cc jipilab added
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
Normaliz and PyNormaliz? have the computation goal EhrhartQuasipolynomial? since Normaliz version 3.6.0.
comment:4 Changed 20 months ago by
- Cc selia added
comment:5 Changed 20 months ago by
- Branch set to public/18957
- Commit set to d35b4e120adb96361aacd7632587060583588d5c
- Keywords days100 added
comment:6 Changed 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months ago by
- Dependencies set to #28235
comment:16 Changed 20 months ago by
- Commit changed from 9bb89f80b85a564ad01475a344ab76cd6713cca1 to 4a9a35eedd34c3632048dad3d41b610788b27bf3
comment:17 Changed 20 months 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 20 months ago by
- Milestone changed from sage-6.9 to sage-8.9
comment:19 Changed 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months ago by
- Commit changed from 0f245b394d3514494bcce58beed09edbca447070 to c7cb6d648e4611f96b0206e99b6e3c24638f7d09
Branch pushed to git repo; I updated commit sha1. New commits:
c7cb6d6 | spacing
|
comment:27 Changed 20 months 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 19 months 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 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')
comment:29 Changed 19 months 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 19 months ago by
ready for review ?
comment:31 Changed 19 months ago by
The branch is red, and the required rebase on the latest develop branch is not trivial.
comment:32 Changed 19 months 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 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 19 months ago by
- Commit changed from d08171b3aa70ebc0637547ddc0d72b9345255d1a to 08a5d67ec1115d0a9f901b65837a7c29a26e0c39
Branch pushed to git repo; I updated commit sha1. New commits:
08a5d67 | added to-dos for changing polynomial via new polynomial ring
|
comment:34 Changed 19 months 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 18 months 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 18 months ago by
- Reviewers set to Jean-Philippe Labbé
- Status changed from new to needs_review
comment:37 Changed 18 months 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/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 18 months 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 18 months ago by
empty TESTS section should be removed:
+ TESTS:: + """
comment:40 Changed 18 months 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 18 months ago by
- Commit changed from 6e34f903561eff2025b3403b77838f7449ce6836 to e7e5ca66920654e97f20c9697c63435a7d667aa1
Branch pushed to git repo; I updated commit sha1. New commits:
e7e5ca6 | pyflakes
|
comment:42 Changed 18 months ago by
- Status changed from needs_work to needs_review
comment:43 Changed 18 months ago by
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 18 months ago by
And there is one superfluous import, see the pyflake error.
There is also the coverage to take care of ...
comment:45 Changed 18 months 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 18 months 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 non-whitespace character after the -
(or *
) in a list.
comment:47 Changed 18 months 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 18 months 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 18 months 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 18 months ago by
and some failing doctests, one missing optional and some misplaced optional
comment:51 Changed 18 months 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 18 months 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 18 months 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 17 months ago by
- Commit changed from 60c2e8a7388e78907194724bf7de94a879bbb877 to f809935738a710718021557ebd15da2844ebaa01
comment:55 Changed 17 months 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 17 months ago by
one last effort is required..
comment:57 Changed 17 months ago by
We're on it! :) The semester starts today, and we'll have more time to look into sage.
comment:58 Changed 17 months ago by
- Commit changed from f809935738a710718021557ebd15da2844ebaa01 to 4786032cd5a0338006b25a3143b6ce67c3f64fbf
comment:59 Changed 17 months 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 17 months ago by
typo "integeral"
sinon, c'est bon : positive review
comment:61 Changed 17 months ago by
- Commit changed from 4786032cd5a0338006b25a3143b6ce67c3f64fbf to b373a3b3447f3c17ef8caaecb0aa349e60d1e091
Branch pushed to git repo; I updated commit sha1. New commits:
b373a3b | typo
|
comment:62 Changed 17 months ago by
- 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 17 months ago by
- Milestone changed from sage-8.9 to sage-9.0
comment:64 Changed 17 months 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.