Add NMSM Pipeline contact elements for use in Moco

[SpencerTWilliams]

Brief summary of changes

This fork includes the new MeyerFregly2016Muscle and completes the implementation of the MeyerFregly2016Force included in the StationPlaneContactForce class.

The MeyerFregly2016Muscle is intended to be used without tendon compliance.

Testing I've completed

I've successfully compiled OpenSim on Ubuntu with these changes. However, I have not been able to test using the new elements.

Looking for feedback on...

I ran into an error while trying to compile a test for the MeyerFregly2016Muscle by modifying testMocoActuators.cpp:

/home/compiler/opensim-workspace/opensim-core-source/OpenSim/Moco/Test/testMocoActuators.cpp:95:26: error: expected type-specifier before ‘MeyerFregly2016Muscle’
   95 |         auto* actu = new MeyerFregly2016Muscle();
      |                          ^~~~~~~~~~~~~~~~~~~~~

I had the same error if I replaced the auto with MeyerFregly2016Muscle, so I think there may be an issue with including the new muscle as a type, even though the code files for the muscle compile correctly. Are there any issues with how I included the muscle in linking or registering the type in 2063d6f2d13177c829a9b7e39e662c0599d275dc?

CHANGELOG.md (choose one)

• updated, but needs a merge conflict resolved I don't have permission for.

---

This change is 

[nickbianco]

@SpencerTWilliams, just checking in on this PR. Let me know if you have any questions about the review.

[SpencerTWilliams]

Thanks for the suggestions! We found the documentation for our contact force equation and the meanings of the constants, so I'll push my changes with the updated class description.

The MeyerFregly2016Muscle class is based on DeGrooteFregly2016Muscle and is very similar, but it uses different active and passive force-length and force-velocity curves with different numbers of coefficients. I made this class since we want a muscle that is completely consistent with the muscle model in our code, though it will look similar to the already existing DeGrooteFregly2016Muscle.

For your comments on the if statements, I included those to guarantee that the contact force elements work exactly the same as they do in our code. We haven't had issues with discontinuities, but if these statements will cause problems for Moco, we can try removing them. The slipOffset constant likely accounts for any of these potential issues already as it prevents dividing by zero when calculating horizontal forces. This constant is not a property, so I'll move it up to be with the other hardcoded values for clarity.

[nickbianco]

Thanks for the update @SpencerTWilliams!

The MeyerFregly2016Muscle class is based on DeGrooteFregly2016Muscle and is very similar, but it uses different active and passive force-length and force-velocity curves with different numbers of coefficients.

Could you be more specific about these differences? My understanding is that the curves in the MeyerFregly2016Muscle entirely different, or do they use the same general formulations (e.g., sum of Gaussians for the force-length curve) with slightly different coefficients? If it's the latter, then I'm somewhat inclined to not introduce a new muscle class.

For your comments on the if statements, I included those to guarantee that the contact force elements work exactly the same as they do in our code. We haven't had issues with discontinuities, but if these statements will cause problems for Moco, we can try removing them.

I'm generally concerned about hiding Inf or NaN values in a component accessible to the wider userbase. If your force is produce Inf or NaN values something should probably be changed about your model, initial guess, etc. Zeroing out velSliding in that if-statement will introduce a discontinuity, but that doesn't mean your probably won't converge, especially with finite differences. I would want to know if these if-statements are merely safeguards or if they provide some functional benefit before agreeing to leave them in.

[nickbianco]

Also, just a heads up that the Force API will be changing based on https://github.com/opensim-org/opensim-core/pull/3891 soon. It might make sense to break up these two changes: we can use this PR for the contact force addition and then (if needed) open a second PR for the new muscle class.

[SpencerTWilliams]

I looked at the curve differences again, and the active force-length curves do have a similar formulation. However, the passive force-length and force-velocity curves are very different between the two muscles, with different numbers of coefficients. I think these differences would require a new muscle class.

I'll test our model more to see whether it's possible to remove the Inf and NaN values. I think this check would be safe to remove at this point, so I'll remove it if it isn't possible to get Inf and NaN values from actual double inputs.

Let me know if it ends up making more sense to make a different PR for the muscle. Would I need to change the contact force class to account for the Force API changes?

[adamkewley]

The force API changes are non-breaking so, in general, code can still override+use the "raw" computeForce APIs. The ForceProducer + ForceConsumer APIs are extra - they're to facilitate other systems, such as visualizers, force reporters, and so on.

Specifically for this PR, I don't think you need to change what you're doing in order to receive the benefits of the new API. I've already ensured that the new API is rolled out to OpenSim::PathActuator, which OpenSim::Muscle inherits from:

• https://github.com/opensim-org/opensim-core/pull/3891/files#diff-5531d7f57434db355a0f850f1439e04caeab955f43dc90a8b5ee22fb740c4ef5R201

Which ultimately means that any forces/actuations produced by MeyerFregly2016Muscle muscle class will automatically be report-able, visualize-able, etc.

Your changes to the StationPlaneContactForce look like they're separate to the API changes I made in order to ensure StationPlaneContactForce emits its forces as point forces to a ForceConsumer (so that they could potentially be visualized). The change I made is here:

• https://github.com/opensim-org/opensim-core/pull/3891/files#diff-70c5e57686d093bb16e1765aeb5cb014cb317a50baaef60508ba2c8c83812faaR76

But it's far away enough from your modifications that you may not even see a merge error.

[nickbianco]

@adamkewley, thanks for the info!

@SpencerTWilliams, yes, let's focus on the contact force additions here and have a separate PR for the muscle.

[SpencerTWilliams]

I've pushed my documentation and changes based on your suggestions. The NaN and Inf checks have been deleted from the force element. Should I go ahead and make the second PR for the muscle?

[SpencerTWilliams]

It looks like it's okay to remove that if-statement, so I took it out. I'll make the new muscle PR now.

[SpencerTWilliams]

Thanks for checking the formatting! I've just pushed my most recent changes.

I moved the StationPlaneContactForce files as you suggested, and I added some to the class documentation. Since the other two contact models in the class that I'm not familiar with have different calculations from MeyerFregly2016Force, I decided to keep the specifics of force calculations in the concrete class documentation.

I looked at the tests in testMocoContact, and I'm not sure how to make these new tests work. I think the TEMPLATE_TEST_CASE would need to be updated:

TEMPLATE_TEST_CASE("testStationPlaneContactForce", "[tropter]", 
        AckermannVanDenBogert2010Force, EspositoMiller2018Force
        /* TODO MeyerFregly2016Force */) {
    testStationPlaneContactForce<TestType>();
}

Would it work to just replace the /* TODO MeyerFregly2016Force */ with MeyerFregly2016Force? And since the other two concrete force types are not fully implemented, should we only be testing MeyerFregly2016Force?

If I'm understanding the existing tests correctly, they are currently testing that the model will be able to balance a body's weight. I think it would be good to have a few tests with given position and velocity values in the state to make sure the contact model produces the expected values from its parameters. It would be easy for me to figure out what those contact forces should be from our pipeline code, so would it be possible to create single time point states to test with?

[SpencerTWilliams]

I made your suggested change to the documentation and added to the contact element test. The MeyerFregly2016Force is now uncommented, and I commented out the other concrete types for now since they aren't fully implemented.

I wrote a test for known kinematics based on how the others were formatted. I'm haven't been able to run these tests with the code changes, but the values I gave are what I would expect the model to produce. Does the test I added make sense?

[SpencerTWilliams]

I added a TEST_CASE based on how the others were set up. These calculations are specific to the type of model, so I made the test specific to the MeyerFregly2016Force.

[nickbianco]

@SpencerTWilliams, the configuration step in these steps are failing because the CMakeLists.txt file in OpenSim/Moco is looking for StationPlaneContactForce, but it is no longer there. The lines for those components need to be removed.

The CI failures on Windows and Mac are unrelated, a fix is in the works for those (#3921).

[nickbianco]

@SpencerTWilliams, I left a few comments on why the CI builds are failing. These are compile time errors, so I would recommend recompiling locally to catch these and any other compile errors still lingering. I would also recommend running the tests locally so you don't have to wait on CI to see if they pass or fail.

[SpencerTWilliams]

Thanks for the advice on handling the tests. For running tests locally, I've been having trouble modifying the build script to compile my fork with tests on Linux, which I had thought would be more convenient at first.

I remember you had mentioned that you used CMake when you ran tests locally, so do you have directions I can follow for using CMake to compile and run OpenSim tests on Windows?

[nickbianco]

The C++ tests will be built by default, I don't think we have any CMake options to turn them off. I usually run tests locally through VS Code, but you can always run the compiled testMocoContact from the terminal.

[SpencerTWilliams]

Are there any changes I might need to make to compile with CMake locally? When I try to build through VS Code, I get this error:

[cmake] CMake Error at CMakeLists.txt:713 (find_package):
[cmake]   Could not find a package configuration file provided by "spdlog" with any
[cmake]   of the following names:
[cmake] 
[cmake]     spdlogConfig.cmake
[cmake]     spdlog-config.cmake
[cmake] 
[cmake]   Add the installation prefix of "spdlog" to CMAKE_PREFIX_PATH or set
[cmake]   "spdlog_DIR" to a directory containing one of the above files.  If "spdlog"
[cmake]   provides a separate development package or SDK, be sure it has been
[cmake]   installed.

I haven't changed the directory structure from the main branch of OpenSim, so I was wondering if it's a machine-specific issue.

[nickbianco]

spdlog is a dependency. You will need to build the CMake project located in the dependencies folder first.

But you have a working build in your Linux virtual machine? If so, the dependencies should be built there already and you should be able to run the tests.

[SpencerTWilliams]

Thanks, that error was fixed by building dependencies first! I did build OpenSim on Linux previously, but that was from the .sh build script, which clones OpenSim from the main branch before building. I also don't think it includes tests, so I wasn't sure how I needed to modify that script.

With CMake, I"m still getting a similar error:

[cmake]   Could not find a package configuration file provided by "casadi" (requested
[cmake]   version 3.4.4) with any of the following names:
[cmake] 
[cmake]     casadiConfig.cmake
[cmake]     casadi-config.cmake
[cmake] 
[cmake]   Add the installation prefix of "casadi" to CMAKE_PREFIX_PATH or set
[cmake]   "casadi_DIR" to a directory containing one of the above files.  If "casadi"
[cmake]   provides a separate development package or SDK, be sure it has been
[cmake]   installed.

It looks like it would be the same sort of issue of needing to find a directory to build first, but I can't tell where this is. Do you have a guide for which directories I should build from before the project will work?

[nickbianco]

To build CasADi, you'll need to enable the OPENSIM_WITH_CASADI CMake variable. I'd try to follow the CMake settings in the build scripts for other settings.

[SpencerTWilliams]

From the build script, I can see that CMake is only used to build dependencies and then at the opensim-core, level, so this should be the last use of CMake, then. For the OPENSIM_WITH_CASADI variable, I have this on lines 192-193 in CMakeLists.txt:

option(OPENSIM_WITH_CASADI
        "Build CasADi support for Moco (MocoCasADiSolver)." ON)

If OPENSIM_WITH_CASADI is already enabled, is there something else CMake could be missing at this point, or something with CasADi I need to install before this will work?

[nickbianco]

Sorry, you also need to set that flag when building the dependencies.

[SpencerTWilliams]

Thanks, that was turned off in the dependencies. I built the dependencies again with that flag enabled, but I'm getting the same error at the core level. In the dependencies, am I supposed to see a directory for CasADi? I'm only seeing catch2, docopt, simbody, and spdlog.

[nickbianco]

Hmm, you might need to delete your CMake cache and reload for the changes to take effect.

[SpencerTWilliams]

I'm not getting a CMakeCache.txt file, so I don't think it's a caching issue. I met with Dr. Fregly, and he'll email you about our next steps for finishing this in a minute.

[nickbianco]

Closing. Work continuing on #3949.