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

Two bugs with dilation #29899

Closed
kliem opened this issue Jun 19, 2020 · 26 comments
Closed

Two bugs with dilation #29899

kliem opened this issue Jun 19, 2020 · 26 comments

Comments

@kliem
Copy link
Contributor

kliem commented Jun 19, 2020

The new dilation with #29200 discovered two bugs:

sage: 2*Polyhedron([[]], backend='cdd')
...
TypeError ...

and

sage: K.<sqrt2> = QuadraticField(2)
sage: sqrt2*Polyhedron(backend='normaliz')
...
AttributeError

The underlying errors are the following:

sage: from sage.geometry.polyhedron.backend_cdd import Polyhedron_QQ_cdd
sage: from sage.geometry.polyhedron.parent import Polyhedra_QQ_cdd
sage: parent = Polyhedra_QQ_cdd(QQ, 0, 'cdd')
sage: p = Polyhedron_QQ_cdd(parent, [iter([]), iter([]), iter([])], None)
Traceback (most recent call last):
...
TypeError: can't multiply sequence by non-int of type 'NoneType'

sage: Polyhedron(ieqs=[], ambient_dim=5, backend='cdd')
Traceback (most recent call last):
...
TypeError: unsupported operand type(s) for -=: 'NoneType' and 'int'

sage: from sage.geometry.polyhedron.parent import Polyhedra_RDF_cdd
sage: from sage.geometry.polyhedron.backend_cdd import Polyhedron_RDF_cdd
sage: parent = Polyhedra_RDF_cdd(RDF, 1, 'cdd')
sage: Vrep = [[], [], [[1.0]]]
sage: Hrep = [[], []]
sage: p = Polyhedron_RDF_cdd(parent, Vrep, Hrep,
....:                        Vrep_minimal=True, Hrep_minimal=True) 
Traceback (most recent call last):
...
TypeError: unsupported operand type(s) for -=: 'NoneType' and 'int'

There are two tiny fixes to it:

  • Make sure to special-case the empty polyhedron in Polyhedron_base.__init__.
  • Add a trivial inequality for initialization of the universe polyhedron with backend cdd.

We add doctests for each fix. Note that #29907 will also indirectly test this.

CC: @jplab @LaisRast

Component: geometry

Keywords: polyhedra, dilation

Author: Jonathan Kliem

Branch: 0bb6413

Reviewer: Matthias Koeppe

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

@kliem kliem added this to the sage-9.2 milestone Jun 19, 2020
@kliem
Copy link
Contributor Author

kliem commented Jun 19, 2020

New commits:

e41d698fix universe from Hrep for cdd
e055bc7correctly detect the empty polyhedron

@kliem
Copy link
Contributor Author

kliem commented Jun 19, 2020

Commit: e055bc7

@kliem
Copy link
Contributor Author

kliem commented Jun 19, 2020

Branch: public/29899

@kliem
Copy link
Contributor Author

kliem commented Jun 19, 2020

comment:2

What also doesn't work for cdd and this is how the TestSuite detected the problem is 2*Polyhedron(backend='cdd'). The problem is that it tries to discover the action via

parent(Polyhedron(backend='cdd')).an_element(), which is Polyhedron([[]], backend='cdd').

@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Jun 19, 2020

comment:3

There is no thing as a trivial equation of course.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2020

Changed commit from e055bc7 to 7eba64d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2020

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

7eba64dtrivial inequality

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2020

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

067fb16also fix it for initialization from Hrep and Vrep

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 19, 2020

Changed commit from 7eba64d to 067fb16

@kliem
Copy link
Contributor Author

kliem commented Jun 19, 2020

comment:6

Again, the branch that I push into public/29842 in a second really tests those things. Otherwise, I wouldn't have discovered this stuff in the first place.

@kliem

This comment has been minimized.

@mkoeppe
Copy link
Member

mkoeppe commented Jun 22, 2020

comment:8

I think you should add a doctest that more specifically illustrates this subtle point of the behavior of Polyhedron_base.__init__ when generators are involved.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2020

Changed commit from 067fb16 to e94f71c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 22, 2020

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

e94f71cadd doctests

@kliem

This comment has been minimized.

@mkoeppe
Copy link
Member

mkoeppe commented Jun 22, 2020

comment:11

inequalites -> inequalities.

Also could you rephrase/expand the comment involving "The damage is limited." I don't fully understand it

@kliem
Copy link
Contributor Author

kliem commented Jun 22, 2020

comment:12

I will improve this.

The reason for using generators is that we don't have to build a list/tuple that is possibly discarded. Once we are there, we need to generate all the generators. Of course it might be better if you can really just generate them, when you feed them to the backend, but the backends require that you don't call _init_from_Vrepresentation, when vertices and rays and lines are empty. So we should respect that.

@kliem
Copy link
Contributor Author

kliem commented Jul 5, 2020

Changed commit from e94f71c to 0bb6413

@kliem
Copy link
Contributor Author

kliem commented Jul 5, 2020

Changed branch from public/29899 to public/29899-reb

@kliem
Copy link
Contributor Author

kliem commented Jul 5, 2020

New commits:

53a64b0Merge branch 'public/29899' of git://trac.sagemath.org/sage into public/29899-reb
0bb6413documentation

@mkoeppe
Copy link
Member

mkoeppe commented Jul 19, 2020

Reviewer: Matthias Koeppe

@kliem
Copy link
Contributor Author

kliem commented Jul 19, 2020

comment:16

Thank you.

@vbraun
Copy link
Member

vbraun commented Jul 25, 2020

Changed branch from public/29899-reb to 0bb6413

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Jul 26, 2020

Changed commit from 0bb6413 to none

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

4 participants