-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fixed fall through error in binary operation #117
Fixed fall through error in binary operation #117
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #117 +/- ##
==========================================
+ Coverage 77.41% 77.76% +0.35%
==========================================
Files 24 24
Lines 5136 5137 +1
==========================================
+ Hits 3976 3995 +19
+ Misses 1160 1142 -18 ☔ View full report in Codecov by Sentry. |
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. thanks for identifying the issue.
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.
Maybe take my suggested change?
spatialmath/baseposematrix.py
Outdated
@@ -1657,7 +1657,7 @@ def _op2(left, right: Self, op: Callable): # pylint: disable=no-self-argument | |||
========= ========== ==== ================================ | |||
|
|||
""" | |||
if isinstance(right, left.__class__): | |||
if right.__class__ is left.__class__: |
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.
I'm pretty sure isinstance
is better?
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.
i made this change because, given the inheritance structure,
- SO3 is not an instance of SE3, but
- SE3 is an instance of SO3.
This PR seems to intend to disallow both se3.angdist(so3) and so3.angdist(se3).
Please feel free to make any change as necessary @bokorn-bdaii
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.
We could allow angdist between those too, but I am not sure if all binary operations will work here, translation based operations being computed between SO3 and SE3. That being said, it's currently only used by angdist. Depends if you want to allow cross type binary operations and put the validity checks up to future implementers.
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.
This is an option if we want to be able to look at angular distances between SO3 and SE3, or visa versa
if right.__class__ is left.__class__: | |
if isinstance(right, left.__class__) or isinstance(left, right.__class__): |
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.
suggestion applied in b848e7b
Fixes fall through error in the binary operator for BasePoseMatrix described in #116:
https://github.com/bdaiinstitute/spatialmath-python/blob/1b89c49395a21b5241e2f0a233e69394f3bc27b1/spatialmath/baseposematrix.py#L1632C7-L1632C7
If you want to compare the rotation of an SE3 to an SO3, it returns
None
as opposed to throwing an error or returning a valid value.Modified
BasePoseMatrix._op2
to throw an error if the types don't match.All tests still pass.