Opened 6 years ago
Closed 6 years ago
#18812 closed enhancement (fixed)
latte_int: count integer points
Reported by: | ncohen | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.8 |
Component: | geometry | Keywords: | |
Cc: | dimpase, mkoeppe, vdelecroix, vbraun | Merged in: | |
Authors: | Nathann Cohen | Reviewers: | Jeroen Demeyer, Dima Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | aa93226 (Commits) | Commit: | aa93226e7408c22c7e6ce3664274eeb9e82a8e85 |
Dependencies: | #18887 | Stopgaps: |
Description
Exposes the counting of the integer points of a polytope.. Mostly a copy/paste from ehrhart_polynomial
.
This branch also adds a file_output
optional flag to cdd_Hrepresentation
and cdd_Vrepresentation
. The functions calling external executable already contain sufficiently non-mathematical code.
Nathann
Change History (58)
comment:1 Changed 6 years ago by
- Branch set to public/18812
- Commit set to 62b4d8dcc65fc2f7ec558939198d79d1c4c50478
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 6 years ago by
- Status changed from needs_review to needs_work
I don't think that printing is the normal way to communicate errors:
if not verbose: print err
comment:3 in reply to: ↑ 2 Changed 6 years ago by
I don't think that printing is the normal way to communicate errors:
if not verbose: print err
How do you think it should be done? The next line raises an exception.
Nathann
comment:4 Changed 6 years ago by
Just put the error in the exception message...
raise RuntimeError("...\n" + err)
comment:5 Changed 6 years ago by
- Commit changed from 62b4d8dcc65fc2f7ec558939198d79d1c4c50478 to 28ad77b9babd48ecee6e9026d2311aa944574fe4
comment:6 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:7 follow-up: ↓ 8 Changed 6 years ago by
I don't think we should add a file_output
argument to the cdd_Hrepresentation()
function (are you going to add such an argument to every function returning a string?)
Can't latte_int
read the problem from stdin?
comment:8 in reply to: ↑ 7 ; follow-ups: ↓ 11 ↓ 20 Changed 6 years ago by
I don't think we should add a
file_output
argument to thecdd_Hrepresentation()
function
It makes life easier, and it seems that this file format is standard. Why would you oppose having a way to store a polytope into a file?
Or do you want .write_cdd_Hrepresentation
and .write_cdd_Vrepresentation
instead?
Can't
latte_int
read the problem from stdin?
I don't know. But I think that we should be able to write polytopes to files regardless of whether latte_int
can avoid it.
Nathann
comment:9 follow-up: ↓ 10 Changed 6 years ago by
I also don't understand why one method using latte
is in base.py
and the other in base_ZZ.py
. Why is that? Which polytopes are valid input for count
?
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 33 Changed 6 years ago by
I also don't understand why one method using
latte
is inbase.py
and the other inbase_ZZ.py
. Why is that? Which polytopes are valid input forcount
?
My understanding is that ehrhart_polynomial
is only defined for lattice polytopes. So you need an explicit "lattice", and this lattice is ZZ^d
.
Nathann
comment:11 in reply to: ↑ 8 ; follow-up: ↓ 12 Changed 6 years ago by
Replying to ncohen:
Why would you oppose having a way to store a polytope into a file?
Making simple code more complicated. Writing to a file can already be done using
with open(filename, 'w') as f: f.write(whatever)
I don't see the advantage of the file_input
argument here.
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 13 Changed 6 years ago by
I don't see the advantage of the
file_input
argument here.
I want a way to export a Sage object (here, a polyhedron) to a file format that is understood by other libraries.
Nathann
comment:13 in reply to: ↑ 12 Changed 6 years ago by
Replying to ncohen:
I want a way to export a Sage object (here, a polyhedron) to a file format that is understood by other libraries.
Here is that way:
with open(filename, 'w') as f: f.write(polyhedron.cdd_Hrepresentation())
Really, would you want a file_input
argument to every function returning a string?
comment:14 Changed 6 years ago by
- Status changed from needs_review to needs_work
comment:15 Changed 6 years ago by
- Commit changed from 28ad77b9babd48ecee6e9026d2311aa944574fe4 to 8abf9a9ba49f9f750dac5f338899a4ac7e5c59b0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8abf9a9 | trac #18812: Count integer points
|
comment:16 Changed 6 years ago by
- Status changed from needs_work to needs_review
This is my implementation without temporary files.
comment:17 follow-up: ↓ 21 Changed 6 years ago by
You go on somebody's ticket, set his ticket to needs_work
, set yourself as an author, disregard his opinion bout the code, overwrite his branch with a commit that only bears your name and rewriting the code to your liking?
comment:18 Changed 6 years ago by
- Commit changed from 8abf9a9ba49f9f750dac5f338899a4ac7e5c59b0 to 28ad77b9babd48ecee6e9026d2311aa944574fe4
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
711a249 | trac #18812: fil_output for Hrepresentation and Vrepresentation
|
62b4d8d | trac #18812: Count integer points
|
6f56b74 | trac #18812: The reviewer wants the error reported in a different order
|
28ad77b | trac #18812: missing doc
|
comment:19 follow-up: ↓ 26 Changed 6 years ago by
You are not an author of this ticket, and this is the branch that I propose for review.
You have the choice to refuse it for a valid reason (which you may expose), or to not review it.
Nathann
comment:20 in reply to: ↑ 8 ; follow-up: ↓ 22 Changed 6 years ago by
Replying to ncohen:
Or do you want
.write_cdd_Hrepresentation
and.write_cdd_Vrepresentation
instead?
Even though I still think they are not needed, I have a slight preference for that, yes.
comment:21 in reply to: ↑ 17 Changed 6 years ago by
Replying to ncohen:
You go on somebody's ticket
To review it.
set his ticket to
needs_work
Indeed, there were problems.
set yourself as an author
I add myself as author, yes.
disregard his opinion bout the code
Absolutely not true.
overwrite his branch with a commit that only bears your name
OK, I see the problem here. I can fix this.
rewriting the code to your liking?
Of course, that's what a reviewer should do.
comment:22 in reply to: ↑ 20 ; follow-up: ↓ 25 Changed 6 years ago by
Even though I still think they are not needed, I have a slight preference for that, yes.
What about the names of those functions? I wrote them without thinking, but if we are to keep them around thn perhaps it deserves more thinking about.
Do you prefer a to_cdd_Hrepresentation
or anything else?
Nathann
comment:23 Changed 6 years ago by
- Commit changed from 28ad77b9babd48ecee6e9026d2311aa944574fe4 to a7e407c5e35aae684a6c6157806848259ba2278c
Branch pushed to git repo; I updated commit sha1. New commits:
a7e407c | Simplify executing latte integrale
|
comment:24 Changed 6 years ago by
- Status changed from needs_review to needs_work
overwrite his branch with a commit that only bears your name
Fixed this now.
comment:25 in reply to: ↑ 22 ; follow-up: ↓ 27 Changed 6 years ago by
Replying to ncohen:
Even though I still think they are not needed, I have a slight preference for that, yes.
What about the names of those functions? I wrote them without thinking, but if we are to keep them around then perhaps it deserves more brain time.
Do you prefer a
to_cdd_Hrepresentation
or anything else?
I think your original proposal with write_
was better. I associate that with writing a file.
comment:26 in reply to: ↑ 19 Changed 6 years ago by
Replying to ncohen:
You have the choice to refuse it for a valid reason (which you may expose), or to not review it.
I believe that a reviewer also has the choice to make changes to the proposed code, which is sometimes simpler than explaining which changes should be made.
comment:27 in reply to: ↑ 25 ; follow-up: ↓ 31 Changed 6 years ago by
I think your original proposal with write_ was better. I associate that with writing a file.
Okay. Done.
I believe that a reviewer also has the choice to make changes to the proposed code, which is sometimes simpler than explaining which changes should be made.
As a courtesy to the author, yes. He can fix typos directly, reformat straightforward things, that's way easier than explaining them and saves everybody time.
The reviewer, however, is a counter-power to the author. He is meant to check things that the author could forget or mess-up. But he cannot have "full write access" to the branch either, for the reviewer simply cannot have more power than the author on a ticket.
If I propose changes it is very often because I need to have them inside. If a reviewer comes, makes whatever changes he likes and sets the branch to needs_review
, then it is like I am forced to accept those changes if I want my code to make it in. In this specific case, it was particularly clear that I wanted functions to export polytopes to a file 12. If you disregard that and implement the branch to your liking, then you are basically taking the other feature as a hostage.
So no, the reviewer should not feel free to make non-trivial change to somebody else's ticket. The way I work as a reviewer is to push non-trivial changes to another branch and say to the author: "Here it is and here is what it does, add it to your branch if you agree with them".
Nathann
comment:28 Changed 6 years ago by
- Commit changed from a7e407c5e35aae684a6c6157806848259ba2278c to aa62ab1daaba87ff3f179446b7fbbfc7feb2ab45
Branch pushed to git repo; I updated commit sha1. New commits:
aa62ab1 | trac #18812: independent write_* functions
|
comment:29 Changed 6 years ago by
- Commit changed from aa62ab1daaba87ff3f179446b7fbbfc7feb2ab45 to a609439c3480fb7744b689ee22f73e14c86acb8a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a609439 | trac #18812: independent write_* functions
|
comment:30 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:31 in reply to: ↑ 27 ; follow-up: ↓ 32 Changed 6 years ago by
Replying to ncohen:
The way I work as a reviewer is to push non-trivial changes to another branch and say to the author: "Here it is and here is what it does, add it to your branch if you agree with them".
Modulo the branch name, that's exactly what I did...
comment:32 in reply to: ↑ 31 Changed 6 years ago by
The way I work as a reviewer is to push non-trivial changes to another branch and say to the author: "Here it is and here is what it does, add it to your branch if you agree with them".
Modulo the branch name, that's exactly what I did...
And modulo the fact that you removed a feature that I said I wanted, that you did not ask first, and that you overwrote my branch. That's the difference between asking politely and taking over.
Nathann
comment:33 in reply to: ↑ 10 ; follow-up: ↓ 34 Changed 6 years ago by
Replying to ncohen:
I also don't understand why one method using
latte
is inbase.py
and the other inbase_ZZ.py
. Why is that? Which polytopes are valid input forcount
?My understanding is that
ehrhart_polynomial
is only defined for lattice polytopes. So you need an explicit "lattice", and this lattice isZZ^d
.
A polytope could as well be defined using rational data, and still happen to have integer vertices. So I think it should be made available for general polytopes. If it's not a lattice polytope, LattE will signal an error.
comment:34 in reply to: ↑ 33 Changed 6 years ago by
A polytope could as well be defined using rational data, and still happen to have integer vertices. So I think it should be made available for general polytopes. If it's not a lattice polytope, LattE will signal an error.
If you can manage to make this message clear to the user, then indeed nothing prevents you from moving that other function to base.py
.
comment:35 Changed 6 years ago by
- Status changed from needs_review to needs_work
Hello,
A failing doctest
sage -t --long src/sage/geometry/polyhedron/cdd_file_format.py ********************************************************************** File "src/sage/geometry/polyhedron/cdd_file_format.py", line 110, in sage.geometry.polyhedron.cdd_file_format.cdd_Hrepresentation Failed example: cdd_Vrepresentation('rational', [[0,0]], [[1,0]], [[0,1]], file_output=filename) Exception raised: Traceback (most recent call last): ... NameError: name 'cdd_Vrepresentation' is not defined **********************************************************************
comment:36 Changed 6 years ago by
... not sure it is still up to date since it was not run on the last commit.
comment:37 Changed 6 years ago by
- Commit changed from a609439c3480fb7744b689ee22f73e14c86acb8a to ca2a01b4874a3c65b310ed06eb2c838dfe5b36fa
Branch pushed to git repo; I updated commit sha1. New commits:
ca2a01b | trac #18812: Broken doctest
|
comment:38 Changed 6 years ago by
- Status changed from needs_work to needs_review
Sorry, stupid copy/paste ^^;
comment:39 follow-up: ↓ 41 Changed 6 years ago by
Some comments:
- The correct capitalization of the name of the software is: "LattE integrale"
- typos in docstrings: "repersentation" -> "representation"
I think that Sage should refuse to write a cdd-style format with floats or whatever other numbers are tried, and raise a meaningful exception, rather than just having LattE complain about the input format.
comment:40 Changed 6 years ago by
- Commit changed from ca2a01b4874a3c65b310ed06eb2c838dfe5b36fa to 41b9d92284e218018f5b41d043ced7bc4f767e51
Branch pushed to git repo; I updated commit sha1. New commits:
41b9d92 | trac #18812: review
|
comment:41 in reply to: ↑ 39 Changed 6 years ago by
- The correct capitalization of the name of the software is: "LattE integrale"
- typos in docstrings: "repersentation" -> "representation"
Done.
comment:42 Changed 6 years ago by
- Status changed from needs_review to needs_work
You should avoid
tmp_filename() + '.ext'
Instead, use
tmp_filename(ext='.ext')
comment:43 Changed 6 years ago by
Given that we now have write_cdd_{H|V}representation()
, can the file_output
arguments in src/sage/geometry/polyhedron/cdd_file_format.py
be removed?
comment:44 Changed 6 years ago by
In the cdd_Hrepresentation()
method docstring, please do not remove the OUTPUT: a string
block.
comment:45 Changed 6 years ago by
- Commit changed from 41b9d92284e218018f5b41d043ced7bc4f767e51 to aa93226e7408c22c7e6ce3664274eeb9e82a8e85
comment:46 follow-up: ↓ 47 Changed 6 years ago by
- Status changed from needs_work to needs_review
I implemented your two remarks. About the file_output
in cdd_file_format
: right now the two higher-level functions call it, so it is used in that sense. It also makes sense for such a feature to be available in a file called 'cdd_file_format`. Furthermore, the functions that this file contains do not take a polyhedron object, but rather lists of vertices/inequalities: this way you can write this data to a file with the very same information you need to get the string. No need to create a higher-level object.
Nathann
comment:47 in reply to: ↑ 46 ; follow-up: ↓ 48 Changed 6 years ago by
Replying to ncohen:
I implemented your two remarks. About the
file_output
incdd_file_format
: right now the two higher-level functions call it, so it is used in that sense.
Well, the file_output
argument is not used...
It also makes sense for such a feature to be available in a file called 'cdd_file_format`.
Why can't cdd_file_format
just be a low-level interface? I don't see why we should complicate those functions with a file_output
argument. Especially since the user can simply use
with open(...) as f: f.write(cdd_Hrepresentation(...))
comment:48 in reply to: ↑ 47 Changed 6 years ago by
Well, the
file_output
argument is not used...
What do you mean by that? In those two functions, the file_output
argument is used to write the file.
Nathann
comment:49 Changed 6 years ago by
NTL update in 6.8.beta8 has broken building of latte_int. It fails, saying that NTL is not found...
comment:50 Changed 6 years ago by
In that case you might want to note #18875: Update NTL to 9.3.0
comment:51 Changed 6 years ago by
- Status changed from needs_review to needs_work
comment:52 follow-up: ↓ 53 Changed 6 years ago by
What exactly do you want me to change on this ticket? If the problem is elsewhere, wouldn't it be more appropriate to leave this ticket in needs_review
and make it depend on the fix to the problem Dima mentionned?
Nathann
comment:53 in reply to: ↑ 52 Changed 6 years ago by
- Status changed from needs_work to needs_review
Replying to ncohen:
What exactly do you want me to change on this ticket? If the problem is elsewhere, wouldn't it be more appropriate to leave this ticket in
needs_review
and make it depend on the fix to the problem Dima mentionned?
OK, fine.
comment:54 Changed 6 years ago by
- Dependencies set to #18887
I suppose it has to wait till #18887...
comment:56 Changed 6 years ago by
Thanks !
comment:57 Changed 6 years ago by
- Reviewers set to Jeroen Demeyer, Dima Pasechnik
comment:58 Changed 6 years ago by
- Branch changed from public/18812 to aa93226e7408c22c7e6ce3664274eeb9e82a8e85
- Resolution set to fixed
- Status changed from positive_review to closed
Volker: if you have a spare second, could you document the arguments of
cdd_Hrepresentation
andcdd_Vrepresentation
incdd_file_format.py
? I do not know exactly what assumptions are made on their format.THaaaaaaaaaanks,
Nathann
New commits:
trac #18812: fil_output for Hrepresentation and Vrepresentation
trac #18812: Count integer points