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

#29843 introduces a bug in Polyhedron().linear_transformation #30146

Closed
Etn40ff opened this issue Jul 14, 2020 · 13 comments
Closed

#29843 introduces a bug in Polyhedron().linear_transformation #30146

Etn40ff opened this issue Jul 14, 2020 · 13 comments

Comments

@Etn40ff
Copy link
Contributor

Etn40ff commented Jul 14, 2020

In 9.2.beta5 applying linear transformations to a polyhedron containing a ray is broken:

sage: Polyhedron(rays=[(0,1)]).linear_transformation(identity_matrix(2))
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-185-3c5f2df4fe7c> in <module>()
----> 1 Polyhedron(rays=[(Integer(0),Integer(1))]).linear_transformation(identity_matrix(Integer(2)))

/opt/sage/sage/local/lib/python3.7/site-packages/sage/geometry/polyhedron/base.py in linear_transformation(self, linear_transf, new_base_ring)
   5093 
   5094             sage: P = Polyhedron([(0,0),(1,1)], base_ring=ZZ)
-> 5095             sage: P.intersection(P)
   5096             A 1-dimensional polyhedron in ZZ^2 defined as the convex hull of 2 vertices
   5097             sage: Q = Polyhedron([(0,1),(1,0)], base_ring=ZZ)

/opt/sage/sage/local/lib/python3.7/site-packages/sage/geometry/polyhedron/base.py in __init__(self, parent, Vrep, Hrep, Vrep_minimal, Hrep_minimal, pref_rep, **kwds)
    218         # TODO: find something better *but* fast
    219         return hash((self.dim(),
--> 220                      self.ambient_dim(),
    221                      self.n_Hrepresentation(),
    222                      self.n_Vrepresentation(),

/opt/sage/sage/local/lib/python3.7/site-packages/sage/geometry/polyhedron/backend_ppl.py in _init_from_Vrepresentation(self, vertices, rays, lines, minimize, verbose)
     75             d = LCM_list([denominator(r_i) for r_i in r])
     76             if d.is_one():
---> 77                 gs.insert(ray(Linear_Expression(r, 0)))
     78             else:
     79                 dr = [ d*r_i for r_i in r ]

ppl/generator.pyx in ppl.generator.Generator.ray()

ppl/generator.pyx in ppl.generator.Generator.ray()

ValueError: PPL::ray(e):
e == 0, but the origin cannot be a ray.

this used to work just fine in 9.2.beta4.

git bisect seems to blame the regression on #29843

CC: @jplab @LaisRast @kliem @mkoeppe

Component: geometry

Keywords: combinatorial polyhedron, linear transformation

Author: Jonathan Kliem

Branch/Commit: c9c7b63

Reviewer: Matthias Koeppe

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

@kliem
Copy link
Contributor

kliem commented Jul 15, 2020

comment:2

Thanks for catching this. This is another friendly reminder that working on those test suites is really useful.

@kliem
Copy link
Contributor

kliem commented Jul 15, 2020

Commit: 4eff413

@kliem
Copy link
Contributor

kliem commented Jul 15, 2020

Author: Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Jul 15, 2020

New commits:

07e4ab1fix bog introduced by 39843
4eff413add tiny testsuite to doctest

@kliem
Copy link
Contributor

kliem commented Jul 15, 2020

Branch: public/30146

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2020

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

7fe6b43One transposition only

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 15, 2020

Changed commit from 4eff413 to 7fe6b43

@kliem
Copy link
Contributor

kliem commented Jul 15, 2020

comment:5

Replying to @sagetrac-git:

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

7fe6b43One transposition only

I was thinking about that one. At first glance this might be a bit weird, because the matrix is supposed to act from the left. However it is shorter and easier to read I guess (and slightly faster I suppose).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 19, 2020

Changed commit from 7fe6b43 to c9c7b63

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 19, 2020

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

c9c7b63missed preceeding `sage:` of doctests

@mkoeppe
Copy link
Member

mkoeppe commented Jul 22, 2020

Reviewer: Matthias Koeppe

@kliem
Copy link
Contributor

kliem commented Jul 22, 2020

comment:8

Thank you.

@vbraun
Copy link
Member

vbraun commented Jul 28, 2020

Changed branch from public/30146 to c9c7b63

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