Add MeyerFregly2016Muscle

[nickbianco]

Brief summary of changes

Adds MuscleFregly2016Muscle, a new Muscle class equivalent to the model available in the Neuromusculoskeletal Modeling (NMSM) Pipeline. This model will enable generating simulations in the NMSM pipeline that are more tightly integrated into the OpenSim API and hopefully improve performance.

Testing I've completed

Added testing for MuscleFregly2016Muscle to testDeGrooteFregly2016Muscle.cpp, since this muscle model is a modification of the original DeGrooteFregly2016Muscle model.

Looking for feedback on...

CHANGELOG.md (choose one)

• updated.

---

This change is 

[adamkewley]

Did a quick coding pass, I can't speak for the maths because there's lots of it.

Almost all of my comments are really about maintenance. Lets be real, specific components like these tend to get added into OpenSim and the maintenance is inevitably kicked over the fence because researchers have an outsized incentive to move onto their next thing. Therefore, I would strongly recommend spending time right now ensuring this is cleaned up, clear comments, no silly TODOs, and hardened with a maintenance-centric view while everyone's still around to help out.

@adamkewley reviewed 10 files and all commit messages, and made 21 comments.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @nickbianco).

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 138 at r1 (raw file):

            getPassiveFiberDampingForceAlongTendon, SimTK::Stage::Dynamics);

    MeyerFregly2016Muscle() { constructProperties(); }

Stick stuff like this inside the cpp file

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 147 at r1 (raw file):

    /// @{
    void extendFinalizeFromProperties() override;
    void extendAddToSystem(SimTK::MultibodySystem& system) const override;

Nit: variable names aren't necessary in the header file, you can just write SimTK::MultibodySystem& - it's obvious what it is, so also calling it system is fluff

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 169 at r1 (raw file):

    /// If ignore_activation_dynamics is true, this gets excitation instead.
    double getActivation(const SimTK::State& s) const override {

This too - every compilation unit in OpenSim doesn't need to know this, it's most likely called via a not-inlineable virtual API at runtime anyway.

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 181 at r1 (raw file):

    /// If ignore_activation_dynamics is true, this sets excitation instead.
    void setActivation(SimTK::State& s, double activation) const override {

This three...

I'm going to stop pointing it out now because this header file contains maybe 20-30 instances of this issue. Stick stuff in the cpp file unless you have a very convincing reason for it to be inlined in the header.

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 216 at r1 (raw file):

    /// Get the portion of the passive fiber force generated by the elastic
    /// element only (N).

Are you sure this API returns newtons? IIRC there is technically some dancing around in OpenSim, where forces and lengths are actually runtime-defined by the length_units and force_units fields in the model (imo, it should always just be meters and newtons, but technically all OpenSim implementations should work with any made-up units, provided all properties use those units).

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 232 at r1 (raw file):

    }

    static double getMinNormalizedFiberLength() { return m_minNormFiberLength; }

It makes no sense for a static class member function to return a variable prefixed with m_. m_ means "member (instance)", static means "not ran on an instance, therefore no member variables".

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 243 at r1 (raw file):

    }
    /// @}

Note: these all look like private helper functions, I'd even consider putting the comments etc. into the cpp file, and try to separate things into pure functions (anonymous namespace, no member variables) where possible - most of these functions are just reading a couple of properties and calculating something.

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 254 at r1 (raw file):

    /// property.
    SimTK::Real calcActiveForceLengthMultiplier(
            const SimTK::Real& normFiberLength) const {

const SimTK::Real& is an unnecessary indirection, just SimTK::Real is fine - copying a floating-point number into a function argument is a trivial operation.

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 392 at r1 (raw file):

        if (get_ignore_tendon_compliance()) return fiberStiffnessAlongTendon;
        // TODO Millard2012EquilibriumMuscle includes additional checks that

How bout you DO it before it's buried inside OpenSim alongside all the other TODOs? ;) Either it's relevant, and should be handled now, before someone writes a paper, or it's irrelevant and should be removed as a TODO.

The alternative I'm pointing out is when it is relevant, but you don't do it now, and then things are published against it. Later, it's then fixed (because someone finds an issue where its relevance is important), but the fix retrospectively invalidates the earlier results.

Or the other (more likely) alternative of it being irrelevant but a developer has to unnecessarily keep it in mind in 10 years time during a refactor.

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 565 at r1 (raw file):

    ///     include the filename. By default, the files are printed to the
    ///     current working directory.
    void printCurvesToSTOFiles(const std::string& directory = ".") const;

std::filesystem::path?

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 634 at r1 (raw file):

    }

    // Curve parameters.

Once things are moved into the cpp file you can then just put all of these constants into the cpp file and remove them from the header completely: they're magic numbers that other compilation units shouldn't need to see.

---

OpenSim/Actuators/MeyerFregly2016Muscle.h line 639 at r1 (raw file):

    // Parameters for the active fiber force-length curve.
    // ---------------------------------------------------
    constexpr static double b11 = 0.8174335195120225;

I'd consider whether some of these should be grouped into a struct or std::array or similar

---

OpenSim/Actuators/MeyerFregly2016Muscle.cpp line 36 at r1 (raw file):

const std::string MeyerFregly2016Muscle::STATE_ACTIVATION_NAME("activation");

// We must define these variables in some compilation unit (pre-C++17).

OpenSim is C++17 these days

But you shouldn't need these if you move the implementation code into the cpp file: they're a side-effect of dumping everything into the header file.

---

OpenSim/Actuators/MeyerFregly2016Muscle.cpp line 50 at r1 (raw file):

constexpr double MeyerFregly2016Muscle::b33;
constexpr double MeyerFregly2016Muscle::b43;
constexpr double MeyerFregly2016Muscle::m_minNormFiberLength;

Not m_ though, because it isn't a member variable

---

OpenSim/Actuators/MeyerFregly2016Muscle.cpp line 179 at r1 (raw file):

        const auto& activation = getActivation(s);
        const auto& excitation = getControl(s);
        static const double actTimeConst = get_activation_time_constant();

This definitely shouldn't be static, or it should be more obvious that it's reading something constant (e.g. use constexpr). As it's written, it looks like it's reading a runtime property once and then never allowing any other value ever again.

---

OpenSim/Actuators/MeyerFregly2016Muscle.cpp line 279 at r1 (raw file):

            conPassiveFiberForce + nonConPassiveFiberForce;

    // TODO revisit this if compressive forces become an issue.

Either DO it or dump it. Don't create a situation where a future developer has to change the numeric output of a muscle after things have already been published when people start noticing problems later on - especially when there's already solutions.

If this isn't actually a good solution, then dump it - don't seed a bad idea that a less-specialized developer might just uncomment in 10 years time to fix an immediate issue because they won't know all of the drawbacks.

---

OpenSim/Actuators/MeyerFregly2016Muscle.cpp line 330 at r1 (raw file):

    // passive fiber force is lumped into active fiber power. This is based on
    // the implementation in Millard2012EquilibriumMuscle (and verified over
    // email with Matt Millard).

An email conversation with an academic is irrelevant when I'm looking at this in 5-15 years time. In that future, Matt isn't going to read through this code and say "oh yes, that's completely wrong!" - he probably won't even have your original email hanging around so we can figure out the developer's intention. Either:

• Refer to specific OpenSim implementation details (e.g. "this is also performed in Millard2012EquilibriumMuscle::calcMuscleDynamicsWhateverWhatever"), so that developers can look it up and confirm it themselves and read relevant comments about it in-tree (you don't say where it happens)
• Refer (e.g. DOI) to a published document or written technical explanation (not code) of the process if it isn't in-tree

In an ideal world, you'd also write a test that exercises that behavior here or in the Millard muscle with a noisy error message like "UH OH, IF THIS TEST'S FAILING THEN YOU SHOULD PROBABLY ALSO CHECK THIS OTHER MUSCLE IMPLEMENTATION".

---

OpenSim/Actuators/MeyerFregly2016Muscle.cpp line 379 at r1 (raw file):

    if (mli.tendonLength < get_tendon_slack_length()) {
        // TODO the Millard model sets fiber velocity to zero when the

Ok, so DO something, or dump this, in version 1 - don't make some random developer have to pick up the pieces when that TODO becomes a "yeah, so these results are only valid in OpenSim <4.9" in the future, because that developer won't know all of the details that the original implementer knows.

---

OpenSim/Actuators/MeyerFregly2016Muscle.cpp line 527 at r1 (raw file):

        // Pre-emptively create a default MeyerFregly2016Muscle
        // (TODO: not ideal to do this).

You clearly know it, so DO something about it, or dump this comment.

I give no bonus points to developers for saying "yes this is a shit thing to do, and I know it, but I'll definitely DO something about it later" call it what it is: a BODGE, or a HACK, because BODGEs and HACKs might live in a codebase forever, whereas TODO implies that someone somewhere actually has a plan to fix it.

Put it this way, if it isn't on a TODO list that's realistically going to be churned during this PR or in the short-term, it isn't a TODO, it's a HACK. Future developers can then justify spending time fixing it when a bug pops up because they can look through the code and go "this is sus/shit" rather than "this is a planned future change".

---

OpenSim/Tools/Test/testSerializeOpenSimObjects.cpp line 124 at r1 (raw file):

            continue;
        } else if (dynamic_cast<MeyerFregly2016Muscle*>(clone)) {
            // TODO: we can't randomize MeyerFregly2016Muscle, since

So are you going to DO something about this, or is it just a note that should be kept in mind?