-
Notifications
You must be signed in to change notification settings - Fork 129
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
Add untransform #552
Add untransform #552
Conversation
return [jacobian_g, jacobian_p], ret | ||
|
||
|
||
class UntransformFrom(lie_group.BinaryOperator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UntransformFrom -> Untransform ?
return [jacobian_g, jacobian_p], ret | ||
|
||
|
||
class UntransformFrom(lie_group.BinaryOperator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UntransformFrom -> Untransform ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I am guessing CI fails for same reason the other transform PR is failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
+1 on replacing UntranformFrom -> Unstranform.
@@ -261,34 +261,8 @@ def transform_to( | |||
jacobians: Optional[List[torch.Tensor]] = None, | |||
) -> Point3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhmukadam Do we want to make a similar (future) change on theseus
? If so, I can make this in another PR so that we add a deprecation message and an alias for the new names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes in general it would ideal to make the naming consistent with torchlie so it isn't confusing. Sounds good on adding the deprecation warning.
c87c4be
to
c52deba
Compare
2b31997
to
567e51b
Compare
08083a3
to
85b0cb7
Compare
567e51b
to
61f5382
Compare
85b0cb7
to
2cd5aee
Compare
61f5382
to
46a5b66
Compare
Motivation and Context
How Has This Been Tested
Types of changes
Checklist