Opened 3 years ago

Closed 3 years ago

#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 3 years 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 3 years 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 3 years ago by gh-kliem

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

comment:4 Changed 3 years 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 3 years 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 3 years 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 3 years ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:8 Changed 3 years 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 3 years 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 (

template <typename Integer>
mpq_class Cone<Integer>::getVolume() {
    return volume;

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

template <>
mpq_class Cone<renf_elem_class>::getVolume() {
    return 0;

template <>
renf_elem_class Cone<renf_elem_class>::getRenfVolume() {
    return renf_volume;

This is also spelled out in the manual 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 3 years 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 3 years ago by Winfried

At present, calling


for a cone of renf_elem_class results in an


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