-
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
Fit b spline se3 #129
base: master
Are you sure you want to change the base?
Fit b spline se3 #129
Conversation
Converted to draft because this uses scipy functionality not available in the version associated with the dependency list for 3.7 python. |
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.
Not sure why this is needed. You would change the code pattern from:
x = SO3(R1)
x = SO3(R2)
to
x = SO3(R1)
x.R = R2
It's no shorter, clearer, or cheaper. The second line has a hidden SO3
constructor and like the first option leaves an object to be garbage collected. It also makes the check for matrix validity happen, which is in principle a good thing, but is expensive. If an SMTB module knows that the matrix belongs to SO(3) then that check can be circumvented by
x = SO3(R1, check=False)
x = SO3(R2, check=False)
This is a "feature" user's can also use but at their own peril. In your interpolator you know that the matrix you compute is in SO(3).
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.
Given the usefulness of the functionality, and their independence from the rest of the code, nothing in SMTB depends on these, the risk is low, and should be added. But see comments below.
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.
These are a really useful addition. The control poses
def __init__(
self,
control_poses: List[SE3],
) -> None:
are passed as a list of SE3
s not as a multi-element SE3
. What's the thinking there?
There are multiple interpolator classes, each algorithm specific. Would it be better to have one interpolator class with a passed argument to say which one is used. This is kind of the approach that SciPy takes with optimisers and integrators that have a method
parameter. Simplest approach would be a wrapper class that instantiates one of the specific classes, all of them with a common parent class.
@myeatman-bdai a few more thoughts on this PR:
|
Add functionality to fit a cubic-SE3-B-spline to data.