Opened 4 years ago

Closed 4 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 4 years ago by ncohen

  • Branch set to public/18812
  • Commit set to 62b4d8dcc65fc2f7ec558939198d79d1c4c50478
  • Status changed from new to needs_review

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:

711a249trac #18812: fil_output for Hrepresentation and Vrepresentation
62b4d8dtrac #18812: Count integer points

comment:2 follow-up: Changed 4 years ago by jdemeyer

  • 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 4 years ago by 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

comment:4 Changed 4 years ago by jdemeyer

Just put the error in the exception message...

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

comment:5 Changed 4 years ago by git

  • Commit changed from 62b4d8dcc65fc2f7ec558939198d79d1c4c50478 to 28ad77b9babd48ecee6e9026d2311aa944574fe4

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

6f56b74trac #18812: The reviewer wants the error reported in a different order
28ad77btrac #18812: missing doc

comment:6 Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:7 follow-up: Changed 4 years ago by 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?

comment:8 in reply to: ↑ 7 ; follow-ups: Changed 4 years ago by 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

comment:9 follow-up: Changed 4 years ago by 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?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by 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

comment:11 in reply to: ↑ 8 ; follow-up: Changed 4 years ago by 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.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by 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

comment:13 in reply to: ↑ 12 Changed 4 years ago by 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?

comment:14 Changed 4 years ago by jdemeyer

  • Authors changed from Nathann Cohen to Nathann Cohen, Jeroen Demeyer
  • Status changed from needs_review to needs_work

comment:15 Changed 4 years ago by git

  • Commit changed from 28ad77b9babd48ecee6e9026d2311aa944574fe4 to 8abf9a9ba49f9f750dac5f338899a4ac7e5c59b0

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

8abf9a9trac #18812: Count integer points

comment:16 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

This is my implementation without temporary files.

comment:17 follow-up: Changed 4 years ago by 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?

comment:18 Changed 4 years ago by git

  • Commit changed from 8abf9a9ba49f9f750dac5f338899a4ac7e5c59b0 to 28ad77b9babd48ecee6e9026d2311aa944574fe4

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

711a249trac #18812: fil_output for Hrepresentation and Vrepresentation
62b4d8dtrac #18812: Count integer points
6f56b74trac #18812: The reviewer wants the error reported in a different order
28ad77btrac #18812: missing doc

comment:19 follow-up: Changed 4 years ago by ncohen

  • Authors changed from Nathann Cohen, Jeroen Demeyer to Nathann Cohen

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: Changed 4 years ago by 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.

comment:21 in reply to: ↑ 17 Changed 4 years ago by 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.

comment:22 in reply to: ↑ 20 ; follow-up: Changed 4 years ago by 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?

Nathann

Last edited 4 years ago by ncohen (previous) (diff)

comment:23 Changed 4 years ago by git

  • Commit changed from 28ad77b9babd48ecee6e9026d2311aa944574fe4 to a7e407c5e35aae684a6c6157806848259ba2278c

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

a7e407cSimplify executing latte integrale

comment:24 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

overwrite his branch with a commit that only bears your name

Fixed this now.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:25 in reply to: ↑ 22 ; follow-up: Changed 4 years ago by 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.

comment:26 in reply to: ↑ 19 Changed 4 years ago by 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.

comment:27 in reply to: ↑ 25 ; follow-up: Changed 4 years ago by 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

comment:28 Changed 4 years ago by git

  • Commit changed from a7e407c5e35aae684a6c6157806848259ba2278c to aa62ab1daaba87ff3f179446b7fbbfc7feb2ab45

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

aa62ab1trac #18812: independent write_* functions

comment:29 Changed 4 years ago by git

  • Commit changed from aa62ab1daaba87ff3f179446b7fbbfc7feb2ab45 to a609439c3480fb7744b689ee22f73e14c86acb8a

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

a609439trac #18812: independent write_* functions

comment:30 Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:31 in reply to: ↑ 27 ; follow-up: Changed 4 years ago by 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...

comment:32 in reply to: ↑ 31 Changed 4 years ago by 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

comment:33 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by 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.

comment:34 in reply to: ↑ 33 Changed 4 years ago by 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.

comment:35 Changed 4 years ago by vdelecroix

  • 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 4 years ago by vdelecroix

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

comment:37 Changed 4 years ago by git

  • Commit changed from a609439c3480fb7744b689ee22f73e14c86acb8a to ca2a01b4874a3c65b310ed06eb2c838dfe5b36fa

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

ca2a01btrac #18812: Broken doctest

comment:38 Changed 4 years ago by ncohen

  • Status changed from needs_work to needs_review

Sorry, stupid copy/paste ^^;

comment:39 follow-up: Changed 4 years ago by 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.

comment:40 Changed 4 years ago by git

  • Commit changed from ca2a01b4874a3c65b310ed06eb2c838dfe5b36fa to 41b9d92284e218018f5b41d043ced7bc4f767e51

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

41b9d92trac #18812: review

comment:41 in reply to: ↑ 39 Changed 4 years ago by ncohen

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

Done.

comment:42 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

You should avoid

tmp_filename() + '.ext'

Instead, use

tmp_filename(ext='.ext')

comment:43 Changed 4 years ago by 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?

comment:44 Changed 4 years ago by jdemeyer

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

comment:45 Changed 4 years ago by git

  • Commit changed from 41b9d92284e218018f5b41d043ced7bc4f767e51 to aa93226e7408c22c7e6ce3664274eeb9e82a8e85

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

0cc26c9trac #18812: concatenation
aa93226trac #18812: output block

comment:46 follow-up: Changed 4 years ago by ncohen

  • 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: Changed 4 years ago by 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(...))

comment:48 in reply to: ↑ 47 Changed 4 years ago by 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

comment:49 Changed 4 years ago by dimpase

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

Last edited 4 years ago by dimpase (previous) (diff)

comment:50 Changed 4 years ago by vbraun

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

comment:51 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:52 follow-up: Changed 4 years ago by 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

comment:53 in reply to: ↑ 52 Changed 4 years ago by jdemeyer

  • 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 4 years ago by dimpase

  • Dependencies set to #18887

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

comment:55 Changed 4 years ago by dimpase

  • Status changed from needs_review to positive_review

LGTM

comment:56 Changed 4 years ago by ncohen

Thanks !

comment:57 Changed 4 years ago by dimpase

  • Reviewers set to Jeroen Demeyer, Dima Pasechnik

comment:58 Changed 4 years ago by vbraun

  • Branch changed from public/18812 to aa93226e7408c22c7e6ce3664274eeb9e82a8e85
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.