#28872 closed defect (fixed)

Wrong usage of normaliz/pynormaliz makes sage crash hard

Reported by: gh-kliem Owned by:
Priority: critical Milestone: sage-9.0
Component: geometry Keywords: polyhedron, normaliz, volume
Cc: tscrim, dimpase, novoselt, Winfried, nthiery, vdelecroix, moritz, jipilab, mmasdeu, gh-braunmath, gh-kliem Merged in:
Authors: Jonathan Kliem Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 516a622 (Commits, GitHub, GitLab) Commit: 516a62270a54e6d15f32e87241cf953a4f5daf75
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-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').

Change History (14)

comment:1 Changed 20 months ago by gh-kliem

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.

comment:2 Changed 20 months ago by gh-kliem

  • Authors set to Jonathan Kliem
  • Branch set to public/28872
  • Commit set to 6a15a44b8702bcd05d441f794493be350ce461f4
  • Status changed from new to needs_review

New commits:

6a15a44raised error instead of crashing for a bug in normaliz

comment:3 Changed 20 months ago by gh-kliem

  • Cc tscrim dimpase novoselt Winfried nthiery vdelecroix moritz jipilab mmasdeu gh-braunmath gh-kliem added

comment:4 Changed 20 months ago by gh-kliem

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.

comment:5 Changed 20 months ago by gh-kliem

  • Description modified (diff)
  • Priority changed from blocker to critical
  • Status changed from needs_review to needs_work
  • Summary changed from Bug in normaliz/pynormaliz makes sage crash hard to Wrong usage of normaliz/pynormaliz makes sage crash hard

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

comment:6 Changed 20 months ago by git

  • Commit changed from 6a15a44b8702bcd05d441f794493be350ce461f4 to 516a62270a54e6d15f32e87241cf953a4f5daf75

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

516a622actually fixing our error

comment:7 Changed 20 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:8 Changed 20 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

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

comment:9 Changed 20 months ago by gh-kliem

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

comment:10 Changed 20 months ago by tscrim

  • Status changed from needs_review to positive_review

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.

comment:11 Changed 20 months ago by Winfried

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

comment:12 Changed 20 months ago by Winfried

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.

comment:13 Changed 20 months ago by tscrim

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

comment:14 Changed 19 months ago by vbraun

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