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.

Sign up for GitHub

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

Jump to bottom

Rigid Transformations #39

Merged
merged 11 commits into from
Feb 7, 2017
Merged

Rigid Transformations #39

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
324b850
ENH: Add rotate function to the Region and Polytope.
carterbox Jan 3, 2017
af401d8
ENH: Add translate function to Polytope and Region.
carterbox Jan 10, 2017
aff57bf
DOC: Reformat documentation, TODO, and log messages for rotate.
carterbox Jan 12, 2017
7f069da
MAINT: Rename functions to tranlation, rotation, _translate, and _rot…
carterbox Jan 21, 2017
79d85ec
BUG: Change `cvxopt.glpk` output to match `scipy` solver output.
carterbox Jan 29, 2017
cb9e9ed
DOC: Improve _rotate docs for third case.
carterbox Jan 29, 2017
bf65d33
DOC: Add epydoc to translation functions.
carterbox Feb 2, 2017
fa7fda9
DOC: Refine rotation and translation docs.
carterbox Feb 4, 2017
173106e
DOC: Change two vector rotation angle specification.
carterbox Feb 4, 2017
65245d9
DOC: Fix some typos in rotation docs.
carterbox Feb 5, 2017
5d4b036
API: Change u, v to i, j in rotate functions.
carterbox Feb 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 64 additions & 48 deletions polytope/polytope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,26 +291,34 @@ def intersect(self, other, abs_tol=ABS_TOL):
return reduce(Polytope(iA, ib), abs_tol=abs_tol)

def translation(self, d):
"""Translate the Polytope by a given vector.
"""Returns a copy of C{self} translated by the vector C{d}.

Consult L{polytope.polytope._translate} for implementation details.

@type d: 1d array
@param d: The translation vector.

@rtype: L{Polytope}
@return: A translated copy of the Polytope.
"""
newpoly = self.copy()
_translate(newpoly, d)
return newpoly

def rotation(self, u, v, theta=None):
"""Rotate the Polytope. Only simple rotations are implemented at this
"""Returns a rotated copy of C{self}.

Use one of two methods that to describe the plane of rotation and the
angle of rotation (in radians) with u, v, and theta.

(1) u and v are the indicies (0, N] of two basis vectors, and theta is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should that be [0, N)? I am also happy with [0,N[.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or 0..N-1 (as in TLA+)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that 0..(N-1) or 0..N-1 are correct, but [0, N) is ambiguous, because the indices should be integers. In a typed formalism, one could claim to have declared that u, v are of "integer type" (whatever that may mean). However, in polytope we have neither type declarations of any form, nor are integers the norm. So, I think that 0..(N-1) avoids this ambiguity. Explicit is better than implicit (PEP 20).

In a context that involves real numbers, I prefer [0, N) to [0, N[, because I find it easier to distinguish a a closing parentheses ) from a closing bracket ], than an opening bracket [ from a closing bracket ]. I do appreciate the symmetry of the latter notation though.

I believe that "indicies" is a typo. Both "indices" and "indexes" appear to be used as plural form of "index". A remark about the same was made earlier.

Also, I think that if one method is supported now, then No.2 should be omitted. Why have the user read two items, and then find out the 2nd isn't available?

I am opposed to the polymorphism of u, v. I had to read No.1 multiple times to convince myself that I understand correctly. u, v should better have one type in each signature. If they are integer indices, it is more familiar to name them i, j or m, n.

"basis" could be ambiguous. We could specify that the basis is the same as that used to express the coordinates.

Relevant for those interested in untypeness: omega is in transition from typed to untyped (cf tulip-control/omega#3).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Special cases aren't special enough to break the rules [PEP 20]. It seems more uniform to let u, v be vectors. This way:

  1. the interface remains unchanged when the implementation is extended to handle arbitrary vectors
  2. an UnimplementedError is raised if vectors outside the intended basis are passed
  3. users don't have to think about any particular basis
  4. index-based thinking is avoided. This is more Pythonic and more geometric / structural.

Defining the basis vectors u0, u1 etc in user code is relatively simple, and it also names them, so it is probably easier to read (as opposed to some integers 0, 1, ...).

Copy link
Contributor Author

@carterbox carterbox Feb 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slivingston That is correct, I meant to type 0..N-1.

@johnyf "indicies" is a typo. I keep spelling it wrong. I agree that polymorphic u, v probably isn't the best. However, I'm not going to get rid of rotation by indexed basis vector because the current implementation is based around that special case. What I will do is rename the parameters to i, j, and hide the second method. When I figure out how to implement the second option, the parameters u, v will reappear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I had in mind was raising an error if either u or v isn't a basis vector. If both are basis vectors, then convert them to indices, and proceed with the rest of the computation as currently done. Your current decision yields a simpler interface, and simple is better than complex (PEP 20).

the angle of rotation.

(2) u and v are two unit vectors, angle of rotation is the angle
between them from u to v. NOTE: This method is not implemented at this
time.

@rtype: L{Polytope}
@return: A rotated copy of the Polytope.
Consult L{polytope.polytope._rotate} for more detail.

@type u: number, 1d array
@type v: number, 1d array
@type theta: number
"""
newpoly = self.copy()
_rotate(newpoly, u, v, theta)
Expand Down Expand Up @@ -469,13 +477,9 @@ def text(self, txt, ax=None, color='black'):


def _translate(polyreg, d):
"""Translate a polyreg by a vector in place. Does not return a copy.
"""Translate C{polyreg} by the vector C{d}. Modifies C{polyreg} in-place.

@type d: 1d array
@param d: The translation vector.

@type polyreg: L{Polytope} or L{Region}
@param polyreg: The polytope or region to be translated.
"""

if isinstance(polyreg, Polytope):
Expand All @@ -495,43 +499,47 @@ def _translate(polyreg, d):


def _rotate(polyreg, u=None, v=None, theta=None, R=None):
"""Rotate this polyreg in place. Return the rotation matrix.

Simple rotations, by definition, occur only in 2 of the N dimensions; the
other N-2 dimensions are invariant with the rotation. Any rotations thus
require specification of the plane(s) of rotation. This function has three
different ways to specify this plane:

(1) Providing the indices [0, N) of the orthogonal basis vectors which
define the plane of rotation and an angle of rotation (theta) between them.
This allows easy construction of the Givens rotation matrix. The right hand
rule defines the positive rotation direction.

(2) Providing two unit vectors with no angle. The two vectors are contained
within a plane and the degree of rotation is the angle which moves the
first vector, u, into alignment with the second vector, v. NOTE: This
method is not implemented at this time.

(3) Providing an NxN rotation matrix, R. WARNING: No checks are made to
"""Rotate C{polyreg} in-place. Return the rotation matrix.

There are two types of rotation: simple and compound. For simple rotations,
by definition, all motion can be projected as circles in a single plane;
the other N-2 dimensions are invariant. Therefore any simple rotation can
be parameterized by its plane of rotation. Compound rotations are the
combination of multiple simple rotations; they have more than one plane of
rotation. For N > 3 dimensions, a compound rotation may be necessary to map
one orientation to another (Euler's rotation theory no longer applies).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would "Euler's rotation theorem" be more appropriate here, rather than "Euler's rotation theory"?


Use one of the following three methods to specify rotation. The first two
can only express simple rotation, but simple rotations may be applied in a
sequence to acheive a compound rotation.

(1) Provide the indices [0, N) of the orthogonal basis vectors, u and v,
which define the plane of rotation and a radian angle of rotation, theta,
between them. This method contructs the Givens rotation matrix. The right
hand rule defines the positive rotation direction.

(2) Provide two unit vectors, The two vectors define the plane of rotation
and angle of rotation is the angle which moves the first vector, u, into
alignment with the second vector, v. NOTE: This method is not implemented
at this time.

(3) Provide an NxN rotation matrix, R. WARNING: No checks are made to
determine whether the provided transformation matrix is a valid rotation.

Further Reading
https://en.wikipedia.org/wiki/Plane_of_rotation

@type polyreg: L{Polytope} or L{Region}
@param polyreg: The polytope or region to be rotated.
@type u: number, 1d array
@type polyreg: L{Polytope} or L{Region}
@param u: The first index or vector describing the plane of rotation.
@type v: number, 1d array
@param u: The second index or vector describing the plane of rotation.
@type theta: number
@type u: number, 1d array
@param u: The second index or vector describing the plane of rotation
@type v: number, 1d array.
@param theta: The radian angle to rotate the polyreg in the plane defined
by u and v.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the description for theta be updated to "...defined by i and j" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I have fixed this.

@type R: 2d array
@type theta: number
@param R: A predefined rotation matrix.

@rtype: 2d array
@return: The matrix used to rotate the polyreg.
@type R: 2d array
"""
# determine the rotation matrix based on inputs
if R is not None:
Expand Down Expand Up @@ -775,26 +783,34 @@ def intersect(self, other, abs_tol=ABS_TOL):
return P

def rotation(self, u, v, theta=None):
"""Rotate this Region. Only simple rotations are implemented at this
"""Returns a rotated copy of C{self}.

Use one of two methods that to describe the plane of rotation and the
angle of rotation (in radians) with u, v, and theta.

(1) u and v are the indicies (0, N] of two basis vectors, and theta is
the angle of rotation.

(2) u and v are two unit vectors, angle of rotation is the angle
between them from u to v. NOTE: This method is not implemented at this
time.

@rtype: L{Region}
@return: A translated copy of the Region.
Consult L{polytope.polytope._rotate} for more detail.

@type u: number, 1d array
@type v: number, 1d array
@type theta: number
"""
newreg = self.copy()
_rotate(newreg, u, v, theta)
return newreg

def translation(self, d):
"""Translate this Region by a given vector.
"""Returns a copy of C{self} translated by the vector C{d}.

Consult L{polytope.polytope._translate} for implementation details.

@type d: 1d array
@param d: The translation vector.

@rtype: L{Region}
@return: A translated copy of the Region.
"""
newreg = self.copy()
_translate(newreg, d)
Expand Down