Throw if passed in name or index of marker is invalid

[aymanhab]

Fixes issue #3951

Brief summary of changes

This was mainly an issue with trying to call methods on markers that are not in the intersection of model markers and those in the trc file. Code innocently assumed names passed in to be valid. Now throw exceptions and test case beefed up accordingly.

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

• updated

---

This change is 

[carmichaelong]

@aymanhab thanks for looking into this. The code looks good, but I think I'm realizing there are some higher level questions to sort first (we can chat about this in person too if that's quicker too, but wanted to put my thoughts down here before I forget):

• Skimming the InverseKinematicsSolver code quickly, it doesn't seem like there is minimal error checking (e.g., there is at least a marker in the model and incrementing a count). It seems like IKTool deals with it here. While we probably shouldn't add more checks to IKSolver, perhaps we should be clearer somewhere about what inputs it can and can't account for?
• Given the previous bullet point, we should think about if extra checks like this one added would be worth the extra computational cost (in this case, it might be helpful, since it's an additional check and wouldn't get in the way of actually running IK?)
• From a user perspective, it might be odd that the core IKSolver will work, but then a method like this doesn't. This seems like users should be more careful about inputs (and we should make that clearer), or we should give some feedback to users when they construct the IKSolver.
• In the specific type of user error in the example, I think the correct user fix is to make sure to only include markers that are both in the model and the marker file. I'm afraid that, with the types of checks in IKSolver, there then might be cases where sometimes you'll get the right answer if the order is correct, but sometimes you'll get a different answer if there was a different order. Let me know if I'm missing some other reordering that's being done.

[aymanhab]

@carmichaelong I agree with your assessment overall. Just keep in mind that the solver is a low level class designed to be efficient use-at-your-own-risk type, while the tool is more the user facing abstraction. I'd move the methods that take an index to be private as indexing is into an internal container, and require clients to use names exclusively along with the method to get the name of marker/orientation for an index. Please let me know if that would be sufficient on balance.

[aymanhab]

Done @carmichaelong thanks for the quick turnaround and review.

[carmichaelong]

Thanks @aymanhab. Sorry one more other thing I should have mentioned: just adding the same tests that make sure all the updated methods properly throw would be helpful.

[aymanhab]

ok, done @carmichaelong will wait for ci to finish, let me know if ready to go then. Thank you

[aymanhab]

@carmichaelong ready to go?

[carmichaelong]

LGTM, thanks @aymanhab

[aymanhab]

Thanks @carmichaelong 👍