Move DeGrooteFregly2016Muscle from Moco to OpenSim

[nickbianco]

Addresses issue #2714

Brief summary of changes

• Brought in the DeGrooteFregly2016Muscle class files from Moco
• Updated header files for bindings

Testing I've completed

• Nothing yet, this PR is for review the meat of the DGF muscle code.

Looking for feedback on...

• We rely on many utilities in Moco to use this class (e.g., createVectorLinspace(), writeTableToFile, etc) that are not included currently in this PR. Some of these utilities might fit better in a PR to master (e.g., format(), which is related to logging stuff).

CHANGELOG.md (choose one)

• no need to update because...merging to feature branch.

---

This change is 

[nickbianco]

@carmichaelong I requested your review since you've been working on the Millard muscle recently, but I could see this PR having another reviewer -- up to you.

[carmichaelong]

Still in review, but posting what I have so far mainly so that I don't lose any comments and keep track of my place (through most of .h file)

Reviewed 1 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: 2 of 5 files reviewed, 11 unresolved discussions (waiting on @carmichaelong and @nickbianco)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 49 at r2 (raw file):

/// 0.049 (same as in TendonForceLengthCurve) rather than 0.0474.
///
/// The fiber damping helps with numerically solving for fiber velocity at low

I'd like to see more detail about the implementation here (since it's not in the original paper).

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 93 at r2 (raw file):

public:
    OpenSim_DECLARE_PROPERTY(activation_time_constant, double,
            "Smaller value means activation can change more rapidly (units: "

change -> increase?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 107 at r2 (raw file):

            "Scale factor for the width of the active force-length curve. "
            "Larger values make the curve wider. Default: 1.0.");
    OpenSim_DECLARE_PROPERTY(fiber_damping, double,

The comment for this property is not as clear as the other properties

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 228 at r2 (raw file):

    /// We don't need the state, but the state parameter is a requirement of
    /// Output functions.
    bool getImplicitEnabledNormalizedTendonForce(const SimTK::State&) const {

The name of this function is confusing to me, especially with the name of the following function. At first I thought this should output a Force value, and the name doesn't indicate to me that it's just doing two boolean checks.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 250 at r2 (raw file):

    /// tendon force derivative value. If integration_mode is 'explicit', this
    /// gets the value returned by getStateVariableDerivativeValue() for the
    /// 'normalized_tendon_force' state. If ignore_tendon_compliance is false,

To follow logic, I'd move this last sentence to the front of the comment.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 341 at r2 (raw file):

        // Shift the curve so its peak is at the origin, scale it
        // horizontally, then shift it back so its peak is still at x = 1.0.
        const double x = (normFiberLength - 1.0) / scale + 1.0;

I think it could be useful for users to have some info about how this scaling is done in the overall commends at the top of the file (especially for those using MATLAB/Python)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 358 at r2 (raw file):

        // horizontally, then shift it back so its peak is still at x = 1.0.
        const double x = (normFiberLength - 1.0) / scale + 1.0;
        return (1.0 / scale) *

Ah, nice scale multiplier here. I couldn't figure this out until I just leaned fully into Wolfram Alpha...

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 367 at r2 (raw file):

    /// static.
    /// Domain: [-1, 1]
    /// Range: [0, 1.794]

Perhaps note that these ranges aren't checked so it's up to user to check?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 384 at r2 (raw file):

        // Groote et al., 2016 has an error (it's missing a "-d3" before
        // dividing by "d2").
        return (sinh(1.0 / d1 * (forceVelocityMult - d4)) - d3) / d2;

I did not derive this again, but double checked that this was consistent with the supplementary materials and the note above

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 395 at r2 (raw file):

    /// through y = 0 at x = 1.0 and allows for negative forces. We do not want
    /// negative forces within the allowed range of fiber lengths, so we
    /// modified the equation to pass through y = 0 at x = 0.2. (This is not an

0.2 -> minNormFiberLength(?)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 438 at r2 (raw file):

        const double denom = exp(kPE * (1.0 + 1.0 / e0)) - temp1;
        const double temp2 = kPE / e0 * normFiberLength;
        return (e0 / kPE * exp(temp2) - normFiberLength * temp1) / denom;

Need to double check for myself by hand (or if it's equivalent), but this isn't what i got out of wolfram alpha (where kPE = k, e0 = a, minNormFiberLength =b ): https://www.wolframalpha.com/input/?i=integrate+%28%28e%5E%28k*%28x-1.0%29%2Fa%29+-+e%5E%28k*%28b-1.0%29%2Fa%29%29%2F%28e%5E%28k%29-e%5E%28k*%28b-1.0%29%2Fa%29%29%29+dx

[carmichaelong]

Still in review, part 2 (note to self: working on calcFiberVelocityInfoHelper)

Reviewable status: 2 of 5 files reviewed, 21 unresolved discussions (waiting on @nickbianco)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 38 at r2 (raw file):

// TODO prohibit fiber length from going below 0.2.

/// This muscle model was published in De Groote et al. 2016.

I would add somewhere that this is also from formulation 1 of the appendix (or at least I think it is, please correct me if I'm wrong).

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 93 at r2 (raw file):

public:
    OpenSim_DECLARE_PROPERTY(activation_time_constant, double,
            "Smaller value means activation can change more rapidly (units: "

change -> increase?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 94 at r2 (raw file):

    OpenSim_DECLARE_PROPERTY(activation_time_constant, double,
            "Smaller value means activation can change more rapidly (units: "
            "seconds).");

Could add defaults:
activation_time_constant: 0.015
deactivation_time_constant: 0.060
default_activation: 0.5
default_normalized_tendon_force: 0.5

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 472 at r2 (raw file):

    }

    /// This is the derivative of the inverse tendon-force length. Given the

derivative with respect to time?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 490 at r2 (raw file):

            const SimTK::Real& forceVelocityMultiplier,
            const SimTK::Real& normPassiveFiberForce,
            const SimTK::Real& normFiberVelocity, SimTK::Real& activeFiberForce,

(minor) could push the second argument on this line down one line for consistency

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 550 at r2 (raw file):

        if (get_ignore_tendon_compliance()) return fiberStiffnessAlongTendon;
        // TODO Millard2012EquilibriumMuscle includes additional checks that
        // the stiffness is non-negative and that the denomenator is non-zero.

denomenator -> denominator

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 666 at r2 (raw file):

    /// Export the active force-length multiplier and passive force multiplier
    /// curves to a DataTable. If the normFiberLengths argument is omitted, we
    /// use createVectorLinspace(200, minNormFiberLength, maxNormFiberLength).

This comment for createVectorLinspace I think is fine for now, but could change to be more precise when the MocoUtilities are brought over?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 691 at r2 (raw file):

    /// Currently, only Millard2012EquilibriumMuscles and Thelen2003Muscles
    /// are replaced. If the model has muscles of other types, an exception is
    /// thrown unless allowUnsupportedMuscles is true.

Could improve clarity for users: 1) include some high level details about how conversion of parameters are done (if any). my interpretation of the code is that it actually just copies the parameters over so that there isn't really any conversion, 2) explicitly state that if allowUnspportedMuscles is true that the other muscles will be converted to default DGF muscles (or at least this is how I interpreted the code)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 765 at r2 (raw file):

            const int maxIterations) const;

    // Curve parameters.

Really nice documentation

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 97 at r2 (raw file):

    SimTK_ERRCHK2_ALWAYS(get_default_normalized_tendon_force() >= 0,
            "DeGrooteFregly2016Muscle::extendFinalizeFromProperties",
            "%s: default_normalized_tendon_force must be >= 0, but it is %g.",

>= -> greater than or equal to for consistency with other messages

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 102 at r2 (raw file):

    SimTK_ERRCHK2_ALWAYS(get_default_normalized_tendon_force() <= 5,
            "DeGrooteFregly2016Muscle::extendFinalizeFromProperties",
            "%s: default_normalized_tendon_force must be <= 5, but it is %g.",

<= -> less than or equal to 5 for consistency

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 1031 at r2 (raw file):

        model.addForce(actu.release());

        // Workaround for a bug in prependComponentPathToConnecteePath().

Is this documented somewhere? Could add TODO here to update once bug is fixed?

[carmichaelong]

This is all super cool, thanks for letting me review this. I either checked the math by re-deriving equations or by making sure they were consistent with the source (either the De Groote supplementary or OpenSim's muscle classes). I noted when I did the latter rather than the former. I think I was able to check everything, so I don't think there's a need for a second reviewer.

Since functionality seems to check out, all the comments should be about documentation or style. For the most part then, they should be considered suggestions rather than any blocking comments. Please let me know if anything is unclear.

Reviewable status: 2 of 5 files reviewed, 33 unresolved discussions (waiting on @nickbianco)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 54 at r2 (raw file):

/// support for fiber dynamics is added).
///
/// This class supports tendon compliance dynamics in both explicit and implicit 

I'd like to see another high-level sentence in this paragraph about the difference in explicit vs implicit formulations that could help guide users in which to use. The rest of the paragraph is still useful, especially for maintainers

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 326 at r2 (raw file):

    const SimTK::Real tanPennationAngle =
            m_fiberWidth / mli.fiberLengthAlongTendon;
    fvi.pennationAngularVelocity =

didn't confirm math, but checked that it's consistent with other muscles

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 473 at r2 (raw file):

        // TODO the Millard model sets fiber velocity to zero when the
        //       tendon is buckling, but this may create a discontinuity.
        std::cout << "Warning: DeGrooteFregly2016Muscle '" << getName()

update with new logger?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 501 at r2 (raw file):

    if (fvi.normFiberVelocity < -1.0 && getDebugLevel() > 0) {
        std::cout << "Warning: DeGrooteFregly2016Muscle '" << getName()

update with new logger?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 538 at r2 (raw file):

    const double muscleTendonVelocity = getLengtheningSpeed(s);

    calcMuscleLengthInfoHelper(muscleTendonLength, true, mli);

Could be useful to add a small comment that this is for ignore_tendon_compliance as true so that it can be hard coded trues in these 3 calls

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 539 at r2 (raw file):

    calcMuscleLengthInfoHelper(muscleTendonLength, true, mli);
    calcFiberVelocityInfoHelper(muscleTendonVelocity, activation, true, true,

Is the second true always correct here? Perhaps add a comment why you can assume explicit here?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 580 at r2 (raw file):

    };

    const auto equilNormTendonForce = solveBisection(calcResidual,

will need to bring this over

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 642 at r2 (raw file):

std::pair<DeGrooteFregly2016Muscle::StatusFromEstimateMuscleFiberState,
        DeGrooteFregly2016Muscle::ValuesFromEstimateMuscleFiberState>
DeGrooteFregly2016Muscle::estimateMuscleFiberState(const double activation,

I'm realizing that this part of the code isn't really in use for this muscle. Feel free to ignore comments in here

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 676 at r2 (raw file):

    // Update velocity and dynamics level quantities and compute residual.
    auto dynamicsFunc = [&] {
        calcFiberVelocityInfoHelper(muscleTendonVelocity, activation, false,

Why must this be calculated with implicit form?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 718 at r2 (raw file):

                // minimum and attempt to approach from the other direction.
                fiberLength = fiberLengthPrev -
                              SimTK::sign(deltaFiberLength) * SimTK::SqrtEps;

How was the size of this step (i.e., step to switch to other side) determined?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 841 at r2 (raw file):

    // Recompute residual if cache is invalid.
    if (!isCacheVariableValid(
                s, "implicitresidual_" + STATE_NORMALIZED_TENDON_FORCE_NAME)) {

Could these lines use RESIDUAL_NORMALIZED_TENDON_FORCE_NAME instead?

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 862 at r2 (raw file):

        x = &normFiberLengths;
    } else {
        def = createVectorLinspace(

will need to move this utility over

[nickbianco]

Reviewable status: 2 of 5 files reviewed, 33 unresolved discussions (waiting on @carmichaelong and @nickbianco)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 38 at r2 (raw file):

Previously, carmichaelong wrote…

I would add somewhere that this is also from formulation 1 of the appendix (or at least I think it is, please correct me if I'm wrong).

It's based on formulations 1 and 3, which use normalized tendon force as the state variable. Explicit dynamics uses formulation 1 uses and implicit dynamics uses formulation 3. I've added clarification to the docs.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 93 at r2 (raw file):

Previously, carmichaelong wrote…

change -> increase?

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 94 at r2 (raw file):

Previously, carmichaelong wrote…

Could add defaults:
activation_time_constant: 0.015
deactivation_time_constant: 0.060
default_activation: 0.5
default_normalized_tendon_force: 0.5

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 228 at r2 (raw file):

Previously, carmichaelong wrote…

The name of this function is confusing to me, especially with the name of the following function. At first I thought this should output a Force value, and the name doesn't indicate to me that it's just doing two boolean checks.

This function is used to flag that solver that the muscle is using implicit fiber dynamics and we need to add variables to the problem representing the derivative of normalized tendon force. We use this in the output starting with 'implicitenabled_' since we do a regex search looking for output names starting with 'implicit'. So as a result this function starts with 'getImplicitEnabled'.

Kind of confusing, but most users shouldn't need to use this function unless they are trying to interface this muscle with an implicit solver. I've added some more detail to the doc comment (and for the two other "solver-facing" outputs), but let me know if you think it's still unclear.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 250 at r2 (raw file):

Previously, carmichaelong wrote…

To follow logic, I'd move this last sentence to the front of the comment.

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 341 at r2 (raw file):

Previously, carmichaelong wrote…

I think it could be useful for users to have some info about how this scaling is done in the overall commends at the top of the file (especially for those using MATLAB/Python)

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 358 at r2 (raw file):

Previously, carmichaelong wrote…

Ah, nice scale multiplier here. I couldn't figure this out until I just leaned fully into Wolfram Alpha...

Chain rule baby.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 367 at r2 (raw file):

Previously, carmichaelong wrote…

Perhaps note that these ranges aren't checked so it's up to user to check?

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 384 at r2 (raw file):

Previously, carmichaelong wrote…

I did not derive this again, but double checked that this was consistent with the supplementary materials and the note above

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 395 at r2 (raw file):

Previously, carmichaelong wrote…

0.2 -> minNormFiberLength(?)

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 472 at r2 (raw file):

Previously, carmichaelong wrote…

derivative with respect to time?

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 490 at r2 (raw file):

Previously, carmichaelong wrote…

(minor) could push the second argument on this line down one line for consistency

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 550 at r2 (raw file):

Previously, carmichaelong wrote…

denomenator -> denominator

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 666 at r2 (raw file):

Previously, carmichaelong wrote…

This comment for createVectorLinspace I think is fine for now, but could change to be more precise when the MocoUtilities are brought over?

I agree. But yes, we'll wait until we see how the utilities get converted over.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 691 at r2 (raw file):

Previously, carmichaelong wrote…

Could improve clarity for users: 1) include some high level details about how conversion of parameters are done (if any). my interpretation of the code is that it actually just copies the parameters over so that there isn't really any conversion, 2) explicitly state that if allowUnspportedMuscles is true that the other muscles will be converted to default DGF muscles (or at least this is how I interpreted the code)

I attempted to clarify. For unsupported muscles, we do copy over the base Muscle class property values, since it must derive from that anyway.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 765 at r2 (raw file):

Previously, carmichaelong wrote…

Really nice documentation

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 97 at r2 (raw file):

Previously, carmichaelong wrote…

>= -> greater than or equal to for consistency with other messages

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 102 at r2 (raw file):

Previously, carmichaelong wrote…

<= -> less than or equal to 5 for consistency

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 1031 at r2 (raw file):

Previously, carmichaelong wrote…

Is this documented somewhere? Could add TODO here to update once bug is fixed?

It is in #2388, and possibly fixed by the PR linked in that issue. I'll have to test to confirm.

[nickbianco]

Reviewable status: 2 of 5 files reviewed, 33 unresolved discussions (waiting on @carmichaelong and @nickbianco)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 326 at r2 (raw file):

Previously, carmichaelong wrote…

didn't confirm math, but checked that it's consistent with other muscles

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 473 at r2 (raw file):

Previously, carmichaelong wrote…

update with new logger?

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 501 at r2 (raw file):

Previously, carmichaelong wrote…

update with new logger?

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 538 at r2 (raw file):

Previously, carmichaelong wrote…

Could be useful to add a small comment that this is for ignore_tendon_compliance as true so that it can be hard coded trues in these 3 calls

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 539 at r2 (raw file):

Previously, carmichaelong wrote…

Is the second true always correct here? Perhaps add a comment why you can assume explicit here?

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 580 at r2 (raw file):

Previously, carmichaelong wrote…

will need to bring this over

Yep. I'm thinking I'll handle utilities in a separate PR.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 642 at r2 (raw file):

Previously, carmichaelong wrote…

I'm realizing that this part of the code isn't really in use for this muscle. Feel free to ignore comments in here

Yeah, I attempted to replicate the gradient descent method used to enforce muscle-tendon as in the Millard muscle, but it wasn't working well. I commented out this code and the relevant code in the header file for now. We could probably remove this, but I want to leave it here until we're sure our bisection approach will be adequate for all OpenSim applications.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 676 at r2 (raw file):

Previously, carmichaelong wrote…

Why must this be calculated with implicit form?

Commented out, see comment above.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 718 at r2 (raw file):

Previously, carmichaelong wrote…

How was the size of this step (i.e., step to switch to other side) determined?

Commented out, see comment above.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 841 at r2 (raw file):

Previously, carmichaelong wrote…

Could these lines use RESIDUAL_NORMALIZED_TENDON_FORCE_NAME instead?

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 862 at r2 (raw file):

Previously, carmichaelong wrote…

will need to move this utility over

Yep, will do in a subsequent PR.

[nickbianco]

Thanks for the thorough review!

I've addressed everything besides the passive muscle force curve integral comment. I'm waiting for our upcoming discussion and Moco PR before addressing that.

Reviewable status: 2 of 5 files reviewed, 33 unresolved discussions (waiting on @carmichaelong)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 49 at r2 (raw file):

Previously, carmichaelong wrote…

I'd like to see more detail about the implementation here (since it's not in the original paper).

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 54 at r2 (raw file):

Previously, carmichaelong wrote…

I'd like to see another high-level sentence in this paragraph about the difference in explicit vs implicit formulations that could help guide users in which to use. The rest of the paragraph is still useful, especially for maintainers

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 107 at r2 (raw file):

Previously, carmichaelong wrote…

The comment for this property is not as clear as the other properties

Done.

[chrisdembia]

Reviewable status: 2 of 5 files reviewed, 35 unresolved discussions (waiting on @carmichaelong and @nickbianco)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 69 at r2 (raw file):

///       use in custom solvers.
///
/// @underdevelopment

This exists in Moco's doxygen input file but not yet in opensim-core's.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 472 at r2 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Done.

I don't think this comment is quite right; this function does not really involve the inverse of the tendon force-length curve (we do not solve for tendon length). I like the second sentence more.

Perhaps replace the first sentence with a description of how the function is derived (taking the derivative of the tendon force-length curve and solving for normalized tendon velocity).

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 249 at r3 (raw file):

    /// Get whether fiber dynamics is in implicit dynamics mode when using 
    /// normalized tendon force as the state. This is useful to indicate to 
    /// solvers handle the normalized tendon force derivative and muscle-tendon

solver to handle

[nickbianco]

Reviewable status: 2 of 5 files reviewed, 35 unresolved discussions (waiting on @carmichaelong and @chrisdembia)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 69 at r2 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

This exists in Moco's doxygen input file but not yet in opensim-core's.

Thanks for catching this. We could add it OpenSim's input file, but if explicit dynamics starts to work we might not need this anymore.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 472 at r2 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

I don't think this comment is quite right; this function does not really involve the inverse of the tendon force-length curve (we do not solve for tendon length). I like the second sentence more.

Perhaps replace the first sentence with a description of how the function is derived (taking the derivative of the tendon force-length curve and solving for normalized tendon velocity).

Ah, good points. I updated based on your comments.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 249 at r3 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

solver to handle

Done.

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.cpp, line 1031 at r2 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

It is in #2388, and possibly fixed by the PR linked in that issue. I'll have to test to confirm.

Done.

[nickbianco]

Ready for review.

Reviewable status: 2 of 6 files reviewed, 35 unresolved discussions (waiting on @carmichaelong and @chrisdembia)

---

OpenSim/Actuators/DeGrooteFregly2016Muscle.h, line 438 at r2 (raw file):

Previously, carmichaelong wrote…

Need to double check for myself by hand (or if it's equivalent), but this isn't what i got out of wolfram alpha (where kPE = k, e0 = a, minNormFiberLength =b ): https://www.wolframalpha.com/input/?i=integrate+%28%28e%5E%28k*%28x-1.0%29%2Fa%29+-+e%5E%28k*%28b-1.0%29%2Fa%29%29%2F%28e%5E%28k%29-e%5E%28k*%28b-1.0%29%2Fa%29%29%29+dx

I've corrected the integral muscle curves per our discussion yesterday (including the tendon force integral curve).

[chrisdembia]

@nickbianco can you include the relevant test cases from Moco that test the muscle without Moco?

[aymanhab]

I'd like to take a look once changes are complete to get a sense of what's involved with the new muscle model. Also do we know if the muscle shows up in GUI, or how it interacts with Tools etc. Thanks

[nickbianco]

Added tests. Since our DGF muscle tests leverage Catch, I create a separate file for the basic tests that we have in Moco. I also added a test case to testMuscles.cpp.

@aymanhab sounds good. The muscle shows up in the GUI. We have tested it with current OpenSim tools, partially because we currently don't support explicit tendon compliance dynamics, which is necessary for forward simulations. It might be possible to use it with the static optimization tool, but I'm not sure.

Reviewable status: 2 of 8 files reviewed, 35 unresolved discussions (waiting on @carmichaelong and @chrisdembia)

[nickbianco]

I think we can wait to merge this PR until #2808 is merged so we can build this branch and run the tests.

Reviewable status: 2 of 8 files reviewed, 35 unresolved discussions (waiting on @carmichaelong and @chrisdembia)

[chrisdembia]

I think we can wait to merge this PR until #2808 is merged so we can build this branch and run the tests

Sounds good.

BTW, catch.hpp is already on opensim-core's master branch. See testFunctions.cpp.

[chrisdembia]

The tests fail because the symbol OpenSim::DeGrooteFregly2016Muscle::m_minNormTendonForce is not defined.

[nickbianco]

@carmichaelong ready for review.

Reviewable status: 2 of 11 files reviewed, 35 unresolved discussions (waiting on @carmichaelong and @chrisdembia)

[carmichaelong]

Changes all look good to me. I conservatively closed most discussion threads in case any are still needed, but did review all discussions and read through the (thorough!) test cases.

Reviewable status: 2 of 10 files reviewed, 6 unresolved discussions (waiting on @carmichaelong and @chrisdembia)

[nickbianco]

Thanks @carmichaelong.