RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.36k stars 1.27k forks source link

More specific "Transform" class name. #9072

Closed mitiguy closed 6 years ago

mitiguy commented 6 years ago

This issue relates to the name "Transform" and whether it should be modified/changed to make it more unique and disambiguate from Eigen::Transform.

Although I created the class/name "Transform", I have my doubts that this was the best name.

Currently, there are many uses of the word "Transform" in our code base.

  1. There are many (more than 50) uses of "Eigen::Transform".
  2. There is also "using Eigen::Transform"
  3. There are more than 600 case-sensitive matches to Transform in our code (this does not include our documentation or 3rd-party code).
  4. There are many more case-insensitive matches to transform in method names, variable names, etc.

Should it be changed ?

  1. Yes - change it to "RigidTransform".
  2. Yes - change it to "XTransform".
  3. Yes - change it to "__". Please make a suggestion.
  4. No - leave it alone.

Additional context: The Transform class is in drake/math. It contains a RotationMatrix (guaranteed to be right-handed and orthonormal) and a position vector. The RotationMatrix class and its associated RollPitchYaw classes have many methods that are meant to be more "bullet-proof" from a generic 3x3 matrix or 3x1 with 3 angles.

naveenoid commented 6 years ago

@thduynguyen : A topic for you!

SeanCurtis-TRI commented 6 years ago

It seems to me that this is a B&W vs color TV issue. When TVs were first introduced they were just TVs. Then color was introduced and you had to distinguish the two. Now, only the B&W label persists to indicate the anachronisms and everyone takes for granted that "TV" means a color TV.

In this case, it seems you're worried about ambiguity between the drake::Transform and the Eigen::Transform. So, by introducing some prefix (e.g., Rigid, X, whatever) you get a clear distinction between the old TVs and the new TVs. However, once the prefix has served its purpose in the transition, it won't organically fade -- code is far more rigid than natural language. We'll be stuck with that prefix for years to come. So, I'd ask the question, if the drake::Transform were the only transform, would you still feel that it has the wrong name? (For my money, the answer would be "no".)

Also, @mitiguy could you clearly state the problem that the proposed naming convention is meant to solve? I've inferred one above, but I recognize my inference may be in error.

avalenzu commented 6 years ago

I vote for RigidTransform. I think this would be good, not because it distinguishes it from Eigen::Transform, but because if fully specifies what it is. Just in the realm of geometry in SE(3), affine transforms, shear transforms, and scaling transforms have just as much right to be called "transforms". Given that this class lives in the drake::math namespace, other concepts like Laplace transforms and Fourier transforms can also compete for this name. RigidTransform says what we mean.

SeanCurtis-TRI commented 6 years ago

@avalenzu I find that a compelling argument.

SeanCurtis-TRI commented 6 years ago

A follow up on @avalenzu's excellent point.

  1. How many of those transform types will actually appear in Drake? I would argue that we can rely on context to do some culling. So, consideration should only include those transforms that Drake would deal with (for example, we're not going to include glide transforms).
  2. How many of the remaining transforms are nouns? For example, when I think of a Fourier transform, I think of it in terms of a function. I've never seen a FourierTransform class. Our (as proposed) RigidTransform is a noun (and therefore a class) because we use it to represent a pose. Is there a noun-version of the other transforms such that they would appear as classes whose names need to be disambiguated?
rcory commented 6 years ago

Agreed with @avalenzu. The most compelling reason for me is that the name tells you exactly what it is. I contrast that to names like "Isometry", where even now I see folks pulling up the definition to understand exactly what it means! And even Eigen isn't consistent about enforcing its properties.

mitiguy commented 6 years ago

From the discussions here and f2f with Rick, HongKai, Sean, ..., it seems thus far that RigidTransform is preferable. I will e-mail to our group so others can provide input. The current plan is to make the change next week.

ryanelandt commented 6 years ago

I am a fan of Transform3D. I think that RigidTransform is confusing because this include reflections.

ryanelandt commented 6 years ago

According to the top 3 results on Google, rigid transform and isometry are synonyms

sherm1 commented 6 years ago

I was in favor of just Transform, but am persuaded by @avalenzu's argument in favor of RigidTransform. Even Eigen's obscure attempt to capture that with "Isometry3" still could include a reflection. I don't see how any rigid rotation or translation could produce a reflection so I think that term captures exactly the meaning we want to have.

sherm1 commented 6 years ago

As @ryanelandt pointed out, apparently mathematicians think a "rigid rotation" includes a reflection, and that what we have is a "proper rigid rotation". I would be willing to assume Drake users won't expect a RigidRotation to include a reflection, but Ryan points out that as a rigid body dynamicist, I might be biased! Is that term ambiguous?

thduynguyen commented 6 years ago

My personal preference is SE3. That might make people cry, I know, but that is what it is. Pose3 is another choice, adopted in gtsam. No confusion.

amcastro-tri commented 6 years ago

My two cents. I personally hate "RigidTransfrom", nothing personal, it just sounds awful to me. I actually agree with @SeanCurtis-TRI's comments about B&W vs color TV's. I think Transform conveys the right meaning of course, within the right context and right documentation (I have no doubt we won't confuse it with a LaplaceTransform).

However, somehow it'd seem people now hate the term "Transform"? interesting. My next in the list option would be Pose . IMO it conveys very clearly an element in SE(3) and we do say "pose" all the time in our docs already. For instance, our docs have lots of instances where we say something like "... where the pose X_WB of body B in the world frame W is given by ... blah..." So, what about using "Pose"?

Otherwise, I'd just prefer keeping "Transform".

edrumwri commented 6 years ago

I might be biased too, since I called my rigid transform class Pose3 (in Ravelin). I'd vote for Pose (if it applies to 2D and 3D poses) or Pose3 (if it only applies to 3D poses).

avalenzu commented 6 years ago

@amcastro-tri, I don't "hate" the term "Transform," I just think it's a broader term than what we want here. I agree that LaplaceTransform is a stretch, but people coming from a graphics or vision background would be likely to expect a class called Transform to at least include shear and scaling.

Pose sounds good to me. The only downside is that we always talk about the "pose X_WB of body B in the world frame W" as the entity that transforms points relative to P to points relative to W. This connotation is lost if the class is named Pose.

SeanCurtis-TRI commented 6 years ago

On behalf of people coming from graphics, I wouldn't expect shear transforms. :)

It's worth noting that the whole concept of this class is muddied by the fact that it's both a noun and a verb. It's the pose of something, but serves as an operator that take a position vector measured and expressed one way and re-measures and re-expresss it another way (and yes, that contorted vagueness was intentional). The noun-verb ambiguity is present in every name. Transform (for example) is both a noun and verb. Pose is also a noun and a verb. A property and an operator.

Ultimately, when we say "transform points" relative to B to points relative to W, @mitiguy might argue what we're really doing is re-measuring and re-expressing them. (Apologies to @mitiguy if I'm putting mangled words into his mouth.)

sherm1 commented 6 years ago

We already use "Pose" in use in various places -- in practice poses (the noun) are represented compactly with either a quaternion or roll-pitch-yaw angles for orientation. The "Transform" class that we're trying to name will inherently use a drake::math::RotationMatrix object (a 3x3 matrix) for orientation, so is somewhat slanted towards the verb (operator) form. Does that change anything?

SeanCurtis-TRI commented 6 years ago

I would hope that the fact that it contains a RotationMatrix is an implementation detail. Knowing its contents is only relevant to the user if they need to store a bunch and are worried about memory, or they'll do a whole bunch of independent transforms and don't want to keep doing a qq* operation. But whether orientation is implemented as a quarternion, euler angles, or rotation matrix doesn't really change the semantics of the Transform class -- it's a pose and a linear operator.

Or am I missing a subtlety?

sherm1 commented 6 years ago

Well, in a perfect world a Transform would be a purely abstract concept. In fact, I think that's what Eigen was trying to achieve with its rather contorted collection of transform sub-objects. But in practice rotations and transforms occur in the tightest of inner loops so often have to be dealt with concretely. Also, a rotation matrix contains frame axes in its 3x3 representation while a quaternion (e.g.) does not, hence a peek behind the curtain at the matrix is often useful -- consider the operation you encountered the other day where abs(R.matrix()) is used to find point-to-box distance (or whatever that was). Then there is the matter of the extra storage used by the matrix form, not good for transmitting over a bandwidth-limited line. So I think it is worthwhile IRL to have a nice non-abstract representation for a 3x3 Rotation separate from a Quaternion, and Transforms and Poses built from them.

thduynguyen commented 6 years ago

Class name should be a noun. The action transform should be an operator/method of the class. SE3 is a group, which has an operator *. Regarding the implementation details, algorithms should not concern about the underlying implementation of the class, be it a RotationMatrix or a Quaternion. We do need two different classes of rotation, however, Pose3 (as a semi-product of SO3 and R3) can template on SO3 type.

mitiguy commented 6 years ago

Our preferences are not uniform. The highest preference is RigidTransform. RigidTransform is consistent with general use, e.g., https://en.wikipedia.org/wiki/Rigid_transformation

For consolation to those who dislike RigidTransform, I did a google search on "RigidTransform". There are other folks who use that same name (as it turns out - including Mathworks).

Link to soon-to-be submitted PR: https://github.com/RobotLocomotion/drake/pull/9143