WIP: Vectorize TemporalTransformManager #301

JonasHablitzel · 2024-10-17T21:29:20Z

Hello,

i want to add a feature to the libary to support multiple timestamps for the get_transform_at_time method for the TemporalTransformManager class.

For this i already added a class NumpyTimeseriesTransformTrajectory which is able to take a array as input for the as_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

manager = TemporalTransformManager()
...
timestamps = np.asarray([10,20,30])
manager .get_transform_at_time("a","b",timestamps )

Option2

add a new method for batch/trajectorie operations.

manager = TemporalTransformManager()
...
timestamps = np.asarray([10,20,30])
manager .get_transform_at_time_batch("a","b",timestamps )

I got Option1 kinda to work with the following addition.

class TemporalTransformManagerVectorized(TemporalTransformManager):
    def _path_transform(self, path):
        """Convert sequence of node names to rigid transformation."""
        A2B = np.eye(4)
        for from_f, to_f in zip(path[:-1], path[1:]):
            A2B = concat_dynamic(
                A2B,
                self.get_transform(from_f, to_f),
            )
        return A2B

AlexanderFabisch · 2024-10-18T07:28:22Z

Hi @JonasHablitzel ,

in my opinion it is not necessary to introduce another subclass of TimeVaryingTransform that behaves differently from the others when you actually want to modify the interface of the TemporalTransformManager. I would be in favor of modifying the existing NumpyTimeseriesTransform that already stores a trajectory. It should be possible to vectorize the current implementation.

I would be a little bit more in favor of get_transform_at_time("a", "b", timestamps) vs. get_transform_at_time_batch("a", "b", timestamps) as it is a bit more in line with, e.g., the numpy ufuncs.

I would like to involve @kopytjuk who implemented the TemporalTransformManager (if you are available).

kopytjuk · 2024-10-24T17:52:25Z

pytransform3d/transform_manager/_temporal_transform_manager.py

@@ -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):


We have to adapt typings here as well since we allow both floats and arrays

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?

kopytjuk

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 ...

kopytjuk · 2024-10-24T18:02:45Z

pytransform3d/test/test_transform_manager.py

@@ -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():


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.

kopytjuk · 2024-10-24T18:04:21Z

pytransform3d/test/test_transform_manager.py

+    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]


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 ...

Starting with the nitpicking here: please also check for PEP8 with either pep8 or flake8.

kopytjuk · 2024-10-24T18:17:01Z

pytransform3d/trajectories.py

+def concat_dynamic(A2B, B2C):
+    """Concatenate multiple transformations A2B with transformation B2C.


To clarify what "dynamic" means:

Suggested change

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.

kopytjuk · 2024-10-24T18:24:43Z

pytransform3d/trajectories.py

+
+
+    non_zero_mask = ~zero_mask
+    res[non_zero_mask, :3] = a[non_zero_mask, :3] / norm[non_zero_mask, None]  


Since the norm is one-dimensional after you applied np.linalg.norm on a 2D (Nx4) matrix:

Suggested change

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]

kopytjuk · 2024-10-24T18:35:41Z

pytransform3d/trajectories.py

+
+    return qs, s_axis, hs, thetas
+
+def dual_quaternions_from_screw_parameters(qs, s_axis, hs, thetas):


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?

kopytjuk · 2024-10-24T18:36:01Z

pytransform3d/trajectories.py

+    return result
+
+
+def screw_parameters_from_dual_quaternions(dqs):


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?

kopytjuk · 2024-10-24T19:06:45Z

The more I look deeper into this contribution, the more I appreciate the work.

I think a good way forward would be to:

Adapt the TemporalTransformManager to have an array of timestamps as self._current_time (instead of a scalar timestamp as it is now).
Adapt the interface of the TimeVaryingTransform.as_matrix(query_time). It should accept time arrays of size N as well and return Nx4x4 tensor. @JonasHablitzel already kind of addressed that in the NumpyTimeseriesTransform.
Having that, we can use the proposed adaptation of _path_transform with the concat_dynamic directly in the TransformGraphBase.

What do you think @AlexanderFabisch on the plan?

AlexanderFabisch · 2024-10-25T12:51:03Z

What do you think @AlexanderFabisch on the plan?

Sounds good to me. And thanks for reviewing the PR!

added a trajetorie mode for Temporal

c3a1dd3

added sclerp for arrays/trajectories

80408eb

kopytjuk reviewed Oct 24, 2024

View reviewed changes

AlexanderFabisch changed the title ~~added a trajetorie mode for Temporal~~ Vectorize TemporalTransformManager Oct 25, 2024

AlexanderFabisch changed the title ~~Vectorize TemporalTransformManager~~ WIP: Vectorize TemporalTransformManager Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Vectorize TemporalTransformManager #301

WIP: Vectorize TemporalTransformManager #301

JonasHablitzel commented Oct 17, 2024

AlexanderFabisch commented Oct 18, 2024

kopytjuk Oct 24, 2024

AlexanderFabisch Oct 25, 2024

kopytjuk left a comment •

edited

Loading

kopytjuk Oct 24, 2024 •

edited

Loading

kopytjuk Oct 24, 2024

AlexanderFabisch Oct 25, 2024

kopytjuk Oct 24, 2024

kopytjuk Oct 24, 2024 •

edited

Loading

kopytjuk Oct 24, 2024

kopytjuk Oct 24, 2024

kopytjuk commented Oct 24, 2024 •

edited

Loading

AlexanderFabisch commented Oct 25, 2024 •

edited

Loading

		@@ -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():

		def concat_dynamic(A2B, B2C):
		"""Concatenate multiple transformations A2B with transformation B2C.



		non_zero_mask = ~zero_mask
		res[non_zero_mask, :3] = a[non_zero_mask, :3] / norm[non_zero_mask, None]


		return qs, s_axis, hs, thetas

		def dual_quaternions_from_screw_parameters(qs, s_axis, hs, thetas):

		return result


		def screw_parameters_from_dual_quaternions(dqs):

WIP: Vectorize TemporalTransformManager #301

Are you sure you want to change the base?

WIP: Vectorize TemporalTransformManager #301

Conversation

JonasHablitzel commented Oct 17, 2024

Option1

Option2

AlexanderFabisch commented Oct 18, 2024

kopytjuk Oct 24, 2024

Choose a reason for hiding this comment

AlexanderFabisch Oct 25, 2024

Choose a reason for hiding this comment

kopytjuk left a comment • edited Loading

Choose a reason for hiding this comment

kopytjuk Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

kopytjuk Oct 24, 2024

Choose a reason for hiding this comment

AlexanderFabisch Oct 25, 2024

Choose a reason for hiding this comment

kopytjuk Oct 24, 2024

Choose a reason for hiding this comment

kopytjuk Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

kopytjuk Oct 24, 2024

Choose a reason for hiding this comment

kopytjuk Oct 24, 2024

Choose a reason for hiding this comment

kopytjuk commented Oct 24, 2024 • edited Loading

AlexanderFabisch commented Oct 25, 2024 • edited Loading

kopytjuk left a comment •

edited

Loading

kopytjuk Oct 24, 2024 •

edited

Loading

kopytjuk Oct 24, 2024 •

edited

Loading

kopytjuk commented Oct 24, 2024 •

edited

Loading

AlexanderFabisch commented Oct 25, 2024 •

edited

Loading