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

Rigid Transformations #39

Merged
merged 11 commits into from
Feb 7, 2017
182 changes: 181 additions & 1 deletion polytope/polytope.py
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,32 @@ 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.
Copy link
Member

Choose a reason for hiding this comment

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

"by a given vector" leaves argument d unexplained until one reads @param d: The translation vector. It is more direct to write: "by the vector d". (or C{d}...). We can then omit @param d later, so the user has to read less.

"the Polytope" could be replaced with "self". This is precise, survives subclassing (a subclass of Polytope could be called MagicBox), and is shorter.

"Translate" (a verb in active voice) could lead a user to think of in place modification, and miss the @return at the bottom of the docstring. Conversely, the user should read the entire docstring to obtain the necessary information (this docstring is short, but I mean for other cases too).
This situation can be avoided by writing:

"Returns a copy of self translated by the vector d."

We can then omit @return and @rtype, because @return is now part of the docstring's title, and @rtype is communicated by "copy of self".

Putting these changes together, we obtain a shorter docstring:

"""Returns a copy of C{self} translated by the vector C{d}.

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

@type d: 1d array
"""

We could also make d more specific by naming a suitable type of numpy array.

Also, I would recommend for each argument x, to write @param x before @type x, because one is firstly interested in what a variable means, and secondly in what type it has. Python is untyped anyway, and using isinstance is discouraged, so @type is secondary information. The function should process any duck that fits the description.

We could regard @type as a "type hint", with the meaning described in Lamport and Paulson, "Should your specification language be typed?". Type hints have reached Python itself.

Under the convention that @type is a type hint, and not a rigid requirement, we can then be more specific in what type we mention (for example set, instead of "container"), thus demonstrating our typing intent with an example type, knowing that the user will not think of the example as a restriction.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with all of the comments and recommendations above, but I did not consider it required before this PR would be accepted because the meaning of "vector" can be inferred. Instead, we might return to docstrings later during quality assurance review of documentation.

Alternatively, the docstring that @johnyf pitches above is good for me, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


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
Copy link
Member

Choose a reason for hiding this comment

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

The arguments u and v are undefined.

Are "simple rotations" rotations in three-dimensional Euclidean space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that currently, "simple rotations" means "easily implemented rotations" or "rotations that can be achieved in one step".

I've read that for N dimensions, the path between any two orientations will require at least 1 and at most floor(N/2) rotations, so for N > 3, there is now a multi-step transformation required for arbitrary reorientation. I'm not sure yet why the rotation cannot be achieved in one step; since, by definition, all the motion can be projected into a plane. I'll have to do more reading when I implement the missing rotation method.

Copy link
Member

Choose a reason for hiding this comment

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

Finding how to phrase this restriction in a mathematically precise way would be informative for the reader of the docstring.

I am not sure I understand what the path between two orientations is. If we are interested in a function that maps the ambient Euclidean space to itself, so that points on the object in its initial configuration be mapped to "the same points on the object" (using coordinates fixed wrt to the object) in its final configuration, then such a function seems to always exist (we could multiply the transformation matrices).

It seems to depend on how the mapping is parameterized. If the mapping is expressed in terms of rotations, then an example that appears to require at least two rotations is a pen horizontal on a table, and the same pen vertical, but with its cap also rotated by 90 degrees around the pen's longitudinal axis. It may not be the case that any mapping from one orientation to another can be represented by a projection onto a hyperplane.

A reference that could be useful for studying three-dimensional transformations is Chapter 2 from the book "A mathematical introduction to robotic manipulation".

Copy link
Member

@slivingston slivingston Feb 3, 2017

Choose a reason for hiding this comment

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

I might be missing something, but I think that the comment above that u and v are not defined is redundant with my earlier comment that the documentation of hidden function _rotate should be available from that of the class method rotation.

Copy link
Member

Choose a reason for hiding this comment

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

My comment on u and v is redundant, I noticed it after posting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, a more mathematically rigorous description would probably be helpful.

When I say path, I mean "the series of simple rotations required to reorient from one orientation to another." By simple rotation, I revise my earlier definition to "a rotation that may be described by only one plane of rotation". These simple rotations are, as you said, reorientations that may be projected into a hyperplane. It is useful to also define compound rotation, "a rotation that has multiple planes of rotation and is the composition of multiple simple rotations".

Although Euler's rotation theory says that all reorientations in 3D are simple rotations, for N > 3 this is no longer true. As you said, the parameterization limits the capability of the mapping, and I believe that the current input parameters are not capable of expressing the compound rotations. In other words, the parameters of this function are only capable of expressing simple rotations at this time.

I guess the phase "at this time" is misleading. Will this function be expanded to include compound rotations in the future? No. I think it is better that only simple rotations are implemented, and users can perform compound rotations by applying a sequence of simple rotations.

Does this make sense? I will be using this explanation to clear up the docs.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, your explanation makes sense.

Though I am not aware of one yet, there might eventually be motivating use-cases for handling compound rotations. So, I do not regard "at this time" as misleading.

time.

@rtype: L{Polytope}
@return: A rotated copy of the Polytope.
"""
newpoly = self.copy()
_rotate(newpoly, u, v, theta)
return newpoly

def copy(self):
"""Return copy of this Polytope.
"""
Expand Down Expand Up @@ -442,6 +468,134 @@ def text(self, txt, ax=None, color='black'):
_plot_text(self, txt, ax, color)


def _translate(polyreg, d):
"""Translate a polyreg by a vector in place. Does not return a copy.
Copy link
Member

@johnyf johnyf Feb 2, 2017

Choose a reason for hiding this comment

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

"Modifies C{polyreg} in place" provides more information, and emphasizes that the object will change. Both "in-place" and "in place" appear in Python's own documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the context of those links, I think what you're telling me that "in-place" means "without moving" or "without a copy", and "in place" means "instead" such as when used as "in place of".

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, I was too quick to link to the second page. This "in place" from Python's documentation is used in the sense of "in-place" (on the same instance).

Copy link
Member

Choose a reason for hiding this comment

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

To clarify my intent, I am citing both forms to indicate that I don't know which one is more widely used. Wikipedia uses the hyphenated version. Your comment suggests that "in-place" may be a better choice.

Copy link
Member

Choose a reason for hiding this comment

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

I am happy with both "in-place" and "in place". Given there is no reliable distinction that we have found, we can wait for empirical feedback from users.

Copy link
Member

Choose a reason for hiding this comment

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

I did not consider it critical, but since we are debating "in-place" vs. "in place", I will add that "Does not return a copy" is incomplete. It should be given a subject, changed to the passive voice (e.g., "A copy is not returned."), or deleted because it is redundant with the previous sentence.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with keeping the first sentence because it suggests the second sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


@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):
# Translate hyperplanes
polyreg.b = polyreg.b + np.dot(polyreg.A, d)
else:
# Translate subregions
for poly in polyreg.list_poly:
_translate(poly, d)

# Translate bbox and cheby
if polyreg.bbox is not None:
polyreg.bbox = (polyreg.bbox[0] + d,
polyreg.bbox[1] + d)
if polyreg._chebXc is not None:
polyreg._chebXc = polyreg._chebXc + 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
Copy link
Member

Choose a reason for hiding this comment

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

I see that my guess above about what "simple rotation" means was off. Function _rotate is hidden, so some (minimal) mention in the docstring of method Polytope.rotation of what kind of rotations are supported would help a user.

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
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
@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
@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
@param R: A predefined rotation matrix.

@rtype: 2d array
@return: The matrix used to rotate the polyreg.
"""
# determine the rotation matrix based on inputs
if R is not None:
logger.info("rotate via predefined matrix.")

elif u is not None and v is not None:
if theta is not None:
logger.info("rotate via indices and angle.")
if u == v:
raise ValueError("Must provide two unique basis vectors.")
R = np.identity(polyreg.dim)
c = np.cos(theta)
s = np.sin(theta)
R[u, u] = c
R[v, v] = c
R[u, v] = -s
R[v, u] = s

else: # theta is None
logger.info("rotate via 2 vectors.")
# TODO: Assert vectors are non-zero and non-parallel aka exterior
# product is non-zero; then autocalculate the complex rotation
# required to align the first vector with the second.
raise NotImplementedError("Rotation via 2 vectors is not currently"
" available. See source for TODO.")

else:
raise ValueError("R or (u and v) must be defined.")

if isinstance(polyreg, Polytope):
# Ensure that half space is normalized before rotation
n, p = _hessian_normal(polyreg.A, polyreg.b)

# Rotate the hyperplane normals
polyreg.A = np.inner(n, R)
polyreg.b = p
else:
# Rotate subregions
for poly in polyreg.list_poly:
_rotate(poly, None, None, R=R)

# transform bbox and cheby
if polyreg.bbox is not None:
polyreg.bbox = (np.inner(polyreg.bbox[0].T, R).T,
np.inner(polyreg.bbox[1].T, R).T)
if polyreg._chebXc is not None:
polyreg._chebXc = np.inner(polyreg._chebXc, R)

return R


def _hessian_normal(A, b):
"""Normalizes a half space representation according to hessian normal form.
"""
L2 = np.reshape(np.linalg.norm(A, axis=1), (-1, 1)) # needs to be column
if any(L2 == 0):
raise ValueError('One of the rows of A is a zero vector.')

n = A / L2 # hyperplane normals
p = b / L2.flatten() # hyperplane distances from origin

return n, p


class Region(object):
"""Class for lists of convex polytopes

Expand Down Expand Up @@ -620,6 +774,32 @@ def intersect(self, other, abs_tol=ABS_TOL):
P = union(P, isect, check_convex=True)
return P

def rotation(self, u, v, theta=None):
"""Rotate this Region. Only simple rotations are implemented at this
time.

@rtype: L{Region}
@return: A translated copy of the Region.
"""
newreg = self.copy()
_rotate(newreg, u, v, theta)
return newreg

def translation(self, d):
"""Translate this Region by a given vector.

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)
return newreg

def __copy__(self):
"""Return copy of this Region."""
return Region(list_poly=self.list_poly[:],
Expand Down Expand Up @@ -2227,7 +2407,7 @@ def lpsolve(c, G, h):
result['status'] = 3
else:
result['status'] = 4
result['x'] = sol['x']
result['x'] = np.squeeze(sol['x'])
result['fun'] = sol['primal objective']
elif lp_solver == 'scipy':
sol = optimize.linprog(
Expand Down
100 changes: 99 additions & 1 deletion tests/polytope_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import logging

import numpy as np
from numpy.testing import assert_allclose
import polytope as pc


Expand All @@ -11,12 +12,30 @@
log.addHandler(logging.StreamHandler())


# unit square
# unit square in first quadrant
Ab = np.array([[0.0, 1.0, 1.0],
[0.0, -1.0, 0.0],
[1.0, 0.0, 1.0],
[-1.0, 0.0, 0.0]])

# unit square in second quadrant
Ab2 = np.array([[-1.0, 0.0, 1.0],
[1.0, 0.0, 0.0],
[0.0, 1.0, 1.0],
[0.0, -1.0, 0.0]])

# unit square in third quadrant
Ab3 = np.array([[0.0, 1.0, 0.0],
[0.0, -1.0, 1.0],
[1.0, 0.0, 0.0],
[-1.0, 0.0, 1.0]])

# unit square in fourth quadrant
Ab4 = np.array([[0.0, 1.0, 0.0],
[0.0, -1.0, 1.0],
[1.0, 0.0, 1.0],
[-1.0, 0.0, 0.0]])

A = Ab[:, 0:2]
b = Ab[:, 2]

Expand Down Expand Up @@ -48,5 +67,84 @@ def comparison_test():
np.array([1, 0, 0])))


def region_rotation_test():
p = pc.Region([pc.Polytope(A, b)])
p1 = pc.Region([pc.Polytope(A, b)])
p2 = pc.Region([pc.Polytope(Ab2[:, 0:2], Ab2[:, 2])])
p3 = pc.Region([pc.Polytope(Ab3[:, 0:2], Ab3[:, 2])])
p4 = pc.Region([pc.Polytope(Ab4[:, 0:2], Ab4[:, 2])])

p = p.rotation(0, 1, np.pi/2)
print(p.bounding_box)
assert(p == p2)
Copy link
Member

Choose a reason for hiding this comment

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

I, too, used to enclose the asserted expression within parentheses. The parentheses can be omitted, because in Python assert is a statement, not a function (in contrast for example to print, which was a statement in Python 2 and became a function in Python 3).

Copy link
Member

Choose a reason for hiding this comment

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

just to be clear in terms of reviewing this PR, @johnyf are you requesting that the parens be deleted, or are you only making a comment about style that is OK either way (with or without parens)?

Copy link
Member

Choose a reason for hiding this comment

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

The main reason I made this remark is because it took me some time before I noticed that assert is a statement and doesn't need parentheses. I am not requesting this change for merging, though it would be nice to apply it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not going to change this because all of the other tests use assert as a function. They can all be changed together in some other pull request.

Copy link
Member

Choose a reason for hiding this comment

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

Those parentheses are mine. As I mentioned, at some point I noticed that assert is a statement. The parentheses can be removed after merging.

assert(not p == p3)
assert(not p == p4)
assert(not p == p1)
assert_allclose(p.chebXc, [-0.5, 0.5])

p = p.rotation(0, 1, np.pi/2)
assert(p == p3)
assert_allclose(p.chebXc, [-0.5, -0.5])

p = p.rotation(0, 1, np.pi/2)
assert(p == p4)
assert_allclose(p.chebXc, [0.5, -0.5])

p = p.rotation(0, 1, np.pi/2)
assert(p == p1)
assert_allclose(p.chebXc, [0.5, 0.5])


def polytope_rotation_test():
p = pc.Polytope(A, b)
p1 = pc.Polytope(A, b)
p2 = pc.Polytope(Ab2[:, 0:2], Ab2[:, 2])
p3 = pc.Polytope(Ab3[:, 0:2], Ab3[:, 2])
p4 = pc.Polytope(Ab4[:, 0:2], Ab4[:, 2])

p = p.rotation(0, 1, np.pi/2)
print(p.bounding_box)
assert(p == p2)
assert(not p == p3)
assert(not p == p4)
assert(not p == p1)
assert_allclose(p.chebXc, [-0.5, 0.5])

p = p.rotation(0, 1, np.pi/2)
assert(p == p3)
assert_allclose(p.chebXc, [-0.5, -0.5])

p = p.rotation(0, 1, np.pi/2)
assert(p == p4)
assert_allclose(p.chebXc, [0.5, -0.5])

p = p.rotation(0, 1, np.pi/2)
assert(p == p1)
assert_allclose(p.chebXc, [0.5, 0.5])


def region_translation_test():
p = pc.Region([pc.Polytope(A, b)])
p1 = pc.Region([pc.Polytope(A, b)])
p2 = pc.Region([pc.Polytope(Ab2[:, 0:2], Ab2[:, 2])])

p = p.translation([-1, 0])
assert(p == p2)
assert(not p == p1)
p = p.translation([1, 0])
assert(p == p1)


def polytope_translation_test():
p = pc.Polytope(A, b)
p1 = pc.Polytope(A, b)
p2 = pc.Polytope(Ab2[:, 0:2], Ab2[:, 2])

p = p.translation([-1, 0])
assert(p == p2)
assert(not p == p1)
p = p.translation([1, 0])
assert(p == p1)

if __name__ == '__main__':
comparison_test()