[WIP] Abstract interface for TransformManager to deal with time

[kopytjuk]

Currently, the TransformManager only works with static transformations represented as homogenous numpy matrices.

This PR introduces an abstract interface as_homogenous_matrix(time: optional) to work with dynamic transformations:

@dataclass
class TransformPandas(TransformBase):

    df: pd.DataFrame  # df with time-index

    def as_homogenous_matrix(self, time: Optional[float] = None) -> np.ndarray:
        if time:
            record_from_a_timestamp = self.df.loc[time]
        else:
            record_from_a_timestamp = self.df.iloc[0]
        return as_matrix(record_from_a_timestamp)  # or interpolation

a2b_dynamic: TransformPandas
b2c_dynamic: TransformPandas

tm = TransformManager()
tm.add_transform("A", "B", a2b_dynamic)
tm.add_transform("B", "C", b2c_dynamic)

tm.get_transform("A", "C")  # returns the first available
tm.get_transform("A", "C", time=0.1101)  # returns the corresponding

Now the User can implement a TransformBase child class, and define on herself how/whether to interpolate. This approach follows the Strategy pattern.

[AlexanderFabisch]

Hi @kopytjuk ,

first of all, thanks a lot for the very good contribution.

I have a few concerns though. Those are mainly about backwards-compatibility and Python version compatibility:

1. There should be absolutely no breaking changes. Keep in mind that there can also be subclasses of the TransformManager, for example, the UrdfTransformManager. Also the visualization and plotting tools access the internals of the TransformManager.
2. You used quite a lot of idioms that might be available only in more recent Python versions, in particular
   • type hints - we use separate .pyi files for the type hints
   • typing.Optional - Union[float, None] should also work
   • dataclass - not sure when it was introduced

I think a good approach would be to keep the old TransformManager interface as it is, introduce a new TemporalTransformManager (with a better name), and move common code to a base class.

According to PYPI stats there is still a small amount of downloads for Python 2.7 and 3.6. So these are the extreme cases.

[kopytjuk]

Hi @AlexanderFabisch,

thanks for your detailed response!

Python 2.7 & 3.6

You used quite a lot of idioms that might be available only in more recent Python versions

I completely agree, I will remove the type hints and dataclasses (which are avail. >=3.7).

Breaking changes

There should be absolutely no breaking changes.

I understand your concerns regarding backward compatibility and I like the fact that you take care for your users and interfaces you are exposing.

I will try to convince you a little bit:

First of all, the PR does not change any of internal logic nor any of external interfaces. The changes are always additive. In the PR, user facing methods such as add_transform, remove_transform etc. backward compatible. Note, that in my implementation a passed np.array to add_transform is casted to TransformNumpy internally. The user can keep her workflow and existing code as it is.

The time argument (like in get_transform) is always optional. So a long-term user of the library should not see the difference at all. The available unittests (also those for the UrdfTransformManager) pass.

As long as users do not ("illegally" wrt to the docs - the attribute is undocumented) depend their implementations on self.transforms (which we could expose as a public function, too), the approach here should be fine.

The large benefit I can see is that we can give the user freedom about her/his data reprepresentation, as long it supports the as_homogenous_matrix(time: float | None) -> 4x4 matrix interface. Ofc, we can rename this interface.

I think the most major change I proposed in this PR is this:

self.transforms: Dict[Hashable, TransformBase] = {}

# the interface `as_homogenous_matrix` used in the methods (not exposed to the user)
# non-dynamic case
self.transforms[(from_frame, to_frame)].as_homogenous_matrix()
# dynamic case
self.transforms[(from_frame, to_frame)].as_homogenous_matrix(time)

If you could elaborate what in particular worries you the most, maybe we can iterate on that. E.g. we can introduce tests which test common user behavior?

Base class

I think a good approach would be to keep the old TransformManager interface as it is, introduce a new TemporalTransformManager (with a better name), and move common code to a base class.

I was trying to come up with a good approach, the only thing which comes to my mind regarding common code is everything (graph, shortest-path, inversion) except plotting (which may be different for dynamic trasfos).

Is it what you mean? Feel free to elaborate on what you think :)

[AlexanderFabisch]

Hi, just to let you know: I didn't have the chance to read and respond yet, but I will do that during this week.

[AlexanderFabisch]

If you could elaborate what in particular worries you the most, maybe we can iterate on that. E.g. we can introduce tests which test common user behavior?

In particular, this code will break: https://github.com/dfki-ric/hand_embodiment/blob/d506e30a31d494aef751e17daf005bb5f786f49e/hand_embodiment/kinematics.py#L12

Unfortunately, the dictionary transforms is part of the inheritance interface.

I think a good approach would be to keep the old TransformManager interface as it is, introduce a new TemporalTransformManager (with a better name), and move common code to a base class.

I was trying to come up with a good approach, the only thing which comes to my mind regarding common code is everything (graph, shortest-path, inversion) except plotting (which may be different for dynamic trasfos).

Is it what you mean? Feel free to elaborate on what you think :)

Yes, but I think that code should go to a base class. That requires some refactoring.

[AlexanderFabisch]

@kopytjuk Here is a proposal for the refactoring of the TransformManager: #256

Please check if it would be possible to implement your changes by inheriting from the new base class.

[kopytjuk]

@AlexanderFabisch thanks for providing guidance with the TransformGraphBase class!

I tried to implement a solution on top of your branch in this PR https://github.com/dfki-ric/pytransform3d/pull/257

I could not manage to come with a satisfying solution and thought a long time what is the problem. In my opinion, it is in the get_transform(self, from, to) definition which is not flexible with timeseries when going over multiple edges of the graph. I try to elaborate on a simple 3 node graph (R->A, R->B):

• get_transform("R", "A") easy to implement
• get_transform("A", "R") harder to implement, but still doable, since we can invert each timestamp
• get_transform("A", "B") hard to implement, since the temporal interpolation (at concat) kicks in ...

Maybe the get_transform() has to disappear from the TransformGraphBase class?

[kopytjuk]

Another thought -

what about exposing transforms as a property for users using it?

# ...
self._transforms: Dict[Tuple[Hashable, Hashable], Transform]

@property
def transforms(self):
     return {k: v.as_homogenous_matrix(t=None) for k in self._transforms.items}

And in the affected code we would use add_transform instead of using self.transforms ...

[kopytjuk]

Third thought - we implement an independent TransformManagerTemporal with copy & paste code in a new python file/module and over time, step-by-step summarize functionality to a base class for easier maintance. We would prevent a big bang approach.

Currently it is very hard to agree on a common interface when one personen thinking of a base class, and another person trying to fit in ...

[AlexanderFabisch]

I could not manage to come with a satisfying solution and thought a long time what is the problem. In my opinion, it is in the get_transform(self, from, to) definition which is not flexible with timeseries when going over multiple edges of the graph. I try to elaborate on a simple 3 node graph (R->A, R->B):

I was thinking that you could change the interface in the base class to something like

def get_transform(self, from, to, *kwargs): ...

where *args in your subclass would be just time. But we can also remove get_transform from the base class. What do you think?

what about exposing transforms as a property for users using it?

That's actually a very good idea. I will add that to my PR.

Third thought - we implement an independent TransformManagerTemporal with copy & paste code in a new python file/module and over time, step-by-step summarize functionality to a base class for easier maintance. We would prevent a big bang approach.

Yes, we can do that. I don't know whether we need another module though. We could also turn transform_manager.py into a subpackage.

Currently it is very hard to agree on a common interface when one personen thinking of a base class, and another person trying to fit in ...

I don't think we are far apart here.

[kopytjuk]

Let's close this PR in favor of https://github.com/dfki-ric/pytransform3d/pull/257