Fix bugs in PolynomialPathFitter related to the number of input coordinate samples

[nickbianco]

Fixes issue #4038

Brief summary of changes

This change fixes two bugs in PolynomialPathFitter:

• When calculating the time step used to create the "sampled" coordinates table, we previously did not take into account that the table could only contain only one row. A time step of 0.01 is now used if only one row is present.
• If the number of threads used was greater than the number of rows in the table, a segfault would occur during a logging call as described in PolynomialPathFitter segfaults on Logger call #4038.

Testing I've completed

Added a unit test to testPolynomialPathFitter.

Looking for feedback on...

CHANGELOG.md (choose one)

• updated.

---

This change is 

[carmichaelong]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nickbianco)

---

a discussion (no related file):
Looks good overall, and I like the additional test coverage.

One high-level suggestion: instead of checking and throwing if more threads are requested, is there any issue with setting the number of parallel threads to the number of rows for the user? Could print an info statement that we're doing so so that it's not a silent change behind the scenes.

[nickbianco]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @carmichaelong)

---

a discussion (no related file):

Previously, carmichaelong wrote…

Looks good overall, and I like the additional test coverage.

One high-level suggestion: instead of checking and throwing if more threads are requested, is there any issue with setting the number of parallel threads to the number of rows for the user? Could print an info statement that we're doing so so that it's not a silent change behind the scenes.

I considered that, but in general I'm not much of a fan of silent changes like this, however innocuous. The change here forces the user to understand what is going on. For example, the issue with Kristen Morgan was that she was only passing 5 rows of kinematic data; with this change, it would have alerted her that there are only 5 rows, which is likely way less than she intended.

[carmichaelong]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @nickbianco)

---

a discussion (no related file):

Previously, nickbianco (Nick Bianco) wrote…

I considered that, but in general I'm not much of a fan of silent changes like this, however innocuous. The change here forces the user to understand what is going on. For example, the issue with Kristen Morgan was that she was only passing 5 rows of kinematic data; with this change, it would have alerted her that there are only 5 rows, which is likely way less than she intended.

All good, documentation supports that we can get this out and then see where we're at with users.

One other issue did come up during testing:

Error using examplePolynomialPathFitter (line 126)
Java exception occurred:
java.lang.RuntimeException: Expected the coordinate values table to contain a column
for '/jointset/mtp_r/mtp_angle_r' (this coordinate is not locked), but it does not.
	Thrown at PolynomialPathFitter.cpp:533 in
    loadCoordinateValuesAndValidateModel().
	at
    org.opensim.modeling.opensimActuatorsAnalysesToolsJNI.PolynomialPathFitter_run(Native
    Method)
	at org.opensim.modeling.PolynomialPathFitter.run(PolynomialPathFitter.java:271)

Looks like this has come up now because there were changes made to subject_walk_scaled.osim that opened up the mtp joints, changing them from weld joints to pin joints, in the new 3D walking example. I'm guessing that updating the coordinates file is the fastest way to address this.

[nickbianco]

Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @carmichaelong)

---

a discussion (no related file):

Previously, carmichaelong wrote…

All good, documentation supports that we can get this out and then see where we're at with users.

One other issue did come up during testing:

Error using examplePolynomialPathFitter (line 126)
Java exception occurred:
java.lang.RuntimeException: Expected the coordinate values table to contain a column
for '/jointset/mtp_r/mtp_angle_r' (this coordinate is not locked), but it does not.
	Thrown at PolynomialPathFitter.cpp:533 in
    loadCoordinateValuesAndValidateModel().
	at
    org.opensim.modeling.opensimActuatorsAnalysesToolsJNI.PolynomialPathFitter_run(Native
    Method)
	at org.opensim.modeling.PolynomialPathFitter.run(PolynomialPathFitter.java:271)

Looks like this has come up now because there were changes made to subject_walk_scaled.osim that opened up the mtp joints, changing them from weld joints to pin joints, in the new 3D walking example. I'm guessing that updating the coordinates file is the fastest way to address this.

@carmichaelong Fixed the mtp joint issue and added some additional documentation based on our recent discussion. Ready for review.

[carmichaelong]

Reviewed 3 of 3 files at r3.
Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @nickbianco)

---

a discussion (no related file):

Previously, nickbianco (Nick Bianco) wrote…

@carmichaelong Fixed the mtp joint issue and added some additional documentation based on our recent discussion. Ready for review.

The changes look good, but I've been running into an issue when testing in MATLAB, which is likely an issue not introduced in this PR.

When running the new .m file, an error is thrown as follows:

Incorrect number or types of inputs or outputs for function set.

Error in examplePolynomialPathFitter (line 76)
    time.set(0, times.get(i));
    ^^^^^^^^^^^^^^^^^^^^^^^^^^

When using a breakpoint at that line and checking the type, it seems that times.get(i) is returning a java double:

K>> class(times.get(i))

ans =

    'java.lang.Double'

I'm open to either fixing this issue in this PR or in another PR.

---

Bindings/Python/examples/PolynomialPathFitter/examplePolynomialPathFitter.py line 65 at r3 (raw file):

# Add columns for the toe joints. Use a sine function to generate the
# coordinate values.An amplitude of 0.5 keeps the values within the coordinate

typo: space after values.

---

Bindings/Java/Matlab/examples/PolynomialPathFitter/examplePolynomialPathFitter.m line 67 at r3 (raw file):

% Add columns for the toe joints. Use a sine function to generate the
% coordinate values.An amplitude of 0.5 keeps the values within the coordinate

typo: space after values.

---

OpenSim/Actuators/PolynomialPathFitter.h line 159 at r3 (raw file):

 * dimension of coordinate values are needed to achieve a good fit. Users may
 * consider manually creating the coordinates value table to ensure that the
 * sampling covers the fulle range of motion for the model.

typo: fulle -> full

[nickbianco]

Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @carmichaelong)

---

a discussion (no related file):

Previously, carmichaelong wrote…

The changes look good, but I've been running into an issue when testing in MATLAB, which is likely an issue not introduced in this PR.

When running the new .m file, an error is thrown as follows:

Incorrect number or types of inputs or outputs for function set.

Error in examplePolynomialPathFitter (line 76)
    time.set(0, times.get(i));
    ^^^^^^^^^^^^^^^^^^^^^^^^^^

When using a breakpoint at that line and checking the type, it seems that times.get(i) is returning a java double:

K>> class(times.get(i))

ans =

    'java.lang.Double'

I'm open to either fixing this issue in this PR or in another PR.

I fixed the typos but I don't get the error with setting time. Here is the type I'm getting:

K>> class(times.get(0))

ans =

    'double'

I'm running this on Matlab 2024b on arm64. The OpenSim commit I'm using is from Februrary 14, 2025.

[carmichaelong]

Reviewable status: 2 of 6 files reviewed, all discussions resolved (waiting on @nickbianco)

[nickbianco]

Thanks @carmichaelong!

Build failures seem unrelated. I will merge and we can address these issues in a separate PR.