latte_int: count integer points

[ncohen]

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

[ncohen]

Volker: if you have a spare second, could you document the arguments of cdd_Hrepresentation and cdd_Vrepresentation in cdd_file_format.py? I do not know exactly what assumptions are made on their format.

THaaaaaaaaaanks,

Nathann

---

New commits:

​711a249	trac #18812: fil_output for Hrepresentation and Vrepresentation
​62b4d8d	trac #18812: Count integer points

[jdemeyer]

I don't think that printing is the normal way to communicate errors:

if not verbose:
    print err

[ncohen]

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

[jdemeyer]

Just put the error in the exception message...

raise RuntimeError("...\n" + err)

[git]

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

​6f56b74	trac #18812: The reviewer wants the error reported in a different order
​28ad77b	trac #18812: missing doc

[jdemeyer]

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?

[ncohen]

I don't think we should add a file_output argument to the cdd_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

[jdemeyer]

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?

[ncohen]

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?

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

[jdemeyer]

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.

[ncohen]

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

[jdemeyer]

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?

[git]

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

​8abf9a9

trac #18812: Count integer points

[jdemeyer]

This is my implementation without temporary files.

[ncohen]

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?

[git]

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

[ncohen]

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

[jdemeyer]

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.

[jdemeyer]

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.

[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 thn perhaps it deserves more thinking about.

Do you prefer a to_cdd_Hrepresentation or anything else?

Nathann

[git]

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

​a7e407c

Simplify executing latte integrale

[jdemeyer]

overwrite his branch with a commit that only bears your name

Fixed this now.

[jdemeyer]

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.

[jdemeyer]

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.

[ncohen]

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

[git]

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

​aa62ab1

trac #18812: independent write_* functions

[git]

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

​a609439

trac #18812: independent write_* functions

[jdemeyer]

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

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

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

[mkoeppe]

Replying to ncohen:

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?

My understanding is that ehrhart_polynomial is only defined for lattice polytopes. So you need an explicit "lattice", and this lattice is ZZ^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.

[ncohen]

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.

[vdelecroix]

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

[vdelecroix]

... not sure it is still up to date since it was not run on the last commit.

[git]

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

​ca2a01b

trac #18812: Broken doctest

[ncohen]

Sorry, stupid copy/paste ^^;

[mkoeppe]

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.

[git]

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

​41b9d92

trac #18812: review

[ncohen]

• The correct capitalization of the name of the software is: "LattE integrale"
• typos in docstrings: "repersentation" -> "representation"

Done.

[jdemeyer]

You should avoid

tmp_filename() + '.ext'

Instead, use

tmp_filename(ext='.ext')

[jdemeyer]

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?

[jdemeyer]

In the cdd_Hrepresentation() method docstring, please do not remove the OUTPUT: a string block.

[git]

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

​0cc26c9	trac #18812: concatenation
​aa93226	trac #18812: output block

[ncohen]

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

[jdemeyer]

Replying to ncohen:

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.

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

[ncohen]

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

[dimpase]

NTL update in 6.8.beta8 has broken building of latte_int. It fails, saying that NTL is not found...

[vbraun]

In that case you might want to note #18875: Update NTL to 9.3.0

[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?

Nathann

[jdemeyer]

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.

[dimpase]

I suppose it has to wait till #18887...

[dimpase]

LGTM

[ncohen]

Thanks !