Opened 2 years ago
Closed 21 months ago
#28724 closed enhancement (fixed)
Polyhedron._acted_upon_ should handle left multiplication by matrices, linear transformations
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  geometry  Keywords:  polytopes 
Cc:  jipilab, ghjiaweiwangucd  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 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
comment:3 Changed 2 years ago by
Are you on it? I could come up with something.
comment:4 Changed 2 years ago by
 Branch set to public/28724
 Commit set to 755d5c45caa4ddbc310447272fea325d3026a059
 Status changed from new to 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 2 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 2 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 2 years ago by
 Status changed from needs_review to 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 2 years ago by
 Dependencies set to #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 2 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 22 months ago by
 Branch changed from public/28724 to public/28724reb
 Commit changed from 755d5c45caa4ddbc310447272fea325d3026a059 to 1e68f1eaa66e10c8848442d6e4e9b11d0dd17480
 Status changed from needs_work to needs_review
comment:11 Changed 22 months ago by
 Milestone changed from sage9.0 to sage9.1
Ticket retargeted after milestone closed
comment:12 Changed 22 months ago by
One probably should not return anything, if self is on left.
comment:13 Changed 22 months ago by
 Commit changed from 1e68f1eaa66e10c8848442d6e4e9b11d0dd17480 to eca962b6f7f08f8e0770d0ac13452792ed82ca55
Branch pushed to git repo; I updated commit sha1. New commits:
eca962b  matrix must act from the left; improved documentation

comment:14 Changed 21 months ago by
 Branch changed from public/28724reb to public/28724reb2
 Commit changed from eca962b6f7f08f8e0770d0ac13452792ed82ca55 to a1072eea4beeb9da56c418c7aeb264a2b587376f
comment:15 Changed 21 months ago by
 Reviewers set to Jonathan Kliem, JeanPhilippe Labbé
comment:16 Changed 21 months ago by
Bots are pretty much green. Some pyflakes warning, which I fixed in #28880 already (which is green botwise).
comment:17 Changed 21 months ago by
 Status changed from needs_review to positive_review
Looks good to me.
comment:18 Changed 21 months ago by
 Branch changed from public/28724reb2 to a1072eea4beeb9da56c418c7aeb264a2b587376f
 Resolution set to fixed
 Status changed from positive_review to closed
Agreed, that would be nice to add this in the coercion.