getJointId should throw if out of range

[wxmerkt]

Currently, if I query a joint that does not exist, I get the index of vector.end() which is out of bounds of njoints. Instead, if a joint does not exist, it should throw an exception (without me calling existJointName).

This relates to https://github.com/stack-of-tasks/pinocchio/blob/e038c7bf283b1df56a35014455e0e2d6f36e03ac/src/multibody/model.hxx#L209-L218

[jcarpent]

I won't really consider as a bug. You just need to take care of the fact that if the joint input name does not exist in the kinematic tree, then you have as return an index out of the bounds.

Would you think it should be better to throw?

[wxmerkt]

Yes, I don't think it is good to expect to first look up whether it exists by existJointName - because the value that comes back looks reasonable even though it's out-of-bounds. Two ways to resolve it would be to return -1 (but can't because it's unsigned) or to throw an exception with a meaningful message ("A joint with this name does not exist."). I'd prefer the latter. It clearly identifies the source as a user input error (std::invalid_argument) and gives a helpful direction how to fix it. [Otherwise we get an out of bounds when using the id somewhere else... which is pointing in the wrong direction]

[jcarpent]

OK. I share the same opinion as you at some points, and really would like to avoid other issues with respect to similar issues. Then, the next question is do you have time to handle it? It may have some other places where this strategy should be applied.

[gabrielebndn]

I do agree that the current getJointId interface is counterintuitive and error-prone. Returning -1 would force returning a signed variable, and I do not think we want that. So throwing sounds like a good solution. Keep the following in mind:

• Throwing on fail will force to scan the name vector twice if you are not sure whether a joint is indeed there, or use an (expensive) try-catch. This probably only concerns corner cases, and I do not expect it to be problematic.
• If you implement it, please take care to document it well, as it could break existing code relying on the old policy of returning njoints

[jcarpent]

This is indeed the main issue: I do think this change will break existing code. @jmirabel I think you're concerned.

[jmirabel]

My opinion on this is that this is an endless discussion that never goes well beyond Pinocchio. To me, there is no good answer whether an exception or some integer indicating a bad entry.

• If you implement it, please take care to document it well, as it could break existing code relying on the old policy of returning njoints

It will. I suggest to:

• change the prototype to: JointId getJointId (std::string name, bool throwIfNotFound = false).
• and add to the TODO list for version 3 a rework of this function.

This delays the API change to version 3.

[gabrielebndn]

I don't think this change will break existing code.

I think it is extremely possible that it would break existing code. Consider

JointIndex idx = model.getJointId(jointName);
if(idx<model.njoints) {
    // nominal case
}
else {
   // joint does not exist
}

It would not surprise me if code such as the above existed, as it is perfectly legit code and the current behaviour of returning njoints is documented as a part of the normal interface. Indeed, I am pretty sure code like the above exists, I think I wrote it in the past. The point is: do we care that the interface is broken? A few considerations:

• The interface of several method has been broken in the past
• On the other hand, changes in the interface are generally Bad and we are more and more trying to avoid them, especially as the Pinocchio user base is expanding
• SemVer would require that any change in the public interface imply an increase of the major version. However, we have never really followed SemVer. An increase in the minor version at least would be recommendable
• Implementing the throw behavior is not a risky change: when it takes place, it will be immediately obvious to users (the code launches an exception and crashes, duh!) and it will be equally obvious how to fix it (just use existJointName if needed)

[jcarpent]

Sorry @gabrielebndn, I wrote a mistake. I wanted to say:

I do think this change will break existing code

[jcarpent]

@jmirabel I think your suggestion seems to be indeed a smooth transition.

[gabrielebndn]

To push for a smooth transition, it would be nice to issue a runtime warning if the joint is not found:

Warning: the given joint name was not found. Future versions of Pinocchio will throw an exception.
Please use `existJointName` to make sure the joint exists and remove this warning.

This could be in addition to the optional boolean argument

[jmirabel]

I think we should stick to the current behavior for version 2. This warning message would pollute the std err of the people carefully doing what the current API expects you to do, that is:

JointIndex idx = model.getJointId(jointName);
if(idx<model.njoints)
...

[cmastalli]

I think we should stick to the current behavior for version 2. This warning message would pollute the std err of the people carefully doing what the current API expects you to do, that is:

JointIndex idx = model.getJointId(jointName);
if(idx<model.njoints)
...

I do disagree, 'throw'-sentences aren't a pollution; it makes the code robust to human error. Furthermore, it is very unlike that it will affect code performance.

Instead, it is trickier to have a boolean that switches behaviors. I would avoid this kind of "hacks", they make our life unnecessarily harder (simpler --> better) :)

[jmirabel]

@cmastalli the quote you mention was an answer to @gabrielebndn proposition to print a warning message (to std::cerr I guess).

I am not against throwing an exception. I only propose a method to smoothly change the behavior of getJointId so that user upgrading to version 2.x do not have to adapt their code while people upgrading to version 3.x will have to adapt their code.

[gabrielebndn]

it is trickier to have a boolean that switches behaviors. I would avoid this kind of "hacks", they make our life unnecessarily harder (simpler --> better) :)

I do agree with this

[jmirabel]

it is trickier to have a boolean that switches behaviors.

I do agree too. But we cannot change the API right now. Please, propose a way of providing the future API without breaking the code of people using correctly the current API. I am open to other propositions.

[jcarpent]

Another solution would be adding a macro of type: PINOCCHIO_THROW_IN_MODEL_GET_METHODS to allow it, by default unset.

I don't know which solution (this one or the one from @jmirabel) is better. Any idea?

[wxmerkt]

I think all these options of additional flags or macros make the whole process too complex. When is it possible to increase the major version to include these changes?

[jmirabel]

Another solution would be adding a macro of type: PINOCCHIO_THROW_IN_MODEL_GET_METHODS to allow it, by default unset.

The counterpart is that you have one more configuration option. There are already plenty...

When is it possible to increase the major version to include these changes?

Maybe creating a TODO list for Pinocchio v3 is a first step and is a good enough answer to this issue.

[gabrielebndn]

For future reference: similar changes should be applied to getFrameId too (see #1198)