Skip to content

Fixed fall through error in binary operation #117

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

Merged
merged 5 commits into from
Jan 10, 2024
Merged

Fixed fall through error in binary operation #117

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e7b6eda
Fixed fall through error in binary operation
bokorn-bdaii Jan 4, 2024
5c432b3
add unit test coverage
jcao-bdai Jan 5, 2024
336fbc8
update test + type check
jcao-bdai Jan 5, 2024
b848e7b
updates based on review comments
jcao-bdai Jan 10, 2024
c234e6c
one more test
jcao-bdai Jan 10, 2024
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
6 changes: 5 additions & 1 deletion spatialmath/baseposematrix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__:
Copy link
Collaborator

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?

Copy link
Collaborator

@jcao-bdai jcao-bdai Jan 5, 2024 •
edited
Loading

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Suggested change
if right.__class__ is left.__class__:
if isinstance(right, left.__class__) or isinstance(left, right.__class__):

Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion applied in b848e7b

# class by class
if len(left) == 1:
if len(right) == 1:
Expand All @@ -1683,6 +1683,10 @@ def _op2(left, right: Self, op: Callable): # pylint: disable=no-self-argument
return op(left.A, right)
else:
return [op(x, right) for x in left.A]
else:
raise TypeError(
f"Invalid type ({right.__class__}) for binary operation with {left.__class__}"
)


if __name__ == "__main__":
Expand Down
36 changes: 36 additions & 0 deletions tests/test_pose3d.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1260,6 +1260,42 @@ def test_arith_vect(self):
array_compare(a[1], ry - 1)
array_compare(a[2], rz - 1)

def test_angle(self):
# angle between SO3's
r1 = SO3.Rx(0.1)
r2 = SO3.Rx(0.2)
for metric in range(6):
self.assertAlmostEqual(r1.angdist(other=r1, metric=metric), 0.0)
self.assertGreater(r1.angdist(other=r2, metric=metric), 0.0)
self.assertAlmostEqual(
r1.angdist(other=r2, metric=metric), r2.angdist(other=r1, metric=metric)
)
# angle between SE3's
p1a, p1b = SE3.Rx(0.1), SE3.Rx(0.1, t=(1, 2, 3))
p2a, p2b = SE3.Rx(0.2), SE3.Rx(0.2, t=(3, 2, 1))
for metric in range(6):
self.assertAlmostEqual(p1a.angdist(other=p1a, metric=metric), 0.0)
self.assertGreater(p1a.angdist(other=p2a, metric=metric), 0.0)
self.assertAlmostEqual(p1a.angdist(other=p1b, metric=metric), 0.0)
self.assertAlmostEqual(
p1a.angdist(other=p2a, metric=metric),
p2a.angdist(other=p1a, metric=metric),
)
self.assertAlmostEqual(
p1a.angdist(other=p2a, metric=metric),
p1a.angdist(other=p2b, metric=metric),
)
# not allowing angdist between mismatched types
with self.assertRaises(TypeError):
_ = r1.angdist(p1a)

# in general, the _op2 interface allows same type only
with self.assertRaises(TypeError):
_ = r1._op2(right=p1a, op=r1.angdist)

with self.assertRaises(TypeError):
_ = p1a._op2(right=r1, op=p1a.angdist)

def test_functions(self):
# inv
# .T
Expand Down