Opened 16 months ago
Closed 16 months ago
#28872 closed defect (fixed)
Wrong usage of normaliz/pynormaliz makes sage crash hard
Reported by:  ghkliem  Owned by:  

Priority:  critical  Milestone:  sage9.0 
Component:  geometry  Keywords:  polyhedron, normaliz, volume 
Cc:  tscrim, dimpase, novoselt, Winfried, nthiery, vdelecroix, moritz, jipilab, mmasdeu, ghbraunmath, ghkliem  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  516a622 (Commits, GitHub, GitLab)  Commit:  516a62270a54e6d15f32e87241cf953a4f5daf75 
Dependencies:  Stopgaps: 
Description (last modified by )
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 16 months ago by
comment:2 Changed 16 months ago by
 Branch set to public/28872
 Commit set to 6a15a44b8702bcd05d441f794493be350ce461f4
 Status changed from new to needs_review
New commits:
6a15a44  raised error instead of crashing for a bug in normaliz

comment:3 Changed 16 months ago by
 Cc tscrim dimpase novoselt Winfried nthiery vdelecroix moritz jipilab mmasdeu ghbraunmath ghkliem added
comment:4 Changed 16 months ago by
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 16 months ago by
 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 16 months ago by
 Commit changed from 6a15a44b8702bcd05d441f794493be350ce461f4 to 516a62270a54e6d15f32e87241cf953a4f5daf75
Branch pushed to git repo; I updated commit sha1. New commits:
516a622  actually fixing our error

comment:7 Changed 16 months ago by
 Status changed from needs_work to needs_review
comment:8 Changed 16 months ago by
 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 16 months ago by
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 16 months ago by
 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 16 months ago by
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 16 months ago by
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 16 months ago by
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 16 months ago by
 Branch changed from public/28872 to 516a62270a54e6d15f32e87241cf953a4f5daf75
 Resolution set to fixed
 Status changed from positive_review to closed
For the record:
The underlying normaliz error is