Opened 2 years ago

Closed 14 months ago

#25091 closed enhancement (fixed)

Expose some normaliz features

Reported by: jipilab Owned by:
Priority: major Milestone: sage-8.8
Component: geometry Keywords: polytope, normaliz, IMA-PolyGeom
Cc: vdelecroix, moritz, mkoeppe, gh-sebasguts, Winfried, gh-braunmath, tscrim Merged in:
Authors: Jean-Philippe Labbé, Matthias Koeppe Reviewers: Travis Scrimshaw, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: da05706 (Commits) Commit: da057067814b4a5adbc1e77ce0fc9dcc8f0dee83
Dependencies: #27682, #27716 Stopgaps:

Description (last modified by jipilab)

Using Normaliz can compute several things that are currently not interfaced. This ticket implements:

  • Integral points generators
  • Euclidean volume and volume
  • Triangulation
  • Hilbert series
  • Ehrhart series of compact polyhedron
  • Ehrhart quasi-polynomial of compact polyhedron

FOLLOWUP:

  • Make ehrhart_polynomial uniform (in the spirit of .volume() that can have several engines.
  • Move the lattice related methods to Polyhedra_normaliz_QQ.

Change History (123)

comment:1 Changed 2 years ago by jipilab

  • Branch set to u/jipilab/normaliz_features
  • Commit set to e421ff89087f9d231056f163885b5ce251ed5d3b
  • Dependencies set to #22984, #25090

Last 10 new commits:

737552dUpdating patch with upstream fix for wrong number of lattice points.
530302dMerge branch 'u/tscrim/upgrade_noramliz_pynormaliz-22984' of git://trac.sagemath.org/sage into u/tscrim/upgrade_noramliz_pynormaliz-22984
57d40b0Upgrade Normaliz to 3.5.2.
6ba442dAdding tests from comment:24,25 of #22984.
183087aUpgrade PyNormaliz to 1.12
7f47ec9Adapted the polyhedron docstring
117e428Upgrade normaliz to 3.5.3
7057ef4Merge branch to get docstring adaptation
1b37e8bMerge branch 'develop' into test_normaliz
e421ff8First version of integral pts gen

comment:2 Changed 2 years ago by git

  • Commit changed from e421ff89087f9d231056f163885b5ce251ed5d3b to ac35212bdcb4b4671defd9e25ab90634d13f118d

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

b6ae686Merge branch 'develop' of git://trac.sagemath.org/sage into t/25090/pynormaliz2
eaedcc2Update PyNormaliz to 1.14
ac35212Merge branch 'normaliz2' into normaliz_features

comment:3 Changed 2 years ago by jipilab

  • Branch changed from u/jipilab/normaliz_features to u/jipilab/some_normaliz_features
  • Commit changed from ac35212bdcb4b4671defd9e25ab90634d13f118d to bf8b172f1a58a6d71b857575b00dd19b82f9d2e4

New commits:

bf8b172Merge branch 'u/mkoeppe/pynormaliz2'

comment:4 Changed 2 years ago by jipilab

  • Branch changed from u/jipilab/some_normaliz_features to public/some_normaliz_features
  • Commit changed from bf8b172f1a58a6d71b857575b00dd19b82f9d2e4 to 9a6af25046563f288b4c93e61f0e3ab1fa593bf0

New commits:

4dbdc7dUpdated the Cone calls format
0257596Merge branch 'pynormaliz2' into normaliz_features
9a6af25Added volume, first version

comment:5 Changed 2 years ago by git

  • Commit changed from 9a6af25046563f288b4c93e61f0e3ab1fa593bf0 to 3d3f64ab940e973a79557a37ddb7bb346b6b5898

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

3d3f64aHandle default behavior

comment:6 Changed 2 years ago by git

  • Commit changed from 3d3f64ab940e973a79557a37ddb7bb346b6b5898 to f376748a6c5330ea2f23d5e82de289ed7a575433

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

1dcef4c_init_from_normaliz_data: New, use it in _init_from_*representation
fcd8101Updated pynormaliz to 1.15
f376748Merge branch 'pynormaliz2' into normaliz_features

comment:7 follow-up: Changed 2 years ago by vdelecroix

Note that is_package_installed is being deprecated, see #20382

comment:8 Changed 2 years ago by git

  • Commit changed from f376748a6c5330ea2f23d5e82de289ed7a575433 to 55c9a04b9865076dc03dc3d701237ddaf5ee9614

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

55c9a04Added Hilbert series

comment:9 Changed 2 years ago by git

  • Commit changed from 55c9a04b9865076dc03dc3d701237ddaf5ee9614 to 720dc046b8831170f06fe341bdfa42fefeb240c4

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

720dc04Made tests pass

comment:10 Changed 2 years ago by git

  • Commit changed from 720dc046b8831170f06fe341bdfa42fefeb240c4 to a4ddeac32690b8be6a5edf66ff77979a1e9a2b3f

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

a4ddeacCropped feature methods

comment:11 Changed 2 years ago by git

  • Commit changed from a4ddeac32690b8be6a5edf66ff77979a1e9a2b3f to 44549fa150a08fa327c7ae34f322bf4927b2eeec

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

19d8539Polyhedron_normaliz: In verbose mode, write out normaliz format files
4cfcd14_normaliz_format: Fix last change
da8ae7aAdded data and cone methods to normaliz backend
44549faMerge branch 'pynormaliz2' into normaliz_features

comment:12 Changed 2 years ago by git

  • Commit changed from 44549fa150a08fa327c7ae34f322bf4927b2eeec to e32c9fddc39dd6780fea791796c5a536ecf4d42c

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

e32c9fdAdded triangulate with normaliz

comment:13 Changed 2 years ago by jipilab

  • Description modified (diff)

comment:14 Changed 2 years ago by git

  • Commit changed from e32c9fddc39dd6780fea791796c5a536ecf4d42c to 45c39b85af9485489e081b14af08fcde297c4bd5

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

45c39b8Some small edits

comment:15 Changed 2 years ago by git

  • Commit changed from 45c39b85af9485489e081b14af08fcde297c4bd5 to 56798216576de3521fc2276c734cbf6c70f14c62

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

5679821Added handling the triangulation with point configurations

comment:16 Changed 2 years ago by git

  • Commit changed from 56798216576de3521fc2276c734cbf6c70f14c62 to b045caf912268156ab2ea91b9efc425b8510df09

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

b045cafAdapted inhomogeneous case

comment:17 Changed 2 years ago by jipilab

  • Description modified (diff)

comment:18 follow-up: Changed 2 years ago by mkoeppe

It's probably best to add all lattice-point related methods to Polyhedron_QQ_normaliz rather than Polyhedron_normaliz in anticipation of #25097 (which moves integral_points and integral_hull there).

Last edited 2 years ago by mkoeppe (previous) (diff)

comment:19 in reply to: ↑ 18 ; follow-up: Changed 2 years ago by jipilab

Replying to mkoeppe:

It's probably best to add all lattice-point related methods to Polyhedron_QQ_normaliz rather than Polyhedron_normaliz in anticipation of #25097 (which moves integral_points and integral_hull there).

I see. Yes I can do that.

comment:20 in reply to: ↑ 19 Changed 2 years ago by jipilab

Replying to jipilab:

Replying to mkoeppe:

It's probably best to add all lattice-point related methods to Polyhedron_QQ_normaliz rather than Polyhedron_normaliz in anticipation of #25097 (which moves integral_points and integral_hull there).

I see. Yes I can do that.

As discussed with mkoeppe: The move of the rational polyhedron methods will be done in #25097. The merge should go without conflicts.

comment:21 in reply to: ↑ 7 ; follow-up: Changed 2 years ago by jipilab

  • Dependencies changed from #22984, #25090 to #22984, #25090, #20382
  • Description modified (diff)

Replying to vdelecroix:

Note that is_package_installed is being deprecated, see #20382

Oh! Wow! Thanks for the timely warning! I set it as a dependency. I will change the check once it is merged...

comment:22 Changed 2 years ago by git

  • Commit changed from b045caf912268156ab2ea91b9efc425b8510df09 to e7be14a7c0f2b3f2fc13cc2196b6f4cbad3bccdd

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

e7be14aFixed some polytopes and added some tests

comment:23 Changed 2 years ago by git

  • Commit changed from e7be14a7c0f2b3f2fc13cc2196b6f4cbad3bccdd to 3615f8d33d9fb37ca0a905195dd21c8e17a05a7d

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

aa0e2caMerge branch normaliz3.5.3 and pynormaliz1.12 into sage8.2.rc2
2bbd8a6Update normaliz and pynormaliz
82d7190Merged old changes with latest versions
3615f8dMerge branch 'update_py_normaliz' into normaliz_features

comment:24 Changed 2 years ago by git

  • Commit changed from 3615f8d33d9fb37ca0a905195dd21c8e17a05a7d to 8bff81f233fb570e8477db0ea1d898a8a611512d

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

e44cebdMade tests pass for volume
8bff81fFixed the triangulation using normaliz

comment:25 Changed 2 years ago by git

  • Commit changed from 8bff81f233fb570e8477db0ea1d898a8a611512d to d0e2498cf6a3e028c856702c23316e709fe17b00

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

d0e2498Added the Ehrhart Series and Quasi-Polynomial

comment:26 Changed 2 years ago by git

  • Commit changed from d0e2498cf6a3e028c856702c23316e709fe17b00 to 668fdfc3d93a3c567c3258b7d8eaa2c36bcc5400

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

668fdfcAdded an example

comment:27 Changed 2 years ago by jipilab

  • Authors set to Jean-Philippe Labbé
  • Description modified (diff)

comment:28 Changed 2 years ago by jipilab

  • Milestone changed from sage-8.2 to sage-8.4

comment:29 Changed 22 months ago by SimonKing

Out of interest: Given a monomial ideal in Sage, how can one compute its Hilbert series using normaliz?

comment:30 in reply to: ↑ 21 ; follow-up: Changed 22 months ago by SimonKing

Replying to jipilab:

Replying to vdelecroix:

Note that is_package_installed is being deprecated, see #20382

Oh! Wow! Thanks for the timely warning! I set it as a dependency. I will change the check once it is merged...

It is really a pity that #20382 doesn't explain (in the ticket description!!!) what one is supposed to do instead of is_package_installed.

So, how would one change the current branch to make it use #20382?

comment:31 follow-up: Changed 22 months ago by Winfried

Replying to SimonKing?

Normaliz can compute Hilbert series of the integral closures of monomial ideals, not of arbitrary monomial ideals. If one wants to apply it: input the monomials as vertices, the polynomial ring as cone (= unit matrix), and put the weights of the indeterminates into the grading.

Example for Normaliz input (integral closure of (x2, y2, deg x = 2, deg y =1)

amb_space 2
vertices 2
2 0 1
0 2 1
cone 2
1 0
0 1
grading 
2 1
HilbertSeries

comment:32 in reply to: ↑ 31 ; follow-up: Changed 22 months ago by SimonKing

Replying to Winfried:

Replying to SimonKing?

Normaliz can compute Hilbert series of the integral closures of monomial ideals, not of arbitrary monomial ideals. If one wants to apply it: input the monomials as vertices, the polynomial ring as cone (= unit matrix), and put the weights of the indeterminates into the grading.

Example for Normaliz input (integral closure of (x2, y2, deg x = 2, deg y =1)

amb_space 2
vertices 2
2 0 1
0 2 1
cone 2
1 0
0 1
grading 
2 1
HilbertSeries

Thank you. But my question was really: Given a monomial ideal in Sage, how to compute the Hilbert series of it (or maybe of its integral closure) using normaliz as a backend (not: in normaliz).

I.e.:

        sage: n=4;m=11;P = PolynomialRing(QQ,n*m,"x"); x = P.gens(); M = Matrix(n,x)
        sage: I = P.ideal(M.minors(2))
        sage: J = P*[m.lm() for m in I.groebner_basis()]

and what now? How to use the new interface to normaliz to compute the Hilbert series of (the integral closure of) J?

Since I never worked with the integral closure of a monomial ideal: How are the Hilbert series of the ideal and its integral closure related?

comment:33 Changed 22 months ago by Winfried

"What now?" is a question that Jean-Philippe should answer. I have no overview which of the Normaliz computation goals can be reached from Sage. However, all of them can be reached via PyNormaliz?. For example, Normaliz can take a binomial ideal (suitably encoded) as input. It finds the affine monoid A defined by the binomial ideal, and then computes invariants of the normalization of A.

I think there is no way to relate the Hilbert series of an ideal I and the Hilbert series of its integral closure in a useful way.

comment:34 in reply to: ↑ 32 Changed 22 months ago by jipilab

Thank you. But my question was really: Given a monomial ideal in Sage, how to compute the Hilbert series of it (or maybe of its integral closure) using normaliz as a backend (not: in normaliz).

I.e.:

        sage: n=4;m=11;P = PolynomialRing(QQ,n*m,"x"); x = P.gens(); M = Matrix(n,x)
        sage: I = P.ideal(M.minors(2))
        sage: J = P*[m.lm() for m in I.groebner_basis()]

and what now? How to use the new interface to normaliz to compute the Hilbert series of (the integral closure of) J?

Since I never worked with the integral closure of a monomial ideal: How are the Hilbert series of the ideal and its integral closure related?

My knowledge of the theory lacks in order to give you a satisfactory answer to your questions.

The present ticket allows you to get the Hilbert series of a rational polyhedron. Whether this Hilbert series is or corresponds to the Hilbert series of a monomial ideal should be clarified. In case they match, then, what I would do in Sage is

1) a function that transfers the data of the monomial ideal to a polyhedron that uses the backend normaliz and compute its hilbert series (a function implemented in this ticket).

2) use PyNormaliz? package directly as the normaliz backend of polyhedron is currently doing. This is not really well documented as its usage is currently only in the backend for polyhedron.

For this, the present ticket provides some useful functions to deal with PyNormaliz? that builds on #25090.

comment:35 Changed 22 months ago by jipilab

Would this compute what you want? I did this with the current ticket and merge with the latest sage.

sage: n=4;m=11;P = PolynomialRing(QQ,n*m,"x"); x = P.gens(); M = Matrix(n,x)
sage: I = P.ideal(M.minors(2))
sage: J = P*[m.lm() for m in I.groebner_basis()]
sage: P = Polyhedron(rays=[m.degrees() for m in J.gens()],backend='normaliz')
sage: P
A 42-dimensional polyhedron in ZZ^44 defined as the convex hull of 1 vertex and 330 rays
sage: P.hilbert_series()

Well, I'd say this is a quite large computation involving triangulating a 42 dimensional polyhedron with 330 vertices...

comment:36 Changed 22 months ago by Winfried

What one really wants is not the Hilbert series of the ideal. The goal is the Hilbert series of the residue class ring defined by the ideal. The residue class ring can be identified with a monomial algebra of Krull dimension 14. The Hilbert series is easily calculated. If one wants to use Normaliz for it, then the best approach is to input the binomial ideal to Normaliz.

You make a matrix of integer vectors whose rows represent the binomials in the form v-w, corresponding to xv - xw. The input type is "lattice_ideal". You must also define a grading. Simply say "total_degree". In this case it represents a vector with 44 entries 1.

That one can go this way is due to the fact that the residue class ring is a normal monoid algebra. In general this is not the case.

comment:37 in reply to: ↑ 30 Changed 22 months ago by jipilab

Replying to SimonKing:

Replying to jipilab:

Replying to vdelecroix:

Note that is_package_installed is being deprecated, see #20382

Oh! Wow! Thanks for the timely warning! I set it as a dependency. I will change the check once it is merged...

It is really a pity that #20382 doesn't explain (in the ticket description!!!) what one is supposed to do instead of is_package_installed.

Indeed.

comment:38 follow-up: Changed 22 months ago by jipilab

Concerning the is_package_installed thing, it seems that it is not deprecated when we want to check whether a package was installed using sage -i which is what we want here I guess.

So I would remove it from the necessary things to do for this ticket.

comment:39 in reply to: ↑ 38 ; follow-up: Changed 22 months ago by SimonKing

Replying to jipilab:

Concerning the is_package_installed thing, it seems that it is not deprecated when we want to check whether a package was installed using sage -i which is what we want here I guess.

So I would remove it from the necessary things to do for this ticket.

I tend to disagree. is_package_installed is used here to test whether pynormaliz, normaliz, or latte_int is available. And it may be available for reasons that have nothing to do with sage -i.

comment:40 in reply to: ↑ 39 ; follow-ups: Changed 22 months ago by jipilab

Replying to SimonKing:

Replying to jipilab:

Concerning the is_package_installed thing, it seems that it is not deprecated when we want to check whether a package was installed using sage -i which is what we want here I guess.

So I would remove it from the necessary things to do for this ticket.

I tend to disagree. is_package_installed is used here to test whether pynormaliz, normaliz, or latte_int is available. And it may be available for reasons that have nothing to do with sage -i.

Okay. Nevertheless, the mentionned ticket does not provide a way to check if they are available or not. Right?

comment:41 in reply to: ↑ 40 Changed 22 months ago by SimonKing

Replying to jipilab:

Okay. Nevertheless, the mentionned ticket does not provide a way to check if they are available or not. Right?

Yes, it doesn't. But I guess that's because the author of that ticket thought that the solution is relatively straight forward: When you code in Python, how do you use a package? You do so by importing stuff. Hence, you should test whether the package is available by trying to import something from that package (sooner or later, you will do anyway!), surrounded with a try: ... except ImportError: ... clause.

Indeed, in your code, you do import PyNormaliz. So, this could be

try:
    import PyNormaliz
except ImportError:
    raise ImportError("some meaningful error message, telling the user at least one way to install PyNormaliz")

For latte_int, I am not so sure what you do, because I don't see a related import statement in your code. However, in some place of your code, you do use it; you should try and see what happens if latte_int is not available: Will there be an ImportError, or a TypeError, or a ValueError, etc.? Then, instead of is_package_available, you could try to use latte_int in a dry run, and if an error occurs, you take it as an answer (namely the answer that latte_int is not available).

comment:42 in reply to: ↑ 40 ; follow-up: Changed 22 months ago by mkoeppe

Replying to jipilab:

Replying to SimonKing:

Replying to jipilab:

Concerning the is_package_installed thing, it seems that it is not deprecated when we want to check whether a package was installed using sage -i which is what we want here I guess.

So I would remove it from the necessary things to do for this ticket.

I tend to disagree. is_package_installed is used here to test whether pynormaliz, normaliz, or latte_int is available. And it may be available for reasons that have nothing to do with sage -i.

Okay. Nevertheless, the mentionned ticket does not provide a way to check if they are available or not. Right?

I think what may be intended here is to subclass sage.features.PythonModule.

comment:43 Changed 21 months ago by git

  • Commit changed from 668fdfc3d93a3c567c3258b7d8eaa2c36bcc5400 to d5fdf168e49a1a4260dc804d29998f33da3a4aba

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

d5fdf16Merge branch 'develop' into HEAD

comment:44 Changed 21 months ago by git

  • Commit changed from d5fdf168e49a1a4260dc804d29998f33da3a4aba to 20d2fde78750f859770f05473f49d7de460d6bb1

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

20d2fdeFix typo in docstring.

comment:45 Changed 21 months ago by mkoeppe

  • Cc gh-braunmath added

comment:46 Changed 18 months ago by jipilab

I looked back at this now, and I'm getting a weird result from an example in Normaliz' manual:

sage: P = Polyhedron(vertices=[(1/2,1/2),(1/3,1/3),(1/4,-1/2)],backend='normaliz')
sage: P
A 2-dimensional polyhedron in QQ^2 defined as the convex hull of 3 vertices
sage: P.hilbert_series()
0

From calling PyNormaliz?, I get:

sage: h = PyNormaliz.NmzResult(new_cone, "HilbertSeries")
sage: h
[[], [], -1L]

So it's not the parsing from PyNormaliz? to sage, but rather something weird is happening here. It has been a while since I wrote the code so I am a bit lost in trying to figure out what is going on...

I'll try to do some more searching...

comment:47 follow-up: Changed 18 months ago by Winfried

I guess you want the Ehrhart series. The Normaliz computation goal HilbertSeries? requires the existence of a grading which is not defined with this input.

This is an intricate point, but it is inevitable: the grading is defined on the space in which the polyhedron lives, not on the cone over it.

comment:48 in reply to: ↑ 47 Changed 18 months ago by jipilab

Replying to Winfried:

I guess you want the Ehrhart series. The Normaliz computation goal HilbertSeries? requires the existence of a grading which is not defined with this input.

Yes, in the end I want the Ehrhart series, but I was doing both computations to understand their difference again...

This is an intricate point, but it is inevitable: the grading is defined on the space in which the polyhedron lives, not on the cone over it.

Yes, agreed. I take this example from p.23 of the Normaliz manual. I thought that HilbertSeries? would work out of the box like in the example. That's why I am asking myself what I did wrong.

In the code for hilbert_series, if no grading is given, we canonically give the all 1's grading on the ambient space, and then ask for the HilbertSeries?, in which case we get the empty result(!?). (Is that a good choice of canonical grading?)

So, my question would be, how do I get the result from the Normaliz manual?

This issue makes me wonder also if the results in the current examples are actually true (because maybe I am doing something wrong). For example, what would be the expected outputs of for the 3-dimensional 0-1 cube?

Currently it gives me:

sage: C = Polyhedron(vertices = [[0,0,0],[0,0,1],[0,1,0],[0,1,1],[1,0,0],[1,0,1]
....: ,[1,1,0],[1,1,1]],backend='normaliz')
sage: C.hilbert_series()
t^3 + 3*t^2 + 3*t + 1
sage: C.ehrhart_series()
(t^2 + 4*t + 1)/(t^10 - t^9 - t^8 + 2*t^5 - t^2 - t + 1)
sage: C.ehrhart_series().denominator().factor()
(t + 1)^2 * (t - 1)^4 * (t^2 + 1) * (t^2 + t + 1)
sage: C.ehrhart_quasipolynomial()
t^3 + 3*t^2 + 3*t + 1

Hilbert series and ehrhart polynomials look fine, but the denominator of the Ehrhart series is not what I would expect. Is that related to the discussion on p.24 of Normaliz' manual?

I would have expected (1-t)4, so that the degree would be -2 so that when we dilate by a factor two, we see the first interior integer point. Right?

comment:49 Changed 18 months ago by Winfried

HilbertSeries? works out of the box, provided you use the right input type:

polytope defines a cone and a grading, and the Hilbert series of the cone under this grading is the Ehrhart series of the polytope that arises as the intersection of the cone and the hyperplane of degree 1 elements. polytope is a homogeneous input type.

The inhomogeneous type vertices only defines a polytope P, or in connection with cone a potentially umbounded polyhedron. We can have an grding on the space in which the polytope lives, and then HilbertSeries? makes sense. This Hilbert series is the generating function of the lattice point enumerator of the points in P. You can ask Normaliz to compute the Ehrhart series of P if P is bounded. It constructs a cone with a grading behind the scenes and computes the Hilbert series of that cone.

So far, so good.

In your cube example: the vertices are in principle rational vectors. What are your denominators? Does Sage choose them automatically? If so what are they?

Moreover, it seems to me tat Sage implicitly defines a grading on the space of the polytpe, taking the sum of the coordinates. This explains why you get a Hilbert series and that it is exactly the result shown: 1 point of degree 0, 3 points of degree 1, 3 of degree 2 and 1 of degree 3.

The denominator of the Ehrhart series is for a 3-dimensional polytope that has vertices of degree 4,3,2 qnd 1.

Before I can say more, I must know what happens on the way from Sage input to Normaliz input.

comment:50 Changed 18 months ago by Winfried

PS. My question for the denominators must be understood form the viewpoint of Normaliz input. For the 3-cube we need vertices with four coordinates in Normaliz where the last coordinate represents a denominator for the first three. (This was introduced before Normaliz allowed fractions in input; it would be superfluous now.)

If you send the matrix with the 8 vertices as it is to Normaliz as input type vertices, then the result will be a BadInputexception?, because we have the denominator 0 for some vertices. In other words: Something must have happened from the Sage vertices before they go into Normaliz.

I think you want the denominator 1 attached to each vertex. If we do that, the Normaliz input becomes

amb_space auto
vertices
[[0,0,0,1],[0,0,1,1],[0,1,0,1],[0,1,1,1],[1,0,0,1],[1,0,1,1],[1,1,0,1],[1,1,1,1]]
EhrhartSeries

and the output contains

Ehrhart series:
1 4 1 
denominator with 4 factors:
1: 4

as one expects.

Again my question: what happens on the way from Sage input to Normaliz input?

comment:51 Changed 18 months ago by jipilab

Dear Winfried,

Thanks for your quick reply. This explanation is really good, and probably I should add it to the top to shortly describe how the translation from sage to normaliz is done.

Here is the verbose of the example for the cube, we can see the input given by sage to normaliz. The current way is to always use inhomogeneous (vertices, cones, and subspaces). Eventually, inhom. equations and inequalities are added.

sage: C = Polyhedron(vertices = [[0,0,0],[0,0,1],[0,1,0],[0,1,1],[1,0,0],[1,0,1],[1,1,0],[1,1,1]],backend='normaliz',
....: verbose=True)
# Calling PyNormaliz.NmzCone(**{'subspace': [], 'vertices': [[0, 0, 0, 1], [0, 0, 1, 1], [0, 1, 0, 1], [0, 1, 1, 1], [1, 0, 0, 1], [1, 0, 1, 1], [1, 1, 0, 1], [1, 1, 1, 1]], 'cone': []})
# ----8<---- Equivalent Normaliz input file ----8<----
amb_space 3
subspace 0
vertices 8
 0 0 0 1
 0 0 1 1
 0 1 0 1
 0 1 1 1
 1 0 0 1
 1 0 1 1
 1 1 0 1
 1 1 1 1
cone 0
# ----8<-------------------8<-------------------8<----

Once I have this, I compute the hilbert series, without giving any grading. In this case, Sage assign the grading [1,...,1] and passes it to normaliz (as you observed) and get:

sage: C.hilbert_series()
t^3 + 3*t^2 + 3*t + 1

Now, for the Ehrhart series, I get:

sage: C.ehrhart_series()
(t^2 + 4*t + 1)/(t^10 - t^9 - t^8 + 2*t^5 - t^2 - t + 1)

As you said:

It constructs a cone with a grading behind the scenes and computes the Hilbert series of that cone.

What is the grading in this case?

So there seems to be a difference between the above input and the one you mentioned:

amb_space auto
vertices
[[0,0,0,1],[0,0,1,1],[0,1,0,1],[0,1,1,1],[1,0,0,1],[1,0,1,1],[1,1,0,1],[1,1,1,1]]
EhrhartSeries

because they give different output for the Ehrhart series?

comment:52 follow-up: Changed 18 months ago by Winfried

The Normaliz input is the desired one. If I run it with the computation goal EhhhartSeries? I get the right answer, and if I run it with the computation goal HilbertSeries?, then the answer is that it cannot be computed. Everything o.k.

But it is also clear that this workflow is not exactly that of Sage/PyNormaliz?: the latter constructs a cone and then calls functions that trigger computations and return results. For a better analysis the next step is to put the same data into PyNormaliz? and to see what happens.

comment:53 in reply to: ↑ 52 Changed 18 months ago by jipilab

Replying to Winfried:

The Normaliz input is the desired one. If I run it with the computation goal EhhhartSeries? I get the right answer, and if I run it with the computation goal HilbertSeries?, then the answer is that it cannot be computed. Everything o.k.

Ok. Good to know!

But it is also clear that this workflow is not exactly that of Sage/PyNormaliz?: the latter constructs a cone and then calls functions that trigger computations and return results. For a better analysis the next step is to put the same data into PyNormaliz? and to see what happens.

Ok, I will look into that.

comment:54 Changed 18 months ago by jipilab

Ok, I think that I have figured out something, that I did not expect.

sage: C = Polyhedron(vertices = [[0,0,0],[0,0,1],[0,1,0],[0,1,1],[1,0,0],[1,0,1],[1,1,0],[1,1,1]
....: ],backend='normaliz',verbose=True)
# Calling PyNormaliz.NmzCone(**{'subspace': [], 'vertices': [[0, 0, 0, 1], [0, 0, 1, 1], [0, 1, 0, 1], [0, 1, 1, 1], [1, 0, 0, 1], [1, 0, 1, 1], [1, 1, 0, 1], [1, 1, 1, 1]], 'cone': []})
# ----8<---- Equivalent Normaliz input file ----8<----
amb_space 3
subspace 0
vertices 8
 0 0 0 1
 0 0 1 1
 0 1 0 1
 0 1 1 1
 1 0 0 1
 1 0 1 1
 1 1 0 1
 1 1 1 1
cone 0
# ----8<-------------------8<-------------------8<----

So far I simply created the 0-1-cube using normaliz. Then, I get the normaliz cone and the enclosed data:

sage: cone = C._normaliz_cone;cone
<capsule object "Cone" at 0x7fcc880cf210>
sage: data = C._get_nmzcone_data()

From there, if I compute the Hilbert Series directly, it will complain, because it does not have a grading:

sage: PyNormaliz.NmzResult(cone,"HilbertSeries")
---------------------------------------------------------------------------
error                                     Traceback (most recent call last)
<ipython-input-26-93c9989a5377> in <module>()
----> 1 PyNormaliz.NmzResult(cone,"HilbertSeries")

error: Could not compute: No grading specified and cannot find one. Cannot compute some requested properties!

So far so good. Then, I ask for the Ehrhart series and then, it unlocks the Hilbert series:

sage: PyNormaliz.NmzResult(cone,"EhrhartSeries")
[[1L, 4L, 1L], [1L, 1L, 1L, 1L], 0L]
sage: PyNormaliz.NmzResult(cone,"HilbertSeries")
[[1L, 4L, 1L], [1L, 1L, 1L, 1L], 0L]

For now, there is no way to access the difference in the normaliz cone. It is now equipped with a grading in the background, but it seems that we can not extract it:

sage: new_C = C.parent().element_class._from_normaliz_cone(parent=C.parent(),normaliz_cone=cone)
sage: PyNormaliz.NmzResult(new_C._normaliz_cone,"HilbertSeries")
[[1L, 4L, 1L], [1L, 1L, 1L, 1L], 0L]
sage: PyNormaliz.NmzResult(new_C._normaliz_cone,"Grading")
---------------------------------------------------------------------------
error                                     Traceback (most recent call last)
<ipython-input-43-809cb91899e5> in <module>()
----> 1 PyNormaliz.NmzResult(new_C._normaliz_cone,"Grading")

error: Could not compute: Grading !

But, when creating a whole new cone from the data and adding by hand the grading (which is what is done in hilbert_series in sage, we get the enumeration of the lattice points with respect to this grading:

sage: C.hilbert_series()
t^3 + 3*t^2 + 3*t + 1

This can be reproduced manually as:

sage: data['grading'] = [1] * C.ambient_dim()
sage: new_cone = C._make_normaliz_cone(data)
sage: h = PyNormaliz.NmzResult(new_cone, "HilbertSeries");h
[[1L, 3L, 3L, 1L], [], 0L]

So, right now, we get two potential output for Hilbert series.

Further, if we actually change the normaliz cone while doing the computation of the Hilbert series (instead of doing it on a distinct cone created on the side), it then breaks the ehrhart series:

sage: C._normaliz_cone = new_cone
sage: C.ehrhart_series()
---------------------------------------------------------------------------
error                                     Traceback (most recent call last)
<ipython-input-85-0117d93a819f> in <module>()
----> 1 C.ehrhart_series()

/home/jplabbe/sage/local/lib/python2.7/site-packages/sage/geometry/polyhedron/backend_normaliz.pyc in ehrhart_series(self, variable)
    636 
    637         cone = self._normaliz_cone
--> 638         e = PyNormaliz.NmzResult(cone, "EhrhartSeries")
    639 
    640         from sage.rings.polynomial.polynomial_ring_constructor import PolynomialRing

error: Some error in the normaliz input data detected: Grading not allowed with Ehrhart series in the inhomogeneous case

Oi Oi Oi...

comment:55 Changed 18 months ago by Winfried

Everything above is as it should be, with 2 exceptions:

1) The computation of the Ehrhart series should not make the Hilbert series computed. This requires a correction in libnormaliz.

There is no grading in the background. The Ehrhart series was computed on an auxiliary cone.

2) Sage should not add a default grading if one asks for the Hilbert series. This is not in my hands.

Further remark: if one changes the grading later on (or defines one), all grading dependent results are erased.

comment:56 Changed 18 months ago by Winfried

PS. The last observation is the intended behavior of Normaliz: HilbertSeries? and EhrhartSeries? are not allowed on the same input. One intended obstruction to this is that we do not even allow a grading if EhrhartSeries? is to be computed.

comment:57 Changed 18 months ago by Winfried

The next Normaliz release will store the Hilbert series and the Ehrhart series separately and provide a specific access function for the Ehrhart series. Then the confusion that we see in the preceding remarks will not arise anymore.

comment:58 Changed 17 months ago by jipilab

Ok!!

So I went back to PyNormaliz? and had a look at the tutorial mentioned in Appendix E of the normaliz manual:

https://mybinder.org/v2/gh/Normaliz/NormalizJupyter/master

In there, the PyNormaliz? output of .hilbert_series is explained. Importantly, it seems that the output is different than the one of normaliz(!). This was the source of one of the above problems.

[[1L, 4L, 1L], [1L, 1L, 1L, 1L], 0L]

As to be interpreted as t^2+4t+1 divided by (1-t)(1-t)(1-t)(1-t) and a 0 shift.

So, it's all good. But it indicates that the code should be more explicit on those output formats... I'll put this on my list of TODO to add on some comments on the pynormaliz side...

comment:59 follow-up: Changed 17 months ago by Winfried

In the Normaliz pitput file we use a multiset notation. i think this is difficult to represent in Python. Therefore it is translated into a vector format where each element of the multiset is repeated as often as it occurs in the multiset.

comment:60 Changed 17 months ago by git

  • Commit changed from 20d2fde78750f859770f05473f49d7de460d6bb1 to f36bfeb89fd9293a021a0bb2f8f9584ba1a5ca10

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

ce986e3Merge branch 'public/some_normaliz_features' of trac.sagemath.org:sage into 25091
f36bfebcorrected doctests

comment:61 in reply to: ↑ 59 Changed 17 months ago by jipilab

Replying to Winfried:

In the Normaliz pitput file we use a multiset notation. i think this is difficult to represent in Python. Therefore it is translated into a vector format where each element of the multiset is repeated as often as it occurs in the multiset.

Yes, makes sense now. Somehow got it wrong from the beginning. I just commited an update that should cover the mistake. I added several examples coming from normaliz manual to be able to compare.

I still have to smoothen the usage for the user. But now, at least things look like they work and make sense...

comment:62 Changed 17 months ago by jipilab

  • Description modified (diff)

comment:63 Changed 17 months ago by git

  • Commit changed from f36bfeb89fd9293a021a0bb2f8f9584ba1a5ca10 to dbf7db846182a3bc496458d8eea1bf10e8792b0a

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

dbf7db8Fixed the Feature of Latte

comment:64 Changed 17 months ago by jipilab

  • Description modified (diff)

comment:65 Changed 17 months ago by jipilab

Here is a bug, which I think is coming from the PyNormaliz? side of things:

sage: v = [[0,0,0],[0,0,1],[0,1,0],[0,1,1],[1,0,0],[1,0,1],[1,1,0],[1,1,1]]
sage: cube = Polyhedron(vertices=v,backend='normaliz')
sage: cube._volume_normaliz()
sage: cube._volume_normaliz('induced_lattice')
6
sage: cube._volume_normaliz()
1.0

Equivalently:

sage: v = [[0,0,0],[0,0,1],[0,1,0],[0,1,1],[1,0,0],[1,0,1],[1,1,0],[1,1,1]]
sage: cube = Polyhedron(vertices=v,backend='normaliz',verbose=True)
# Calling PyNormaliz.NmzCone(**{'subspace': [], 'vertices': [[0, 0, 0, 1], [0, 0, 1, 1], [0, 1, 0, 1], [0, 1, 1, 1], [1, 0, 0, 1], [1, 0, 1, 1], [1, 1, 0, 1], [1, 1, 1, 1]], 'cone': []})
# ----8<---- Equivalent Normaliz input file ----8<----
amb_space 3
subspace 0
vertices 8
 0 0 0 1
 0 0 1 1
 0 1 0 1
 0 1 1 1
 1 0 0 1
 1 0 1 1
 1 1 0 1
 1 1 1 1
cone 0
# ----8<-------------------8<-------------------8<----
sage: cone = cube._normaliz_cone
sage: import PyNormaliz
sage: PyNormaliz.NmzResult(cone,'EuclideanVolume')
sage: PyNormaliz.NmzResult(cone,'EuclideanVolume')
1.0

This is weird. The volume somehow seems to get lost on the way. When one asks again, the output appears.

comment:66 Changed 17 months ago by git

  • Commit changed from dbf7db846182a3bc496458d8eea1bf10e8792b0a to 25b17381036c5e0bd792437b3dda0efde017fa4c

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

25b1738Fixed some doc

comment:67 Changed 16 months ago by git

  • Commit changed from 25b17381036c5e0bd792437b3dda0efde017fa4c to 21671bdf4561edcbf07617f526d649e962038dc2

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

7356f5bMerge branch 'develop' into 25091
21671bdtypo

comment:68 Changed 16 months ago by jipilab

  • Milestone changed from sage-8.4 to sage-8.7

comment:69 Changed 16 months ago by vdelecroix

This

try:
    import PyNormaliz
except ImportError:
    raise ImportError("This backend requires PyNormaliz. To install PyNormaliz, type 'sage -i pynormaliz' in the terminal.")

is not desirable. You should do that via a "feature". See the class PythonModule in sage/features/__init__.py. The main reason is that sage -i would only work for people compiling from source (which is the case of less and less people nowadays).

comment:70 Changed 16 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:71 Changed 15 months ago by jipilab

  • Report Upstream changed from N/A to Fixed upstream, in a later stable release.
  • Work issues set to Some doctest fails, will need to be based on a newer version

comment:72 Changed 15 months ago by jipilab

  • Dependencies changed from #22984, #25090, #20382 to #22984, #25090, #20382, #27682

comment:73 Changed 15 months ago by mkoeppe

I'd suggest to merge #27716 as a dependency and use the new helper method _nmz_result

comment:74 Changed 15 months ago by git

  • Commit changed from 21671bdf4561edcbf07617f526d649e962038dc2 to 86ce4420c5f2f9b4d72f8afe7c01764eea5391b0

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

9bf2f42Merge branch on top of the latest develop version
57b3569Merge branch 'develop' into 25091
7618ea9package e-antic
d752708upgrade normaliz
4eb02f8fix doctest in backend_normaliz.py
86ce442Merge branch '27682' into 25091

comment:75 Changed 15 months ago by jipilab

  • Dependencies changed from #22984, #25090, #20382, #27682 to #22984, #25090, #20382, #27682, #27716

I merge the latest develop and #27682 in the last commits. I will merge #27716 as the next step.

comment:76 Changed 15 months ago by git

  • Commit changed from 86ce4420c5f2f9b4d72f8afe7c01764eea5391b0 to 79059033a2b14ba9ffaf032a45361e12817efc75

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

0dfec03_format_function_call: Fix doctest
b370268pyflakes fixes
328a04a_nmz_result: Add doctest
ee5ff41Mark newest doctest # optional - pynormaliz
78c2de4Merge branch 't/27716/public/normaliz-backend-refactoring' into t/25091/public/some_normaliz_features
41bd968Fix doctest error message - NormalizError
7662077Fixup merge
885ef9eUse EhrhartQuasiPolynomial
313fee1Fix doctest of hilbert_series
7905903Pass grading as a matrix

comment:77 Changed 15 months ago by git

  • Commit changed from 79059033a2b14ba9ffaf032a45361e12817efc75 to 9249dda436e7e9c58b644ea5e4728f8fe969949f

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

9249ddaUse _nmz_result instead of calling NmzResult directly

comment:78 Changed 15 months ago by mkoeppe

  • Authors changed from Jean-Philippe Labbé to Jean-Philippe Labbé, Matthias Koeppe
  • Dependencies changed from #22984, #25090, #20382, #27682, #27716 to #27682, #27716
  • Report Upstream changed from Fixed upstream, in a later stable release. to N/A
  • Status changed from new to needs_review
  • Work issues Some doctest fails, will need to be based on a newer version deleted

comment:79 Changed 15 months ago by mkoeppe

  • Cc tscrim added

comment:80 Changed 15 months ago by tscrim

I have some comments:

This is a general Python convention we try to follow with the messages, moreover, the error is because the class has the wrong type (you are not expecting it to be implemented at some point, right?):

-raise NotImplementedError("The backend should be normaliz.")
+raise TypeError("the backend should be normaliz")

A similar comment applies elsewhere too. However, the ImportError can be the exception to this guide as it is "long".

The INPUT: bullet points are not considered to be complete sentences, so they should not end in a period/full-stop.

PEP8:

-sage: S = Polyhedron(vertices = [[0,1],[1,0]],backend='normaliz') # optional - pynormaliz
+sage: S = Polyhedron(vertices=[[0,1],[1,0]], backend='normaliz')  # optional - pynormaliz

and similar such things.

In ehrhart_series(), i think this will look better in latex formatting:

-``t^k`` is number of integer lattice points inside the ``k``-th dilation of
+`t^k` is number of integer lattice points inside the `k`-th dilation of

Note that k and t are not input variable names.

comment:81 Changed 15 months ago by git

  • Commit changed from 9249dda436e7e9c58b644ea5e4728f8fe969949f to adc3f2a4e6d82427850517738d1681300565a675

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

f2785c2pep8 and cleaning docstrings
adc3f2amade tests pass without pynormaliz

comment:82 Changed 15 months ago by git

  • Commit changed from adc3f2a4e6d82427850517738d1681300565a675 to 970b46dafef7d96fdcb9d0b97edb8c6b132cc473

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

970b46dFixed coverage and blocks

comment:83 Changed 15 months ago by mkoeppe

  • Status changed from needs_review to needs_work

There's an error in a doctest.

sage -t --long src/sage/geometry/polyhedron/base.py
**********************************************************************
File "src/sage/geometry/polyhedron/base.py", line 2874, in sage.geometry.polyhedron.base.Polyhedron_base._triangulate_normaliz
Failed example:
    K._triangulate_normaliz(engine='normaliz')
Expected:
    Traceback (most recent call last):
    ...
    TypeError: The polyhedron's backend should be 'normaliz'
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1095, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.geometry.polyhedron.base.Polyhedron_base._triangulate_normaliz[1]>", line 1, in <module>
        K._triangulate_normaliz(engine='normaliz')
    TypeError: _triangulate_normaliz() got an unexpected keyword argument 'engine'
**********************************************************************

comment:84 Changed 15 months ago by git

  • Commit changed from 970b46dafef7d96fdcb9d0b97edb8c6b132cc473 to e1eee578228315cab288ef5a0d22aca81a473731

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

e1eee57Fixed some doctests

comment:85 Changed 15 months ago by jipilab

  • Status changed from needs_work to needs_review

comment:86 follow-up: Changed 15 months ago by tscrim

I think I ended up being a little vague about the error messages. In Python, error messages start with a lowercase letter and do not end with a period/full-stop. While I do not expect you to change them all, I think it is good not to make it further from the general convention. I know this is not documented, but is sort of an agreement. So if you want to keep things the way they are, then you can go ahead and set a positive review. Although I would appreciate you changing them (or at least making a uniform convention for all of your added error messages).

comment:87 Changed 15 months ago by slabbe

One comment:

+# FLINT is a standard package and E-ANTIC is a standard pakcage. We

the 2nd occ. of standard -> optional

comment:88 in reply to: ↑ 86 Changed 15 months ago by jipilab

Replying to tscrim:

I think I ended up being a little vague about the error messages. In Python, error messages start with a lowercase letter and do not end with a period/full-stop. While I do not expect you to change them all, I think it is good not to make it further from the general convention. I know this is not documented, but is sort of an agreement. So if you want to keep things the way they are, then you can go ahead and set a positive review. Although I would appreciate you changing them (or at least making a uniform convention for all of your added error messages).

Oh, I see. No problem, I will change it.

comment:89 Changed 15 months ago by jdemeyer

  • Status changed from needs_review to needs_work

This is still using is_package_installed.

comment:90 Changed 15 months ago by jdemeyer

Instead of putting Latte().require() in a try block, you can just check if Latte().is_present()

It can be argued which kind of error should be given here (I would go for RuntimeError), but certainly not a TypeError:

raise TypeError("The induced rational measure can only be computed with the optional packages `latte_int`, or `pynormaliz`")

comment:91 in reply to: ↑ 42 Changed 15 months ago by jdemeyer

Replying to mkoeppe:

I think what may be intended here is to subclass sage.features.PythonModule.

Not subclassing, just use it like PythonModule("package.submodule").is_present() or whatever.

comment:92 Changed 15 months ago by jdemeyer

More in detail: replace

        try:
            import PyNormaliz
        except ImportError:
            raise ImportError("This backend requires PyNormaliz. To install PyNormaliz, type 'sage -i pynormaliz' in the terminal.")

by

from sage.features import PythonModule

        PythonModule("PyNormaliz", spkg="pynormaliz").require()
        import PyNormaliz

comment:93 Changed 15 months ago by git

  • Commit changed from e1eee578228315cab288ef5a0d22aca81a473731 to a4f01d474d6c5a18ce88ce6bd5a2c65dff7bac29

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

59cc7e0Fix Error messages
a4f01d4Fixed typo and added require package

comment:94 Changed 15 months ago by jipilab

  • Status changed from needs_work to needs_review

I fixed the error messages, typos, and the require() of PyNormaliz.

Ready for review again.

comment:95 Changed 15 months ago by jdemeyer

  • Status changed from needs_review to needs_work

89

comment:96 Changed 15 months ago by git

  • Commit changed from a4f01d474d6c5a18ce88ce6bd5a2c65dff7bac29 to fbd3dce364043bbd59dc10dbd1a742fec0d937a6

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

fbd3dceremoved is_package_installed

comment:97 Changed 15 months ago by jipilab

  • Status changed from needs_work to needs_review

... forgot about this latte/pynormaliz switch there. Hopefully complete now.

comment:98 Changed 15 months ago by jdemeyer

I would still recommend to change the type of exception: TypeError makes no sense here.

comment:99 Changed 15 months ago by git

  • Commit changed from fbd3dce364043bbd59dc10dbd1a742fec0d937a6 to 8ceee8473f0b43e9b738923d1de353e66ed84f0a

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

8ceee84Change TypeError for RuntimeError

comment:100 Changed 15 months ago by jipilab

... sorry for the omission, and thanks for the coaching on the features module.

comment:101 Changed 15 months ago by tscrim

Thank you for changing the error message strings.

You also need to make a changes in the combinat/rigged_configurations/kleber_tree.py

         try:
             poly = Polyhedron(ieqs=ieqs, backend='normaliz')
             self._has_normaliz = True
-        except ImportError:
+        except FeatureNotPresentError:
             poly = Polyhedron(ieqs=ieqs)
             self._has_normaliz = False

to reflect the new error message is thrown. That should take care of all of the errors reported on the patchbot (for when normaliz is not installed).

Modulo that, I am happy with this and am willing to set a positive review. Jeroen, any other comments?

comment:102 Changed 15 months ago by git

  • Commit changed from 8ceee8473f0b43e9b738923d1de353e66ed84f0a to 0da067f203fce15a1f27f480e1f2dd29bc8bd669

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

0da067fFixed doctests and added author name

comment:103 Changed 15 months ago by git

  • Commit changed from 0da067f203fce15a1f27f480e1f2dd29bc8bd669 to d7122d75e8149e4324e31928a313623ad427817b

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

d7122d7pyflakes

comment:104 Changed 15 months ago by tscrim

You forgot to import FeatureNotPresentError in kleber_tree.py.

comment:105 Changed 15 months ago by git

  • Commit changed from d7122d75e8149e4324e31928a313623ad427817b to 63a80b6634df8ee26bb0a50056956aec2c1a4b3a

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

63a80b6Forgot an import

comment:106 Changed 15 months ago by jipilab

... it has been a long workshop.

comment:107 Changed 15 months ago by git

  • Commit changed from 63a80b6634df8ee26bb0a50056956aec2c1a4b3a to d8716e3b8d51da404a63f7ca747847088a8d0d2a

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

d8716e3Refactor NmzResult

comment:108 Changed 15 months ago by jipilab

The last commit refactors things in the spirit of #27716.

It is now completely ready for review... No more forgotten things hopefully.

comment:109 follow-ups: Changed 15 months ago by tscrim

  • Reviewers set to Travis Scrimshaw, Jeroen Demeyer
  • Status changed from needs_review to needs_work

You definitely seem to have done quite a bit in your workshop. However, now there are serious things failing: I am getting an infinite recursion happening that ends with

/home/uqtscrim/sage-build/local/lib/python2.7/site-packages/sage/geometry/polyhedron/backend_normaliz.pyc in _nmz_result(self, normaliz_cone, property)
    193         PythonModule("PyNormaliz", spkg="pynormaliz").require()
    194         import PyNormaliz
--> 195         return self._nmz_result(normaliz_cone, property)
    196 
    197     def _init_from_normaliz_cone(self, normaliz_cone):

... last 1 frames repeated, from the frame below ...

/home/uqtscrim/sage-build/local/lib/python2.7/site-packages/sage/geometry/polyhedron/backend_normaliz.pyc in _nmz_result(self, normaliz_cone, property)
    193         PythonModule("PyNormaliz", spkg="pynormaliz").require()
    194         import PyNormaliz
--> 195         return self._nmz_result(normaliz_cone, property)
    196 
    197     def _init_from_normaliz_cone(self, normaliz_cone):

RuntimeError: maximum recursion depth exceeded in cmp

A function that only calls itself will definitely infinitely recurse. ;)

Additionally, a couple of little details noted by the patchbot from pyflakes for backend_normaliz.py:

In def _cone_generators(pynormaliz_cone):, you have

return self._nmz_result(pynormaliz_cone, "Generators")

In particular, self is not been defined in this @staticmethod.

Do you require the import PyNormaliz here in _nmz_result:

        PythonModule("PyNormaliz", spkg="pynormaliz").require()
        import PyNormaliz
        return self._nmz_result(normaliz_cone, property)

comment:110 in reply to: ↑ 109 Changed 15 months ago by jipilab

Oï oï oï... Will fix this, asap.

Replying to tscrim:

You definitely seem to have done quite a bit in your workshop. However, now there are serious things failing: I am getting an infinite recursion happening that ends with

/home/uqtscrim/sage-build/local/lib/python2.7/site-packages/sage/geometry/polyhedron/backend_normaliz.pyc in _nmz_result(self, normaliz_cone, property)
    193         PythonModule("PyNormaliz", spkg="pynormaliz").require()
    194         import PyNormaliz
--> 195         return self._nmz_result(normaliz_cone, property)
    196 
    197     def _init_from_normaliz_cone(self, normaliz_cone):

... last 1 frames repeated, from the frame below ...

/home/uqtscrim/sage-build/local/lib/python2.7/site-packages/sage/geometry/polyhedron/backend_normaliz.pyc in _nmz_result(self, normaliz_cone, property)
    193         PythonModule("PyNormaliz", spkg="pynormaliz").require()
    194         import PyNormaliz
--> 195         return self._nmz_result(normaliz_cone, property)
    196 
    197     def _init_from_normaliz_cone(self, normaliz_cone):

RuntimeError: maximum recursion depth exceeded in cmp

A function that only calls itself will definitely infinitely recurse. ;)

Additionally, a couple of little details noted by the patchbot from pyflakes for backend_normaliz.py:

In def _cone_generators(pynormaliz_cone):, you have

return self._nmz_result(pynormaliz_cone, "Generators")

In particular, self is not been defined in this @staticmethod.

Do you require the import PyNormaliz here in _nmz_result:

        PythonModule("PyNormaliz", spkg="pynormaliz").require()
        import PyNormaliz
        return self._nmz_result(normaliz_cone, property)

comment:111 Changed 15 months ago by git

  • Commit changed from d8716e3b8d51da404a63f7ca747847088a8d0d2a to 08b315bd50d9ae5737f99a2d92d6abe2708a38f3

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

08b315bFixed infinite recursion

comment:112 in reply to: ↑ 109 Changed 15 months ago by jipilab

Replying to tscrim:

A function that only calls itself will definitely infinitely recurse. ;)

I love those...

Additionally, a couple of little details noted by the patchbot from pyflakes for backend_normaliz.py:

In def _cone_generators(pynormaliz_cone):, you have

return self._nmz_result(pynormaliz_cone, "Generators")

In particular, self is not been defined in this @staticmethod.

Is now fixed.

Do you require the import PyNormaliz here in _nmz_result:

        PythonModule("PyNormaliz", spkg="pynormaliz").require()
        import PyNormaliz
        return self._nmz_result(normaliz_cone, property)

This was the cause of the infinite recursion. It is now fixed.

comment:113 Changed 15 months ago by jipilab

  • Status changed from needs_work to needs_review

comment:114 Changed 14 months ago by mkoeppe

Needs review again!

comment:115 follow-up: Changed 14 months ago by tscrim

  • Status changed from needs_review to positive_review

LGTM.

comment:116 in reply to: ↑ 115 Changed 14 months ago by jipilab

Replying to tscrim:

LGTM.

Great news!

comment:117 follow-up: Changed 14 months ago by vbraun

  • Status changed from positive_review to needs_work
**********************************************************************
File "src/sage/geometry/polyhedron/base.py", line 5295, in sage.geometry.polyhedron.base.Polyhedron_base.volume
Failed example:
    P.volume(measure='induced_rational')  # optional - latte_int
Expected:
    3/2
Got:
    1
**********************************************************************
1 item had failures:
   1 of  47 in sage.geometry.polyhedron.base.Polyhedron_base.volume
    [1137 tests, 1 failure, 48.32 s]
----------------------------------------------------------------------
sage -t --long src/sage/geometry/polyhedron/base.py  # 1 doctest failed
----------------------------------------------------------------------

comment:118 in reply to: ↑ 117 ; follow-up: Changed 14 months ago by jipilab

Replying to vbraun:

**********************************************************************
File "src/sage/geometry/polyhedron/base.py", line 5295, in sage.geometry.polyhedron.base.Polyhedron_base.volume
Failed example:
    P.volume(measure='induced_rational')  # optional - latte_int
Expected:
    3/2
Got:
    1
**********************************************************************
1 item had failures:
   1 of  47 in sage.geometry.polyhedron.base.Polyhedron_base.volume
    [1137 tests, 1 failure, 48.32 s]
----------------------------------------------------------------------
sage -t --long src/sage/geometry/polyhedron/base.py  # 1 doctest failed
----------------------------------------------------------------------

Hm. Annoying.

I can reproduce this with --optional=4ti2,bliss,dochtml,e_antic,gfortran,latte_int,lidia,lrslib,memlimit,mpir,ninja_build,python2,sage,topcom

and all tests pass with pynormaliz added. Dammit.

comment:119 in reply to: ↑ 118 Changed 14 months ago by jipilab

Replying to jipilab:

Replying to vbraun:

**********************************************************************
File "src/sage/geometry/polyhedron/base.py", line 5295, in sage.geometry.polyhedron.base.Polyhedron_base.volume
Failed example:
    P.volume(measure='induced_rational')  # optional - latte_int
Expected:
    3/2
Got:
    1
**********************************************************************
1 item had failures:
   1 of  47 in sage.geometry.polyhedron.base.Polyhedron_base.volume
    [1137 tests, 1 failure, 48.32 s]
----------------------------------------------------------------------
sage -t --long src/sage/geometry/polyhedron/base.py  # 1 doctest failed
----------------------------------------------------------------------

Hm. Annoying.

I can reproduce this with --optional=4ti2,bliss,dochtml,e_antic,gfortran,latte_int,lidia,lrslib,memlimit,mpir,ninja_build,python2,sage,topcom

and all tests pass with pynormaliz added. Dammit.

Got it! It's from a previous test... That's a easy fix. I forgot to put both latte_it and pynormaliz as optional...

comment:120 Changed 14 months ago by git

  • Commit changed from 08b315bd50d9ae5737f99a2d92d6abe2708a38f3 to da057067814b4a5adbc1e77ce0fc9dcc8f0dee83

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

da05706Fix forgotten optional package test

comment:121 Changed 14 months ago by jipilab

  • Status changed from needs_work to needs_review

Needs review again...

comment:122 Changed 14 months ago by tscrim

  • Status changed from needs_review to positive_review

comment:123 Changed 14 months ago by vbraun

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