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

Polyhedron._acted_upon_ should handle left multiplication by matrices, linear transformations #28724

Closed
mkoeppe opened this issue Nov 12, 2019 · 28 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Nov 12, 2019

sage: b3 = polytopes.Birkhoff_polytope(3)
sage: b3.ambient_dim()
9
sage: proj_mat=matrix([[0,1,0,0,0,0,0,0,0],[0,0,0,1,0,0,0,0,0],[0,0,0,0,0,1,0,0,0],[0,0,0,0,0,0,0,1,0]])
b3_proj = proj_mat * b3
Traceback (most recent call last)
.....
.... sage/geometry/polyhedron/base.pyc in _acted_upon_(self, actor, self_on_left)
   3687             return self.translation(actor)
   3688         else:
-> 3689             return self.dilation(actor)
   3690 
   3691     def __neg__(self):
.....
ValueError: V-representation data requires a list of length ambient_dim

Depends on #28770

CC: @jplab @jiawei-wang-ucd

Component: geometry

Keywords: polytopes

Author: Jean-Philippe Labbé, Jonathan Kliem

Branch/Commit: a1072ee

Reviewer: Jonathan Kliem, Jean-Philippe Labbé

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

@mkoeppe mkoeppe added this to the sage-9.0 milestone Nov 12, 2019
@mkoeppe

This comment has been minimized.

@jplab
Copy link

jplab commented Nov 13, 2019

comment:2

Agreed, that would be nice to add this in the coercion.

@jplab
Copy link

jplab commented Nov 13, 2019

comment:3

Are you on it? I could come up with something.

@jplab
Copy link

jplab commented Nov 13, 2019

Author: Jean-Philippe Labbé

@jplab
Copy link

jplab commented Nov 13, 2019

comment:4

Here's a first version.

I wanted to have a linear_transformation method for a while... That was a good timing to add it.


New commits:

755d5c4First version of linear transformation

@jplab
Copy link

jplab commented Nov 13, 2019

Commit: 755d5c4

@jplab
Copy link

jplab commented Nov 13, 2019

Branch: public/28724

@tscrim
Copy link
Collaborator

tscrim commented Nov 15, 2019

comment:5

What happens when the linear transformation moves the polyhedron outside of the fields the backend can handle? Can you add such a doctest?

@kliem
Copy link
Contributor

kliem commented Nov 17, 2019

comment:6

Can you just initialize the new parent like this?

new_parent = par.base_extend(linear_transf.base_ring(), ambient_dim=new_dim)

This should automatically keep the backend if possible, but change it, if not possible.

@kliem
Copy link
Contributor

kliem commented Nov 19, 2019

comment:7

This does not work, when the matrix consists of floats.

Please apply the change I suggested, which fixes this, and add a doctest with floats.

However, this still does not work with sqrt5 for example, even with backend field or normaliz.

This is because coercion of parents does not work in this case. I'm trying to figure out what's going on there.

@kliem
Copy link
Contributor

kliem commented Nov 19, 2019

comment:8

I added #28770 as a dependency, because it would be nice to have doctest with a square root of something or so.

If you think this is unnecessary, feel free to remove it again.

@kliem
Copy link
Contributor

kliem commented Nov 19, 2019

Dependencies: #28770

@kliem
Copy link
Contributor

kliem commented Nov 19, 2019

comment:9

Three more things.

  • is_zero catches too many matrices. A 2-by-2 zero-matrix should not change ambient dimension, if it acts on a Polyhedron in ambient dim 2. if it acts on a polyhedron of dimension 3 it should throw an error.
  • (Optional) Are you happy with the error message thrown, if the dimension of the matrix does not fit? If not you should take care of it.
  • Currently, you implemented left action (and I don't know, what a right action should be). You should probably check, whether self_on_left is False.

@kliem
Copy link
Contributor

kliem commented Jan 6, 2020

comment:10

I applied those small changes as suggested above.

The matrix acts on the right now, by action of the transpose on the left.


New commits:

0346231First version of linear transformation
e80556csmall changes
1e68f1emore consistent behaviour for empty matrix

@kliem
Copy link
Contributor

kliem commented Jan 6, 2020

Changed commit from 755d5c4 to 1e68f1e

@kliem
Copy link
Contributor

kliem commented Jan 6, 2020

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

@embray
Copy link
Contributor

embray commented Jan 6, 2020

comment:11

Ticket retargeted after milestone closed

@embray embray modified the milestones: sage-9.0, sage-9.1 Jan 6, 2020
@kliem
Copy link
Contributor

kliem commented Jan 7, 2020

comment:12

One probably should not return anything, if self is on left.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 9, 2020

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

eca962bmatrix must act from the left; improved documentation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 9, 2020

Changed commit from 1e68f1e to eca962b

@kliem
Copy link
Contributor

kliem commented Jan 24, 2020

Changed branch from public/28724-reb to public/28724-reb2

@kliem
Copy link
Contributor

kliem commented Jan 24, 2020

New commits:

a521ca7First version of linear transformation
9385b30small changes
1a01fe3more consistent behaviour for empty matrix
7a694a0matrix must act from the left; improved documentation
a1072eegive a error message if self on left; better error check for zero_matrix

@kliem
Copy link
Contributor

kliem commented Jan 24, 2020

Changed commit from eca962b to a1072ee

@jplab
Copy link

jplab commented Jan 24, 2020

Reviewer: Jonathan Kliem, Jean-Philippe Labbé

@jplab
Copy link

jplab commented Jan 24, 2020

Changed author from Jean-Philippe Labbé to Jean-Philippe Labbé, Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Jan 27, 2020

comment:16

Bots are pretty much green. Some pyflakes warning, which I fixed in #28880 already (which is green botwise).

@jplab
Copy link

jplab commented Jan 31, 2020

comment:17

Looks good to me.

@vbraun
Copy link
Member

vbraun commented Feb 10, 2020

Changed branch from public/28724-reb2 to a1072ee

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

6 participants