-
Notifications
You must be signed in to change notification settings - Fork 327
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 Moco 3D tracking example with foot-ground contact #4008
Conversation
…p Moco problem to create initial guess
…dd other minor fixes
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.
awesome to have an example like this. looks good overall, just some minor comments
Reviewed 23 of 23 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @aymanhab)
Bindings/Python/examples/Moco/example3DWalking/exampleMocoTrack.py
line 219 at r1 (raw file):
problem = study.updProblem() # Set the control effort weights.
i wonder if this is a good place to be a little more explicit in the comments here to emphasize how this sets a weight of 0.1 for all controls except for the pelvis wiht a weight of 10 using a wildcard pattern (something of that nature)
OpenSim/Examples/Moco/example3DWalking/subject_walk_scaled_ContactForceSet.xml
line 77 at r1 (raw file):
<socket_half_space>/contactgeometryset/floor</socket_half_space> <!--The stiffness constant (i.e., plain strain modulus), default is 1 (N/m^2)--> <stiffness>306777.59999999998</stiffness>
nit: this and the left side have slightly different stiffness than the other contact spheres (~1.6 larger)
CHANGELOG.md
line 76 at r1 (raw file):
- Remove unused, legacy `<defaults>` blocks in `.osim` and `.xml` files (#3986). - Added `example3DWalking`, which demonstrates how to create a tracking simulation with foot-ground contact in Moco. (#4008) - Added `ModelFactory::createResidualActuators` and `ModOpAddResiduals` to make it easy to add a set of residual actuators to a model. (#4008)
could also mention the change to the similar function for reserves so that folks know they can now set reserves and residuals separately along with this new function
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 43 at r1 (raw file):
% Set minimum muscle controls and activations to 0 (default is 0.01). actuators = model.updActuators();
can you use updMuscles()
here?
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 178 at r1 (raw file):
% Construct the MocoTrack problem. track = MocoTrack(); track.setName('track_walking');
can use studyName
here
Bindings/Python/examples/Moco/example3DWalking/example3DWalking.py
line 86 at r1 (raw file):
# Construct the MocoTrack problem. track = osim.MocoTrack() track.setName('track_walking')
can use study_name
here
Bindings/Python/examples/Moco/example3DWalking/example3DWalking.py
line 237 at r1 (raw file):
# Set minimum muscle controls and activations to 0 (default is 0.01). actuators = model.updActuators()
is it possible to use updMuscles()
instead and then not have to use the if
?
OpenSim/Examples/Moco/example3DWalking/subject_walk_scaled.osim
line 14393 at r1 (raw file):
<socket_parent_frame>/bodyset/toes_l</socket_parent_frame> <!--The fixed location of the station expressed in its parent frame.--> <location>0.0877793 0.0428623 0.0226433</location>
this isn't symmetric with marker R.Toe
. intentional?
OpenSim/Examples/Moco/example3DWalking/subject_walk_scaled.osim
line 14409 at r1 (raw file):
<socket_parent_frame>/bodyset/toes_l</socket_parent_frame> <!--The fixed location of the station expressed in its parent frame.--> <location>0.00474658 0.0240174 -0.0659469</location>
this isn't symmetric with marker R.MT5
. intentional?
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 the detailed review @carmichaelong! I've addressed all of your comments.
Reviewable status: 15 of 23 files reviewed, 9 unresolved discussions (waiting on @aymanhab and @carmichaelong)
CHANGELOG.md
line 76 at r1 (raw file):
Previously, carmichaelong wrote…
could also mention the change to the similar function for reserves so that folks know they can now set reserves and residuals separately along with this new function
Done.
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 43 at r1 (raw file):
Previously, carmichaelong wrote…
can you use
updMuscles()
here?
Done.
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 178 at r1 (raw file):
Previously, carmichaelong wrote…
can use
studyName
here
Done.
Bindings/Python/examples/Moco/example3DWalking/example3DWalking.py
line 86 at r1 (raw file):
Previously, carmichaelong wrote…
can use
study_name
here
Done.
Bindings/Python/examples/Moco/example3DWalking/example3DWalking.py
line 237 at r1 (raw file):
Previously, carmichaelong wrote…
is it possible to use
updMuscles()
instead and then not have to use theif
?
Done.
Bindings/Python/examples/Moco/example3DWalking/exampleMocoTrack.py
line 219 at r1 (raw file):
Previously, carmichaelong wrote…
i wonder if this is a good place to be a little more explicit in the comments here to emphasize how this sets a weight of 0.1 for all controls except for the pelvis wiht a weight of 10 using a wildcard pattern (something of that nature)
Done.
OpenSim/Examples/Moco/example3DWalking/subject_walk_scaled.osim
line 14393 at r1 (raw file):
Previously, carmichaelong wrote…
this isn't symmetric with marker
R.Toe
. intentional?
Done.
OpenSim/Examples/Moco/example3DWalking/subject_walk_scaled.osim
line 14409 at r1 (raw file):
Previously, carmichaelong wrote…
this isn't symmetric with marker
R.MT5
. intentional?
Done.
OpenSim/Examples/Moco/example3DWalking/subject_walk_scaled_ContactForceSet.xml
line 77 at r1 (raw file):
Previously, carmichaelong wrote…
nit: this and the left side have slightly different stiffness than the other contact spheres (~1.6 larger)
It's actually a factor of 10 lower than the other spheres. Hmm, I hadn't noticed that before. I borrowed these values from Antoine's simulations. I wonder if this is intentional since these spheres are applied to the low-mass toes. Since the example is working as intended, I'm inclined to leave these values as is.
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.
Overall looks great, I meanly wanted explanations for why selections/Since you need to have cpp, py, matlab versions it may make sense to have a readme file explaining the decisions that all flavors go back to, otherwise LGTM
Reviewed 1 of 23 files at r1, 7 of 8 files at r2, all commit messages.
Reviewable status: 22 of 23 files reviewed, 16 unresolved discussions (waiting on @carmichaelong)
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 54 at r2 (raw file):
ebcf_toes_l = ExpressionBasedCoordinateForce(... 'mtp_angle_l', '-25.0*q-2.0*qdot'); ebcf_toes_l.setName('toe_damping_l');
What's the rationale for this expression?
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 65 at r2 (raw file):
ca_toes_l.setName('mtp_angle_l_actuator'); ca_toes_l.setOptimalForce(50); ca_toes_l.setMinControl(-1.0);
justification for 50?
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 90 at r2 (raw file):
end model.finalizeConnections();
Do we need this finalizeConnections? if so maybe document the reason
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 105 at r2 (raw file):
% Solve a torque-driven tracking problem to createa a kinematic % trajectory that is consistent with the ground reaction forces.
create a
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 152 at r2 (raw file):
contactTrackingWeight = 1e-2; end
Some justification of relative weights may help
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 242 at r2 (raw file):
coordName = string(coordinate.getName()); if (contains(coordName, 'beta')) continue;
Rationale for excluding 'beta' coordinates
Bindings/Java/Matlab/examples/Moco/example3DWalking/exampleMocoTrack.m
line 61 at r2 (raw file):
modelProcessor = ModelProcessor("subject_walk_scaled.osim"); % Replace the PinJoints representing the model's toes with WeldJoints. jointsToWeld = StdVectorString();
Why?
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 @aymanhab! I've addressed all comments.
I'm currently testing locally if increasing the stiffness of the toe body contact forces changes the solutions in any meaningful way. Once that's cleared up, we could merge.
Reviewable status: 22 of 23 files reviewed, 16 unresolved discussions (waiting on @aymanhab and @carmichaelong)
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 54 at r2 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
What's the rationale for this expression?
Done.
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 65 at r2 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
justification for 50?
Done.
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 90 at r2 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Do we need this finalizeConnections? if so maybe document the reason
Done.
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 105 at r2 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
create a
Done.
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 152 at r2 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Some justification of relative weights may help
Done.
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 242 at r2 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Rationale for excluding 'beta' coordinates
Done.
Bindings/Java/Matlab/examples/Moco/example3DWalking/exampleMocoTrack.m
line 61 at r2 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Why?
Done.
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.
Reviewed 1 of 8 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @carmichaelong)
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! all my comments are addressed. i'm ok either way with the toe stiffness.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nickbianco)
@aymanhab, @carmichaelong, I made a handful a changes to the example based on comments from the demos meeting yesterday. Mind taking a quick final pass on them before we merge? After adding in |
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.
Example looks much cleaner with the forces moved to a separate file that's reused by all flavors (matlab, cpp, python) also great that it now solves faster.
Reviewed 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @carmichaelong)
@carmichaelong, this is ready for a final review. |
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.
Just noticed a small typo in a comment that was propagated through the .cpp, .py, and .m files. Feel free to skip CI for these changes and then merge.
Reviewed 4 of 4 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @nickbianco)
Bindings/Java/Matlab/examples/Moco/example3DWalking/example3DWalking.m
line 101 at r5 (raw file):
% Construct a MocoStudy to track joint kinematics and ground reaction forces % using a torque-driven or muscle-driven model with foot-ground contact % elements. The objective function weights were chosen such the optimized
typo: were chosen such that the optimized
OpenSim/Examples/Moco/example3DWalking/example3DWalking.cpp
line 38 at r5 (raw file):
/// Construct a MocoStudy to track joint kinematics and ground reaction forces /// using a torque-driven or muscle-driven model with foot-ground contact /// elements. The objective function weights were chosen such the optimized
typo: such that
Bindings/Python/examples/Moco/example3DWalking/example3DWalking.py
line 32 at r5 (raw file):
# Construct a MocoStudy to track joint kinematics and ground reaction forces # using a torque-driven or muscle-driven model with foot-ground contact # elements. The objective function weights were chosen such the optimized
typo: such that
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 @carmichaelong (and @aymanhab)! Merging.
Reviewable status: 22 of 25 files reviewed, all discussions resolved (waiting on @carmichaelong)
Fixes issue #3960
Brief summary of changes
This change adds
example3DWalking.cpp
, an example that demonstrates how to create a simulation of walking tracking both kinematics and ground reaction forces using a contact model. This example lives inside theexample3DWalking
folder and relies on the existing reference data there, some of which was modified to accommodate the new example while still working forexampleMocoInverse
andexampleMocoTrack
. New reference files were added to allow adding contact forces to the model and for loading in marker weights based on anIKTaskSet
.The following minor additions are included in this PR which are incorporated in the new/updated examples:
ModelFactory::createResidualActuators
andModOpAddResiduals
to make it easy to add a set of residual actuators to a model.ModelFactory::createReserveActuators
andModOpAddReserves
with changes that make them compatible with the new residual utility methods.MocoTrack
to allow setting marker weights from aSet<MarkerWeight>
orIKTaskSet
.const
-qualifier toIKTaskSet::createMarkerWeightSet
.exampleMocoInverse
andexampleMocoTrack
based on the updated reference data and the new utility methods.Testing I've completed
WIP: testing all updated examples to make sure results are consistent across C++, Python, and Matlab.
Looking for feedback on...
I tried to check carefully that all updated examples are consistent with each other, but additional spot checking during review would be helpful.
CHANGELOG.md (choose one)
This change is