Opened 17 months ago
Closed 14 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: | sage-9.1 |
Component: | geometry | Keywords: | polytopes |
Cc: | jipilab, gh-jiawei-wang-ucd | Merged in: | |
Authors: | Jean-Philippe Labbé, Jonathan Kliem | Reviewers: | Jonathan Kliem, Jean-Philippe 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: V-representation data requires a list of length ambient_dim
Change History (18)
comment:1 Changed 17 months ago by
- Description modified (diff)
comment:2 Changed 17 months ago by
comment:3 Changed 17 months ago by
Are you on it? I could come up with something.
comment:4 Changed 17 months 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 17 months 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 17 months 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 17 months 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 17 months 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 17 months ago by
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
isFalse
.
comment:10 Changed 15 months ago by
- Branch changed from public/28724 to public/28724-reb
- Commit changed from 755d5c45caa4ddbc310447272fea325d3026a059 to 1e68f1eaa66e10c8848442d6e4e9b11d0dd17480
- Status changed from needs_work to needs_review
comment:11 Changed 15 months ago by
- Milestone changed from sage-9.0 to sage-9.1
Ticket retargeted after milestone closed
comment:12 Changed 15 months ago by
One probably should not return anything, if self is on left.
comment:13 Changed 15 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 15 months ago by
- Branch changed from public/28724-reb to public/28724-reb2
- Commit changed from eca962b6f7f08f8e0770d0ac13452792ed82ca55 to a1072eea4beeb9da56c418c7aeb264a2b587376f
comment:15 Changed 15 months ago by
- Reviewers set to Jonathan Kliem, Jean-Philippe Labbé
comment:16 Changed 15 months ago by
Bots are pretty much green. Some pyflakes warning, which I fixed in #28880 already (which is green botwise).
comment:17 Changed 15 months ago by
- Status changed from needs_review to positive_review
Looks good to me.
comment:18 Changed 14 months ago by
- Branch changed from public/28724-reb2 to a1072eea4beeb9da56c418c7aeb264a2b587376f
- Resolution set to fixed
- Status changed from positive_review to closed
Agreed, that would be nice to add this in the coercion.