MATLAB dq_kinematics_plot modifying the object

[ffasilva]

Hi, @dqrobotics/developers,

I recently noticed dq_kinematics_plot(robot,q) function on MATLAB is modifying the received object robot. Namely, it sets robot.q and forces it to be a column vector.

Although I understand the applications of this, I believe this is a counterintuitive and undesired behavior. A plot function should be "read-only" and not modify any of the received inputs. If we intend to start using robot.q for its methods (e.g., having robot.fkm() rather than robot.fkm(q), plot(robot)/robot.plot() rather than plot(robot,q), etc...), which I never understood why we don't, we should have dedicated methods robot.set_joint_configuration(q)/robot.get_joint_configuration(). This opens a bit of a parallel discussion, but regardless I do not think dq_kinematics_plot(robot,q) function should be modifying the robot object.

However, since this is not a proper issue, I would like to hear your opinions on that.

TL;DR: plot() should never modify the received objects.

Kind regards,
Frederico

[mmmarinho]

Just to mention that for the secondary discussion that showed up during your description, I have a strong opinion. So when we start that discussion, I’d like to be part of it.

[bvadorno]

@mmmarinho Would you like to express your opinion before I close this discussion?

[mmmarinho]

@bvadorno Sure thing!

We have discussed this only internally, but my addition to this discussion is related to the "confusion" in the existence of

DQ_Kinematics.fkm(q)

in which I'm making DQ_Kinematics explicit to show that this is a static method. However, we currently have no method

robot.fkm()

with the internal management of q.

1. The benefits of the static method DQ_Kinematics.fkm(q)

---

Besides the fact that is has always been this way and the entire corpus of the library usage depends on it, I'd like to mention two major benefits from having it static

a. The developers can isolate the mathematical operations from the inner workings of the class. The static method MUST be self contained and, by definition, can't rely on private member variables, so we know for sure that the given inputs are enough to calculate the output without any obfuscated inner state.

b. We do not have to worry about synchronisation issues related to using multiple threads. This is something that is not usually something to reflect too deeply in MATLAB, but it is an important point in CPP. This responsibility is left to the user, who might or might not need to do so, but always can manage by themselves without our interference.

2. When is it better to rely on robot.fkm()

---

Caching.

The cleanest way to prevent the library from having to recalculate a Jacobian without a lot of manual work is to cache prior results in the class. The best way to do so is to store q and such cached Jacobians in the class and manage them in a transparent way. It is not trivial but can be a powerful strategy to reduce computational load.

Hence, supposing that a future proposition for a robot.fkm() does not break any existing code and is compatible with most of the existing use cases, in my opinion it would only be worth the trouble if at least it provided caching capabilities with thread safety. Otherwise, it would be just increasing difficulty/problems/confusion without anything concrete to gain from it.

[bvadorno]

Hi @mmmarinho,

Thank you for summarizing those points. Your suggestion for caching has, indeed, the potential to speed up computation, which is on my ever-increasing list of things I want to do. (Also, we have too much on our plates now regarding the library.)

I'll eventually migrate the caching suggestion to our wish list, but I will leave this discussion open now. One thing I need to think more about is how to ensure thread safety because a conservative approach might even backfire in terms of performance. For example, using mutex to lock access to data when updating the robot configuration might prevent other threads from reading any values from the object until all internal states are updated (i.e., while the cache is updated). Depending on how long it takes, threads requiring computation results from the locked object might wait longer than if they did it using the "static" methods.

Best wishes,
Bruno

[bvadorno]

Hi @ffasilva,

I recently noticed dq_kinematics_plot(robot,q) function on MATLAB is modifying the received object robot. Namely, it sets robot.q and forces it to be a column vector.

Although I understand the applications of this, I believe this is a counterintuitive and undesired behavior. A plot function should be "read-only" and not modify any of the received inputs.

I don't remember why the plot function transposes the vector q; I will have to look at it because I implemented it a decade ago.

If we intend to start using robot.q for its methods (e.g., having robot.fkm() rather than robot.fkm(q), plot(robot)/robot.plot() rather than plot(robot,q), etc...), which I never understood why we don't, we should have dedicated methods robot.set_joint_configuration(q)/robot.get_joint_configuration(). This opens a bit of a parallel discussion, but regardless I do not think dq_kinematics_plot(robot,q) function should be modifying the robot object.

I don't remember discussing the intention to use q as you described, but changes like that should be treated with care due to side effects. Also, such change would significantly affect how we implement things, which is a good reason not to consider it soon. So, in a nutshell, we don't do otherwise because it's been working fine for more than a decade and changing it will create so many problems and require adapting so much code that it is not worth it.

@mmmarinho, feel free to jump in.

Best wishes,
Bruno

[bvadorno]

@ffasilva, please raise this issue of the plot function changing q as a bug.

Regarding the alternative usage you mentioned, as I mentioned previously, we won't proceed with any modifications because of potential side effects. Be aware that low-level changes have huge repercussions across the library and typically can take dozens of hours to implement, ranging from modifications in other classes to modifications in C++ and Python bindings to changes of several test classes and dozens of examples.

Furthermore, when considering those modifications, one should not restrict oneself to the particular change of a specific class. Instead, they must consider the overall usage and how users would behave when using the modified implementation. The usage gains for this particular suggestion (if any) should significantly outweigh the usage drawbacks (if any). Examples of gains would be cleaner implementation, potential for reduced implementation errors, faster execution, less memory usage, etc. Apart from a marginal potential for cleaner implementation, I'm unsure how this would greatly benefit the library now. Nonetheless, I'm open to talking more about it if you come up with convincing arguments.

All the best,
Bruno

[ffasilva]

Dear @bvadorno,

I will open this plot function issue as a bug report.

Regarding the parallel discussion I started, I was not thinking so much about the potential computational advantages, as @mmmarinho envisioned, but rather about the programming paradigm aspects of it. The current status of the library is a mix of object-oriented programming and functional programming; whilst we do have classes and make plenty of use of their properties, the kinematic methods that interface with the users are entirely functional-based. To keep the fkm example we have been using, robot.fkm(q) essentially works as fkm(robot, q) since robot and q are disconnected entities as q is not a property of robot. Every kinematics function a given user routinely interacts with within the library has the same behavior.

From the way I see it, the benefits of migrating the library to exclusive object-oriented programming relate to the advantages of the paradigm itself. Namely,

• Encapsulation: This discussion we are having right now is an example of the problems we face because of the functional-based behavior of the kinematic methods. Since non-set methods in the library currently receive variables as inputs, we are limited in how we can modify their implementation as they need to remain compatible with a given argument, or we will break every end-user code relying on that specific signature. Similarly, if we do decide to modify those inputs, whether it is changing q from column to row vector or having a dedicated data struct, rather than simply adjusting a set() accordingly, we would have to revise a collection of methods. If refactoring this is already costly now, it will get worse as the library grows to include dynamics.
• Abstraction: Also related to the encapsulation aspects of the library, objects currently are entities with physical properties (link length, number of joints, wheel size, etc.) but do not have a strictly coherent internal kinematic state. Take, for instance, the (somewhat ludicrous) example below (assuming robot and plane_normal were previously defined):

pose_jacobian = robot.pose_jacobian(q1);
x_pose = robot.fkm(q2);
Jplane = plane_jacobian(pose_jacobian, x_pose, plane_normal)

Although the calculated Jplane relates to a robot that simultaneously has configurations q1 and q2---thus, having no logical physical interpretation---this is computationally allowed by the library. Currently, when it comes down to the kinematic methods, objects in the library do not have internal states, and it's left to the user to maintain coherence throughout the code. This is another example of something whose complexity will increase once we incorporate robot dynamics and model composition into the library.

However, other than the refactoring costs of those modifications, I do agree with @mmmarinho that one of the drawbacks of this format is that parallelization will be more challenging to handle.

Kind regards,
Frederico

[bvadorno]

Hi @ffasilva,

Thank you for the response. All the @dqrobotics/developers are well-versed in object-oriented programming and understand the advantages you mentioned when using object-oriented programming. However, you are looking at the problem from a narrow perspective. All languages the library supports, Python, C++, and MATLAB, support object-oriented, procedural, and functional paradigms. (By the way, what you called a 'functional' paradigm is actually 'procedural' or 'imperative'). So, mixing different paradigms is acceptable if done consciously and purposefully (see advantage number 1 that @mmmarinho mentioned).

I won't go through the details of why this decision was made in the past, but the fact is that it's been done, and we must deal with it. If you want to take the burden of refactoring the code, go for it. However, any change will only be accepted under the following conditions:

1. It's thread-safe.
2. It enables caching.
3. All examples run without errors.
4. No side effects cause cascade changes (domino effect).
5. All changes are done simultaneously in C++, and the criteria above hold.
6. All Python bindings are duly adapted to accommodate the C++ changes.

I estimate you would need around three to six months dedicated exclusively (i.e., without even thinking about implementing the DQ_Dynamics classes) to this refactoring under all the conditions above, especially because there are several classes and components that nobody used or participated in the implementation, apart from myself. Some implementations include results from unpublished theoretical development that I made available for students who couldn't advance their work if I hadn't done otherwise. Furthermore, each pull request will take dozens of hours to be reviewed. So, you'd be using your time and other people's time for something with a potential small gain, assuming that 1 to 6 above are fulfilled.

Let's be clear here: the inclusion of DQ_Dynamics and composition classes won't add any relevant complexity under the current strategy ('static' methods) because we already have other composition classes for serialization and parallelization of kinematic structures (DQ_WholeBody and DQ_CooperativeDualTaskSpace). Even if it did, we could encapsulate the implementation only for those new classes. Would it be ideal? Perhaps not. But if it's justified from a technical point of view (e.g., doing otherwise would be indeed very complex from the user's point of view), then it becomes acceptable.

The thread-safety is one of my least concerns because, as I mentioned in a previous post, we can solve it using mutexes. However, depending on how conservatively it's done, the performance might be greatly impacted in a multi-threaded implementation. So, there is another criterion to the list above:

7. The thread-safe implementation must not have worse performance than the current implementation.

In summary, developing DQ Robotics is all about fulfilling a multi-criteria problem while respecting several constraints, including uniformity between different languages, which is one of the flagship advantages of the library.

Kind regards,
Bruno