-
Notifications
You must be signed in to change notification settings - Fork 68
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
WIP: Vectorize TemporalTransformManager #301
base: develop
Are you sure you want to change the base?
WIP: Vectorize TemporalTransformManager #301
Conversation
Hi @JonasHablitzel , in my opinion it is not necessary to introduce another subclass of I would be a little bit more in favor of I would like to involve @kopytjuk who implemented the |
@@ -104,9 +103,11 @@ def check_transforms(self): | |||
return self | |||
|
|||
def _interpolate_pq_using_sclerp(self, query_time): | |||
if isinstance(query_time,float) or isinstance(query_time,float): |
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 have to adapt typings here as well since we allow both floats and arrays
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.
Also this line does not really make sense, right? Checks the same thing twice. It is also not very general. What if an int is passed? Or float32?
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.
First of all @JonasHablitzel, thank you for your contribution!
I believe making the transform lookup vectorized is a great addition to the code, for many real world projects looping over the timesteps and calling the existing get_transform_at_time()
is indeed quite slow ...
@@ -504,6 +504,47 @@ def test_numpy_timeseries_transform(): | |||
assert origin_of_A_in_B_y == pytest.approx(-1.28, abs=1e-2) | |||
|
|||
|
|||
def test_numpy_timeseries_transform_multiple_timestamps_query(): |
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.
Since pytest
is used, consider extending the existing test function using pytest.mark.parametrize
:
@pytest.mark.paramertrize("query_times", [4.9, np.array([4.9, 4.9])]
def test_numpy_timeseries_transform(query_times):
# ...
tm.get_transform_at_time("A", "B", query_times)
Having done that, we can have a single test, testing both the scalar and array inputs.
A2W_at_start_2 = tm.get_transform_at_time("A", "W", query_time) | ||
assert_array_almost_equal(A2W_at_start, A2W_at_start_2, decimal=2) | ||
|
||
query_times = [4.9,4.9] # [s] |
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 PR adds quite a lot vectorized mathematical operations, which IMHO need a little more unittests than testing with an array of two equal values ...
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.
Starting with the nitpicking here: please also check for PEP8 with either pep8 or flake8.
def concat_dynamic(A2B, B2C): | ||
"""Concatenate multiple transformations A2B with transformation B2C. |
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.
To clarify what "dynamic" means:
def concat_dynamic(A2B, B2C): | |
"""Concatenate multiple transformations A2B with transformation B2C. | |
def concat_dynamic(A2B, B2C): | |
"""Concatenate multiple transformations A2B with transformation B2C. | |
This function accepts different numbers or shapes of the input transformations, e.g. 4x4 or Nx4x4 shape. |
|
||
|
||
non_zero_mask = ~zero_mask | ||
res[non_zero_mask, :3] = a[non_zero_mask, :3] / norm[non_zero_mask, None] |
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.
Since the norm
is one-dimensional after you applied np.linalg.norm
on a 2D (Nx4) matrix:
res[non_zero_mask, :3] = a[non_zero_mask, :3] / norm[non_zero_mask, None] | |
res[non_zero_mask, :3] = a[non_zero_mask, :3] / norm[non_zero_mask] |
|
||
return qs, s_axis, hs, thetas | ||
|
||
def dual_quaternions_from_screw_parameters(qs, s_axis, hs, thetas): |
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.
Since this function is non-trivial when it comes to mathematical operations, would you please add at least a unittest so that a next developer wants to optimize/change/accelerate could check her work?
return result | ||
|
||
|
||
def screw_parameters_from_dual_quaternions(dqs): |
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.
Since this function is non-trivial when it comes to mathematical operations, would you please add at least a unittest so that a next developer wants to optimize/change/accelerate could check her work?
The more I look deeper into this contribution, the more I appreciate the work. I think a good way forward would be to:
What do you think @AlexanderFabisch on the plan? |
Sounds good to me. And thanks for reviewing the PR! |
Hello,
i want to add a feature to the libary to support multiple timestamps for the
get_transform_at_time
method for theTemporalTransformManager
class.For this i already added a class
NumpyTimeseriesTransformTrajectory
which is able to take a array as input for theas_matrix
method.Regarding the public facing API i wanted to ask you what you would prefer more. For my point of view there are two options:
Option1
Keep interface the same
Option2
add a new method for batch/trajectorie operations.
I got Option1 kinda to work with the following addition.