-
Notifications
You must be signed in to change notification settings - Fork 318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add NMSM Pipeline contact elements for use in Moco #3877
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating the PR @SpencerTWilliams! I've left a handful of comments to get started. My biggest question right now is whether or not we should create a separate class the muscle or if it is possible to use DeGrooteFregly2016Muscle
, since the implementations look very similar. Could you list the primary differences between the two classes? I'd like to get a better understanding before I review.
I ran into an error while trying to compile a test for the
MeyerFregly2016Muscle
by modifyingtestMocoActuators.cpp
:
I think you might have forgotten an include statement in the test file? Feel free to push the test changes if you want me to take a look.
Finally, we use Reviewable for code reviews (click the purple button in the PR description). Let me know if you have any questions on how it works.
Reviewed 6 of 8 files at r1, all commit messages.
Reviewable status: 6 of 8 files reviewed, 10 unresolved discussions (waiting on @SpencerTWilliams)
CHANGELOG.md
line 63 at r1 (raw file):
- Fixed bug in `report.py` preventing plotting multiple MocoParameter values. (#3808) - Added SynergyController, a controller that computes controls for a model based on a linear combination of a set of Input control signals and a set of synergy vectors. (#3796) - Added `MeyerFregly2016Muscle` and completed the implementation of the `MeyerFregly2016Force` included in the `StationPlaneContactForce` class to support NMSM Pipeline-equivalent muscle and contant models in Moco.
Typically we append the PR number in changelog entries:
Suggestion:
- Added `MeyerFregly2016Muscle` and completed the implementation of the `MeyerFregly2016Force` included in the `StationPlaneContactForce` class to support NMSM Pipeline-equivalent muscle and contant models in Moco. (#3877)
OpenSim/Actuators/MeyerFregly2016Muscle.h
line 41 at r1 (raw file):
This muscle implementation is based on the previously implemented DeGrooteFregly2016Muscle.
At first glance, this implementation looks very similar to DeGrooteFregly2016Muscle
, which makes me wonder again if it's worth making these muscle separate classes. I think it would be worth enumerating the differences between the curves in DeGrooteFregly2016Muscle
and the ones here to get a bette sense if an entirely separate class is necessary.
OpenSim/Moco/Components/StationPlaneContactForce.h
line 6 at r1 (raw file):
* OpenSim: StationPlaneContactForce.h * * -------------------------------------------------------------------------- * * Copyright (c) 2017 Stanford University and the Authors *
Might as well update the year too:
Suggestion:
* Copyright (c) 2024 Stanford University and the Authors *
OpenSim/Moco/Components/StationPlaneContactForce.h
line 146 at r1 (raw file):
B. J. (2016). Muscle Synergies Facilitate Computational Prediction of Subject-Specific Walking Motions. Frontiers in Bioengineering and Biotechnology, 4, 1055–27. http://doi.org/10.3389/fbioe.2016.00077
Now that this contact element is being fleshed out, it would be good to expand the class description here (and possibly update the reference, if relevant). Consider explaining how the contact forces are compute and include equations. Refer to MocoControlGoal on how to write equations using Doxygen comments.
OpenSim/Moco/Components/StationPlaneContactForce.h
line 178 at r1 (raw file):
const SimTK::Real y = pos[1]; const SimTK::Real velNormal = vel[1]; SimTK::Real velSliding = sqrt(vel[0] * vel[0] + vel[2] * vel[2]);
The OpenSim convention is that y-direction is vertical, but it would be good to note this somewhere in the class documentation above.
OpenSim/Moco/Components/StationPlaneContactForce.h
line 185 at r1 (raw file):
const SimTK::Real h = 1e-3; const SimTK::Real c = 5e-4; const SimTK::Real ymax = 1e-2;
It would be good to explain these hardcoded parameters in the class docs.
OpenSim/Moco/Components/StationPlaneContactForce.h
line 198 at r1 (raw file):
if (SimTK::isNaN(Fspring) || SimTK::isInf(Fspring)) { Fspring = 0; }
I'm wondering if this if-statment should be removed. I'm not sure if we added these originally or borrowed them from the Meyer et al. implementation.
OpenSim/Moco/Components/StationPlaneContactForce.h
line 211 at r1 (raw file):
if (velSliding < 1e-10) { velSliding = 0; }
Why not just let this be near zero? We try to avoid if-statements like this, as they can introduce discontinuties in the model.
OpenSim/Moco/Components/StationPlaneContactForce.h
line 217 at r1 (raw file):
if (SimTK::isNaN(horizontalForce) || SimTK::isInf(horizontalForce)) { horizontalForce = 0; }
Similar to my comment above: it would be ideal to not require this if-statement. I'm also wary about hiding NaN or Inf values that might indicate deeper issues with the model.
OpenSim/Moco/Components/StationPlaneContactForce.h
line 219 at r1 (raw file):
} const SimTK::Real slipOffset = 1e-4;
What is this? Should it be a property or grouped with the other hardcoded parameters above?
@SpencerTWilliams, just checking in on this PR. Let me know if you have any questions about the review. |
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 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 |
Thanks for the update @SpencerTWilliams!
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.
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 |
Also, just a heads up that the |
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 |
The force API changes are non-breaking so, in general, code can still override+use the "raw" 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 Which ultimately means that any forces/actuations produced by Your changes to the But it's far away enough from your modifications that you may not even see a merge error. |
@adamkewley, thanks for the info! @SpencerTWilliams, yes, let's focus on the contact force additions here and have a separate PR for the muscle. |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updates look good overall! I just have one comment to address since I'm still unsure about the if-statement for velSliding
.
Should I go ahead and make the second PR for the muscle?
Yes, go for it!
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 6 of 8 files reviewed, 5 unresolved discussions (waiting on @SpencerTWilliams)
CHANGELOG.md
line 26 at r4 (raw file):
means that API users can now now ask many `Force` components what forces they produce (see #3891 for a comprehensive overview). - Added `MeyerFregly2016Muscle` and completed the implementation of the `MeyerFregly2016Force` included in the `StationPlaneContactForce` class to support NMSM Pipeline-equivalent muscle and contant models in Moco. (#3877)
Update the changelog when removing MeyerFregly2016Muscle
from this PR.
OpenSim/Moco/Components/StationPlaneContactForce.h
line 211 at r1 (raw file):
Previously, nickbianco (Nick Bianco) wrote…
Why not just let this be near zero? We try to avoid if-statements like this, as they can introduce discontinuties in the model.
I'm still unsure about keeping this if-statement. What would be the downside of removing it?
It looks like it's okay to remove that if-statement, so I took it out. I'll make the new muscle PR now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SpencerTWilliams, I've left a few minor formatting suggestions. Now that the component looks complete and the muscle is no longer in the PR I have a few additional thoughts:
- It looks like there are no testing changes currently in the PR. It looks like the contact force is not tested at all in testMocoContact. At the very least, the contact model should be added to the test there. It might make sense to beef up the tests for the
StationPlaneContactForce
elements, we have not looked at them in a while. - Now that it is being formalized, we should probably move
StationPlaneContactForce.h/.cpp
next toSmoothSphereHalfSpaceForce
under OpenSim/Simulation/Model. TheOpenSim/Moco/Components
folder is mostly reserved for internal classes. (Note thatOSIMMOCO_API
should be changed toOSIMSIMULATION_API
when moved.) I would also moveAckermannVanDenBogert2010Force
to the bottom of the file so thatMeyerFregly2016Force
is the first concrete class underStationPlaneContactForce
. - As a follow on from 2., now that
MeyerFregly2016Force
is becoming a proper OpenSim force element, we should probably also spruce up the base classStationPlaneContactForce
. This would involve adding some class documentation and addressing the TODO comments regarding caching. I can address these TODOs in a later PR, but if you want to take a stab at the class documentation that would be great.
Reviewed 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SpencerTWilliams)
CHANGELOG.md
line 26 at r5 (raw file):
means that API users can now now ask many `Force` components what forces they produce (see #3891 for a comprehensive overview). - Completed the implementation of the `MeyerFregly2016Force` included in the `StationPlaneContactForce` class to support NMSM Pipeline-equivalent contant models in Moco. (#3877)
Suggestion:
- Completed the implementation of the `MeyerFregly2016Force` included in the `StationPlaneContactForce` class to support NMSM Pipeline-equivalent contact models in Moco. (#3877)
OpenSim/Moco/Components/StationPlaneContactForce.h
line 153 at r5 (raw file):
Biotechnology, 4, 1055–27. http://doi.org/10.3389/fbioe.2016.00077 Following OpenSim convention, this contact element assumes that the y direction
Suggestion:
Following OpenSim convention, this contact element assumes that the y-direction
OpenSim/Moco/Components/StationPlaneContactForce.h
line 197 at r5 (raw file):
with a tanh function) and viscous (modeled with a linear function) friction models may be used. */
Suggestion:
models may be used. */
Thanks for checking the formatting! I've just pushed my most recent changes. I moved the I looked at the tests in TEMPLATE_TEST_CASE("testStationPlaneContactForce", "[tropter]",
AckermannVanDenBogert2010Force, EspositoMiller2018Force
/* TODO MeyerFregly2016Force */) {
testStationPlaneContactForce<TestType>();
} Would it work to just replace the 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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current changes look good, left one comment on the added docs.
Would it work to just replace the
/* TODO MeyerFregly2016Force */
withMeyerFregly2016Force
?
That's correct! I'm doubtful the test will pass as-is, but that's how to include the model in the template tests.
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?
That would be great! Yes, you could create a SimTK::State
object via model.initSystem()
and then update the states directly via state.updQ()
, state.updU()
, etc., or use Coordinate
's setValue
and setSpeedValue
, which take the state as an argument.
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SpencerTWilliams)
OpenSim/Simulation/Model/StationPlaneContactForce.h
line 34 at r6 (raw file):
includes multiple contact models, though currently only MeyerFregly2016Force has a complete implementation. Details for how this model calculates forces are included in its documentation below.
It would be more accurate to say something like: "Multiple concrete implementation of this class are available, but currently MeyerFregly2016Force is the only complete and well-tested implementation and therefore recommended."
I don't think you need the last sentence, as it is expected that each implementation will provide its own documentation.
I made your suggested change to the documentation and added to the contact element test. The 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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The added test looks good! I added a few comments.
Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SpencerTWilliams)
OpenSim/Moco/Test/testMocoContact.cpp
line 261 at r7 (raw file):
// set of input kinematics and default parameters. template<typename T> void testKnownKinematics() {
This test looks good, but you'll need to wrap it in a TEST_CASE
to enable it. Would you expect it to pass with the other models? If so, then you could add it testStationPlaneContactForce<T>
. Otherwise, you could create a standalone test just for MeyerFregly2016Force
.
OpenSim/Moco/Test/testMocoContact.cpp
line 541 at r7 (raw file):
TEMPLATE_TEST_CASE("testStationPlaneContactForce", "[tropter]", /*AckermannVanDenBogert2010Force, EspositoMiller2018Force,*/
The tests are still passing with the other contact models, then I would leave those models uncommented here.
OpenSim/Simulation/Model/StationPlaneContactForce.h
line 32 at r7 (raw file):
Vertical force is calculated using stiffness and dissipation coefficients, while horizontal force is calculated using a friction model. Multiple concrete implementation of this class are available, but currently MeyerFregly2016Force
Suggestion:
implementations of this class are available, but currently MeyerFregly2016Force
I added a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just enabled the CI given the recent test changes, let's see if they pass! You should also be able to run the test suite locally.
Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SpencerTWilliams)
@SpencerTWilliams, the configuration step in these steps are failing because the CMakeLists.txt file in OpenSim/Moco is looking for The CI failures on Windows and Mac are unrelated, a fix is in the works for those (#3921). |
Brief summary of changes
This fork includes the new
MeyerFregly2016Muscle
and completes the implementation of theMeyerFregly2016Force
included in theStationPlaneContactForce
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 modifyingtestMocoActuators.cpp
:I had the same error if I replaced the
auto
withMeyerFregly2016Muscle
, 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 2063d6f?CHANGELOG.md (choose one)
This change is