Opened 3 years ago
Closed 3 years 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 3 years ago by
comment:2 Changed 3 years 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 3 years ago by
 Cc tscrim dimpase novoselt Winfried nthiery vdelecroix moritz jipilab mmasdeu ghbraunmath ghkliem added
comment:4 Changed 3 years 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 3 years 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 3 years 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 3 years ago by
 Status changed from needs_work to needs_review
comment:8 Changed 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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