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

Containment checks are wrong for empty polyhedra #10238

Closed
novoselt opened this issue Nov 8, 2010 · 10 comments
Closed

Containment checks are wrong for empty polyhedra #10238

novoselt opened this issue Nov 8, 2010 · 10 comments

Comments

@novoselt
Copy link
Member

novoselt commented Nov 8, 2010

sage: p = Polyhedron()
sage: p
The empty polyhedron in QQ^0.
sage: p.contains(0)
True
sage: p.contains(1)
True
sage: p.contains([0, 1])
True
sage: p.contains([(0, 1)])
True
sage: p.interior_contains([0, 1])
True
sage: p.relative_interior_contains([0, 1])
True

I think all of the above should return False.

CC: @vbraun @novoselt

Component: geometry

Author: Volker Braun

Reviewer: Andrey Novoseltsev

Merged: sage-4.6.1.alpha1

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

@vbraun
Copy link
Member

vbraun commented Nov 8, 2010

comment:1

I changed the "empty" polyhedron to contain one equation 0==-1. I also modified the "contains" checks to be slightly more general and work with points whose coordinates are not necessarily in the same field as the polyhedron.

@novoselt
Copy link
Member Author

novoselt commented Nov 8, 2010

comment:2

Replying to @vbraun:

I changed the "empty" polyhedron to contain one equation 0==-1.

This seems reasonable, but it shows in the doctest as "[An equation () x + -1 == 0]" which is confusing: what is "() x" and why "+ -"?

This is not directly related to this ticket, but how about omitting "() x +" for 0-dimensional equations/inequalities and taking into account the "+ -" situation in general?

@novoselt
Copy link
Member Author

novoselt commented Nov 8, 2010

Reviewer: Andrey Novoseltsev

@novoselt
Copy link
Member Author

novoselt commented Nov 8, 2010

Author: Volker Braun

@vbraun
Copy link
Member

vbraun commented Nov 8, 2010

comment:4

I guess this ticket is as good as any other to fix the string 'repr()' of (in)equalities :-) The updated patch skips "zero coefficient times x" and handles the sign of the constant term nicer.

@novoselt
Copy link
Member Author

novoselt commented Nov 8, 2010

comment:5

Patch order issues:

applying trac_10238_containment_check_for_empty_polyhedra.patch
patching file sage/geometry/polyhedra.py
Hunk #8 succeeded at 4208 with fuzz 2 (offset 446 lines).
now at: trac_10238_containment_check_for_empty_polyhedra.patch

(Tested on top of face lattice.)

I am fine with this version, but just to make sure: you really want to start the line with "+" if there are no variables and have space between the sign and the coefficient only if there are variables?

@vbraun
Copy link
Member

vbraun commented Nov 9, 2010

comment:6

I rearranged the patches an in #9604, should apply cleanly now.

My preferred prettyprinting is

  An equation (-1) x + 1 >= 0
  An equation (2) x - 1 >= 0
  An equation 1 == 0
  An equation -1 == 0

and I fixed the 3rd one in the patch.

@vbraun
Copy link
Member

vbraun commented Nov 9, 2010

Attachment: trac_10238_containment_check_for_empty_polyhedra.patch.gz

Updated patch

@novoselt
Copy link
Member Author

novoselt commented Nov 9, 2010

comment:7

Cool!

@jdemeyer
Copy link

Merged: sage-4.6.1.alpha1

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

3 participants