Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normaliz backend is broken with double description input #30248

Closed
jplab opened this issue Jul 29, 2020 · 14 comments
Closed

Normaliz backend is broken with double description input #30248

jplab opened this issue Jul 29, 2020 · 14 comments

Comments

@jplab
Copy link
Contributor

jplab commented Jul 29, 2020

This bug was found while manipulating hyperplane arrangements.

A minimal example to reproduce the bug is:

sage: p1 = Polyhedron(backend='normaliz', base_ring=AA, rays=[(AA(0), AA(0), AA(1)), (AA(0), AA(1), AA(-1)), (AA(1), AA(0), AA(-1))], vertices=[(AA(0), AA(0), AA(0))])
sage: p1
A 3-dimensional polyhedron in AA^3 defined as the convex hull of 1 vertex and 3 rays
sage: -p1
A 3-dimensional polyhedron in AA^3 defined as the convex hull of 1 vertex and 3 lines
sage: p2 = Polyhedron(backend='normaliz', base_ring=AA, rays=[(AA(-1), AA(0), AA(1)), (AA(-1), AA(1), AA(0)), (AA(0), AA(0), AA(-1))], vertices=[(AA(0), AA(0), AA(0))])
sage: p2
A 3-dimensional polyhedron in AA^3 defined as the convex hull of 1 vertex and 3 rays
sage: -p2
A 3-dimensional polyhedron in AA^3 defined as the convex hull of 1 vertex and 3 lines

Looking at dilation it seems that dilation is not the problem, it does the right thing, but the parent.element_class call messes things up.

Notice that changing the base ring to QQ or ZZ or removing the 'normaliz' backend, one does not get the error... This is nasty.

In the hyperplane arrangement, there are some rational regions, and some irrational regions...

CC: @kliem @LaisRast @mkoeppe @tscrim

Component: geometry

Keywords: polytope, backend, normaliz, hyperplane, regions

Author: Jean-Philippe Labbé

Branch/Commit: 83453fb

Reviewer: Travis Scrimshaw

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

@jplab jplab added this to the sage-9.2 milestone Jul 29, 2020
@kliem
Copy link
Contributor

kliem commented Jul 29, 2020

comment:1

Notice that normaliz doesn't use precomputed double description yet (waiting for the package upgrade). I suspect intitialzing from inequaltities doesn't work correctly and there is an even simpler one line example for this.

@kliem
Copy link
Contributor

kliem commented Jul 29, 2020

comment:2

What might be causing it, is passing a generator for the inequalries (in this case you cannot get the error with the normal constructor).

If we iterate twice through it (only for number fields), the second time you obviosly don't get anything, which explains the three lines.

One solution may be to make a tuple out of the generators, once we decided what we dispose in Polyhedron_base.__init__. The main reason for the generators is that we do not want to generate something, which we dispose of a few lines later.

@jplab
Copy link
Contributor Author

jplab commented Jul 30, 2020

comment:4

Found it.

Line 858 and 859 in backend_normaliz in the method _compute_nmz_data_lists_and_field, which is called by _init_H_representation.

The fact that the base ring is enforced to be AA while the data is really in QQ is taken care of by these 2 lines (normaliz should be used and not Qnormaliz). But! Since dilation ships 2 maps for the Hreps, the data_lists are empty at that point!

Ouff! Ok, I think this shouldn't be too hard to fix.

...this is a sign that this use-case was not really tested... It pops up when one wants to deal with lots of sorts of polyhedra and eventually need to fix a base ring (done in Hyperplane arrangements). Oiii.

@kliem
Copy link
Contributor

kliem commented Jul 30, 2020

comment:5

I implemented a few test suite methods already. If you run a test suite on your polyhedron, does it succeed?

@jplab
Copy link
Contributor Author

jplab commented Jul 30, 2020

New commits:

83453fbFix Hrepr of normaliz for iterators

@jplab
Copy link
Contributor Author

jplab commented Jul 30, 2020

Commit: 83453fb

@jplab
Copy link
Contributor Author

jplab commented Jul 30, 2020

Branch: public/30248

@jplab
Copy link
Contributor Author

jplab commented Jul 30, 2020

comment:7

Replying to @kliem:

I implemented a few test suite methods already. If you run a test suite on your polyhedron, does it succeed?

Yes. It seems so.

@jplab
Copy link
Contributor Author

jplab commented Jul 30, 2020

Author: Jean-Philippe Labbé

@kliem
Copy link
Contributor

kliem commented Jul 30, 2020

comment:9

Replying to @jplab:

Replying to @kliem:

I implemented a few test suite methods already. If you run a test suite on your polyhedron, does it succeed?

Yes. It seems so.

OK. Then this is a friendly reminder to go on with it.

@jplab
Copy link
Contributor Author

jplab commented Aug 5, 2020

comment:10

Ping! I think it would be important to have this ticket in the next release...

@tscrim
Copy link
Collaborator

tscrim commented Aug 5, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 5, 2020

comment:11

LGTM.

@vbraun
Copy link
Member

vbraun commented Aug 9, 2020

Changed branch from public/30248 to 83453fb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants