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
11 changes: 5 additions & 6 deletions polytope/polytope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ def rotation(self, u, v, theta=None):
(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
(2) u and v are two vectors, angle of rotation is TWICE the angle
Copy link
Member

Choose a reason for hiding this comment

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

Can you provide citations? However, this might not be important because it is not implemented yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this website explains, if you use full angles instead of half angles, there is ambiguity when rotations are pi.

between them from u to v. NOTE: This method is not implemented at this
time.

Expand Down Expand Up @@ -518,10 +518,9 @@ def _rotate(polyreg, u=None, v=None, theta=None, R=None):
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.
(2) Provide two vectors, the two vectors define the plane of rotation
and angle of rotation is TWICE the angle from the first vector, u, to
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.
Expand Down Expand Up @@ -791,7 +790,7 @@ def rotation(self, u, v, theta=None):
(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
(2) u and v are two vectors, angle of rotation is TWICE the angle
between them from u to v. NOTE: This method is not implemented at this
time.

Expand Down