Opened 3 years ago
Closed 3 years ago
#28724 closed enhancement (fixed)
Polyhedron._acted_upon_ should handle left multiplication by matrices, linear transformations
Reported by:  Matthias Köppe  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  geometry  Keywords:  polytopes 
Cc:  JeanPhilippe Labbé, Jiawei Wang  Merged in:  
Authors:  JeanPhilippe Labbé, Jonathan Kliem  Reviewers:  Jonathan Kliem, JeanPhilippe Labbé 
Report Upstream:  N/A  Work issues:  
Branch:  a1072ee (Commits, GitHub, GitLab)  Commit:  a1072eea4beeb9da56c418c7aeb264a2b587376f 
Dependencies:  #28770  Stopgaps: 
Description (last modified by )
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: Vrepresentation data requires a list of length ambient_dim
Change History (18)
comment:1 Changed 3 years ago by
Description:  modified (diff) 

comment:2 Changed 3 years ago by
comment:4 Changed 3 years ago by
Authors:  → JeanPhilippe Labbé 

Branch:  → public/28724 
Commit:  → 755d5c45caa4ddbc310447272fea325d3026a059 
Status:  new → needs_review 
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:
755d5c4  First version of linear transformation

comment:5 Changed 3 years ago by
What happens when the linear transformation moves the polyhedron outside of the fields the backend can handle? Can you add such a doctest?
comment:6 Changed 3 years ago by
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.
comment:7 Changed 3 years ago by
Status:  needs_review → needs_work 

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.
comment:8 Changed 3 years ago by
Dependencies:  → #28770 

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.
comment:9 Changed 3 years ago by
Three more things.
is_zero
catches too many matrices. A 2by2 zeromatrix 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
isFalse
.
comment:10 Changed 3 years ago by
Branch:  public/28724 → public/28724reb 

Commit:  755d5c45caa4ddbc310447272fea325d3026a059 → 1e68f1eaa66e10c8848442d6e4e9b11d0dd17480 
Status:  needs_work → needs_review 
comment:11 Changed 3 years ago by
Milestone:  sage9.0 → sage9.1 

Ticket retargeted after milestone closed
comment:12 Changed 3 years ago by
One probably should return anything, if self is on left.
comment:13 Changed 3 years ago by
Commit:  1e68f1eaa66e10c8848442d6e4e9b11d0dd17480 → eca962b6f7f08f8e0770d0ac13452792ed82ca55 

Branch pushed to git repo; I updated commit sha1. New commits:
eca962b  matrix must act from the left; improved documentation

comment:14 Changed 3 years ago by
Branch:  public/28724reb → public/28724reb2 

Commit:  eca962b6f7f08f8e0770d0ac13452792ed82ca55 → a1072eea4beeb9da56c418c7aeb264a2b587376f 
comment:15 Changed 3 years ago by
Authors:  JeanPhilippe Labbé → JeanPhilippe Labbé, Jonathan Kliem 

Reviewers:  → Jonathan Kliem, JeanPhilippe Labbé 
comment:16 Changed 3 years ago by
Bots are pretty much green. Some pyflakes warning, which I fixed in #28880 already (which is green botwise).
comment:18 Changed 3 years ago by
Branch:  public/28724reb2 → a1072eea4beeb9da56c418c7aeb264a2b587376f 

Resolution:  → fixed 
Status:  positive_review → closed 
Agreed, that would be nice to add this in the coercion.