remove matrix computation for calc_spiral_wrap_point

[pepbos]

This PR is a minor refactoring of the cylinder wrapping points computation.

The WrapCylinder::_make_spiral_path() function computes a rotation matrix, transposes it, multiplies is, and proceeds to use a single column. This rotation matrix can essentially be omitted.

The WrapCylinder::_make_spiral_path() function is often called during a simulation using e.g. the Rajagopal model, and takes up quite some time. This was also mentioned in #3164 , but that PR was not merged. This PR essentially takes one element of #3164 , i.e eliminating the computation of a rotation matrix.

Performance Comparison

Small change, and small perf boost of 3% for Rajagopal.

Test Name	lhs [secs]	σ [secs]	rhs [secs]	σ [secs]	Speedup
ToyDropLanding_nomuscles	0.05	0.00	0.05	0.00	0.94
Arm26	0.16	0.00	0.16	0.00	1.00
passive_dynamic	1.97	0.00	1.97	0.00	1.00
Gait2354	0.20	0.00	0.20	0.00	0.97
RajagopalModel	4.80	0.01	4.68	0.01	1.03

Brief summary of changes

The changes are to WrapCylinder::_make_spiral_path():

• the matrix m = ... on line 660 is removed, saving a matrix multiplication and transpose.
• CalcSpiralWrapPoint is redefined as a lambda function within _make_spiral_path
• the radial angle theta is computed using the arctan rather than the acos, to capture the sign.

Testing I've completed

Running the rajagopal model, I verified that the computed wrapping points are the same up to error of ~1e-14.

Looking for feedback on...

I would be interested to hear if you think it is worth revisiting #3164 , or perhaps chopping it up into smaller PRs such as these.

CHANGELOG.md

• no need to update because the change is a minor cleanup of the internal computation.

---

This change is 

[pepbos]

Hi @nickbianco , I was wondering if you could give some insight into the failing unit test for testMocoInverse. This unittest passes on ubuntu, but fails on windows with a small difference in achieved tolerance. Would you know in which direction to look for the problem?

[nickbianco]

Hi @pepbos, the test was failing here, where we're checking the errors between virtual IMU data between two conditions. One of those conditions computes IMU signals based on a 18-muscle model and a previous MocoInverse solution.

If the changes here are changing the muscle wrapping results in that model, that could be leading to the test failures. In the case that it is barely failing and everything looks good, then we might just need to update std_testMocoInverse_subject_18musc_solution.sto to reflect the wrapping updates.

[pepbos]

Hi @nickbianco thank you for your comment! I looked into the failing test:

I verified that with these changes the acceleration signals look very much the same, but the error is just a bit larger.
I assumed that this unit test should recompute the inverse solution, before comparing the acceleration signals, so I opened a pr for that, as it would be a different issue: #3476

If that is the case, it will fix the issue here as well. If I misunderstood the unit test, please let me know :)

[nickbianco]

LGTM! The Windows2022 test failure is unrelated, so feel free to merge.

[pepbos]

Awesome!