Mostly clarifying questions but otherwise LGTM

Reviewed 8 of 8 files at r1.
Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @carmichaelong)

OpenSim/Simulation/Model/ExpressionBasedPathForce.cpp line 75 at r1 (raw file):

        (SimTK::isNaN(get_resting_length()) || get_resting_length() < 0),
        Exception,
        "Expected the 'resting_length' property to be less than zero, but "

Expected length to be less than zero? typo in the message?

OpenSim/Simulation/Model/ExpressionBasedPathForce.cpp line 124 at r1 (raw file):

    AbstractGeometryPath& path = updPath();
    if (path.getPreScaleLength(s) > 0.0) {

Not sure how this works (how/if PreScale was called). Should we explicitly allow this class to handle prescale/postscale in case we decide to change default behavior?

OpenSim/Simulation/Model/ExpressionBasedPathForce.h line 111 at r1 (raw file):

    ExpressionBasedPathForce(const std::string& name, 
            double restLength, const std::string& expression, bool clampStretch);

These constructors look quite similar, can we combine with a default value for clampStretch?

OpenSim/Simulation/Test/testForces.cpp line 514 at r1 (raw file):

    double ldot = force.getLengtheningSpeed(osim_state);
    double analytical_force = 2.0 / (stretch * stretch) - 
                              3.0 * (stretch - 0.2) * (1 + 0.0123456789 * ldot);

Love this test 👍

Thanks! Address all comments and ready for review again.

Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @aymanhab and @carmichaelong)

OpenSim/Simulation/Model/ExpressionBasedPathForce.h line 111 at r1 (raw file):

I think it's better to separate them like this for use in scripting, or at least I don't think SWIG will create separate constructors if a default argument is used for clampStretch.

OpenSim/Simulation/Model/ExpressionBasedPathForce.cpp line 75 at r1 (raw file):

Good catch! Done.

OpenSim/Simulation/Model/ExpressionBasedPathForce.cpp line 124 at r1 (raw file):

This is based on what PathSpring does. It seems like a reasonable modification to make here too. If we do change default behavior, then we'll have to change it in PathSpring too, so it might make sense to keep the behavior of those classes consistent for now.

OpenSim/Simulation/Test/testForces.cpp line 514 at r1 (raw file):

Done.

LGTM except for one comment to resolve

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nickbianco)

OpenSim/Simulation/Model/ExpressionBasedPathForce.h line 111 at r1 (raw file):

Actually SWIG handles this exactly as expected by creating two functions, so I'd remove the second constructor to eliminate the possibility that they get out of sync.

Ready for final review.

Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @aymanhab)

OpenSim/Simulation/Model/ExpressionBasedPathForce.h line 111 at r1 (raw file):

Ah, excellent! Done.

Great job @nickbianco

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nickbianco)