Wrong usage of normaliz/pynormaliz makes sage crash hard

[kliem]

The following innocent code leads sage to crash hard.

sage: P = polytopes.dodecahedron(backend='normaliz')
sage: P.volume(measure='induced_lattice')

The problem is that we use the wrong method. We use
P._nmz_result(cone, 'Volume') but we should use P._nmz_result(cone, 'RenfVolume').

CC: @tscrim @dimpase @novoselt @w-bruns @nthiery @videlec @mo271 @jplab @mmasdeu @braunmath @kliem

Component: geometry

Keywords: polyhedron, normaliz, volume

Author: Jonathan Kliem

Branch/Commit: 516a622

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/28872

[kliem]

comment:1

For the record:

The underlying normaliz error is

python3: ./libnormaliz/cone.cpp:2266: mpq_class libnormaliz::Cone<Integer>::getVolume() [with Integer = renf_elem_class; mpq_class = __gmp_expr<__mpq_struct [1], __mpq_struct [1]>]: Assertion `false' failed.

[kliem]

Branch: public/28872

[kliem]

New commits:

6a15a44

raised error instead of crashing for a bug in normaliz

[kliem]

Commit: 6a15a44

[kliem]

Author: Jonathan Kliem

[kliem]

comment:4

I marked it as blocker because it's an easy fix (for now) and it is potentially really annoying. So I would like to see this being taken care of for the next master.

[kliem]

comment:5

I guess the real question is how we can prevent sage to crash, if normaliz throws an error.

[sagetrac-git]

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

516a622

actually fixing our error

[sagetrac-git]

Changed commit from 6a15a44 to 516a622

[tscrim]

comment:8

I can confirm that this does work in the sense that doctests pass. Could someone more familiar with polytope theory verify the mathematical correctness?

[tscrim]

Reviewer: Travis Scrimshaw

[kliem]

comment:9

I don't know if that helps you:

If I remember correctly, the problem is just that C++ allows multiple inputs but only one output. So for Cone<Integer> one should use getVolume and for Cone<renf_elem_class> one should use getRenfVolume. This is the corresponding code in normaliz (https://github.com/Normaliz/Normaliz/blob/master/source/libnormaliz/cone.cpp)

template <typename Integer>
mpq_class Cone<Integer>::getVolume() {
    compute(ConeProperty::Volume);
    return volume;
}

template <typename Integer>
renf_elem_class Cone<Integer>::getRenfVolume() {
    assert(false);
    return {};
}

#ifdef ENFNORMALIZ
template <>
mpq_class Cone<renf_elem_class>::getVolume() {
    assert(false);
    return 0;
}

template <>
renf_elem_class Cone<renf_elem_class>::getRenfVolume() {
    compute(ConeProperty::RenfVolume);
    return renf_volume;
}
#endif

This is also spelled out in the manual https://github.com/Normaliz/Normaliz/blob/master/doc/Normaliz.pdf in section D.9

In return values Integer must be specialized to renf_elem_class . A special return value is > the volume
that in general is no longer of type mpq_class . It is retrieved by
renf_elem_class Cone<renf_elem_class>::getRenfVolume().

[tscrim]

comment:10

Ah, I think I understand. It is merely an input distinction (not a mathematical one), and the error comes from the fact that things need to be done by reference except for ZZ and QQ. Thanks.

[w-bruns]

comment:11

At present, calling

getVolume()

for a cone of renf_elem_class results in an

assert(false)

and then in a hard crash. This is the standard solution in Normaliz for coding mistakes. I did not think about the possibility that this might happen outside Normaliz. As a first measurte, I will replace the assert(false) by throwing a NonComputableExcetion. It is caught by PyNormaliz. (The disadvantage of exceptions is that they cannot be traced back.)

I will then thry to find a better solution. Perhaps we can overcome the problem by making the return value a template. But this will change the systematics of the interface between libnormaliz and PyNormaliz.

[w-bruns]

comment:12

The assert(false) has been replaced by an exception. There should be no assert(false) in public methods of the class Cone anymore.

Pushed to GitHub branch master.

[tscrim]

comment:13

On a later ticket, we can either port the fix as a patch or just wait for the upgrade. Since this has been positively reviewed for 2+ days, I don't think we should make more changes here (unless Volker rejects it for some reason).

[vbraun]

Changed branch from public/28872 to 516a622