Add MocoExpressionBasedParameterGoal

[AllisonJohn]

Fixes issue #3860

Brief summary of changes

Added a new class for combining parameters with an expression in a Moco goal.

Testing I've completed

Added test cases (separate for tropter and casadi bc casadi is slow)

Looking for feedback on...

CHANGELOG.md (choose one)

• updated.

---

This change is 

[AllisonJohn]

Do I need to update https://github.com/opensim-org/opensim-core/blob/main/Bindings/moco.i? I don't think I did for the last PRs but it seems like all the goals are supposed to be there

[nickbianco]

Great work @AllisonJohn! I left a bunch of comments.

Do I need to update https://github.com/opensim-org/opensim-core/blob/main/Bindings/moco.i? I don't think I did for the last PRs but it seems like all the goals are supposed to be there

Yes, you need to update moco.i so this will appear in the bindings.

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @AllisonJohn)

---

OpenSim/Moco/MocoParameter.h line 26 at r1 (raw file):

#include <OpenSim/Common/Property.h>
#include <SimTKcommon/internal/ReferencePtr.h>
#include <SimTKcommon/internal/State.h>

Is this include necessary?

---

OpenSim/Moco/MocoParameter.h line 145 at r1 (raw file):

    void appendComponentPath(const std::string& componentPath)
    {   append_component_paths(componentPath); }
    int getElement() const {

To stay consistent with the property name:

Suggestion:

    int getPropertyElement() const {

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 27 at r1 (raw file):

#include <lepton/ExpressionProgram.h>
#include <SimTKcommon/internal/ReferencePtr.h>
#include <SimTKcommon/internal/State.h>

Based on some of my comments below, you should be able to remove some includes here:

Suggestion:

#include "MocoGoal.h"

#include "OpenSim/Moco/MocoParameter.h"
#include <lepton/ExpressionProgram.h>
#include <SimTKcommon/internal/ReferencePtr.h>

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 36 at r1 (raw file):

string should match the Lepton (lightweight expression parser) format.

Expressions can be any string that represents a mathematical expression, e.g.,

Some headings could be useful here:

Suggestion:

string should match the Lepton (lightweight expression parser) format.

# Creating expressions

Expressions can be any string that represents a mathematical expression, e.g.,

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 44 at r1 (raw file):

+, -, *, /, and ^.

@ingroup mocogoal */

I think it would be useful to add a few code examples to the class docs here. You can use the Doxygen macro @code for this:

Code snippet:

# Examples

@code
// your example here
@endcode

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 61 at r1 (raw file):

            std::string expression) : MocoGoal(std::move(name), weight) {
        constructProperties();
        setExpression(expression);

To avoid an unnecessary copy:

Suggestion:

        setExpression(std::move(expression));

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 66 at r1 (raw file):

    /** Set the mathematical expression to minimize. Variable names should match
    the names set with addParameter(). See header for explanation of
    Expressions. */

If you add a heading to the class docs above, you could refer to that instead of "Expressions".

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 68 at r1 (raw file):

    Expressions. */
    void setExpression(std::string expression) {
        set_expression(expression);

Suggestion:

        set_expression(std::move(expression));

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 75 at r1 (raw file):

    parameter, but parameters with variables that are not in the expression are
    ignored. */
    void addParameter(MocoParameter& parameter, std::string variableName) {

I don't think we need to pass a modifiable reference to the MocoParameter here:

Suggestion:

    void addParameter(const MocoParameter& parameter, std::string variableName) {

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 77 at r1 (raw file):

    void addParameter(MocoParameter& parameter, std::string variableName) {
        updProperty_parameters().appendValue(parameter);
        updProperty_variable_names().appendValue(variableName);

It is preferable to use property macro append_* for this situation:

Suggestion:

        append_parameters(parameter);
        append_variable_names(std::move(variableName));

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 85 at r1 (raw file):

            const IntegrandInput& input, SimTK::Real& integrand) const override;
    void calcGoalImpl(
            const GoalInput& input, SimTK::Vector& cost) const override;

We should support both "cost" and "endpoint constraint" mode for this goal. To enable support for endpoint constraint mode, you can override MocoGoal::getSupportsEndpointConstraintImpl()to return true. We should add at least one test for that mode too.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 3 at r1 (raw file):

/* -------------------------------------------------------------------------- *
* OpenSim: MocoExpressionBasedParameterGoal.h                                 *
 * -------------------------------------------------------------------------- *

Suggestion:

/* -------------------------------------------------------------------------- *
 * OpenSim: MocoExpressionBasedParameterGoal.h                                *
 * -------------------------------------------------------------------------- *

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 38 at r1 (raw file):

    m_program = Lepton::Parser::parse(get_expression()).optimize()
                .createProgram();
    setRequirements(1, 1);

We should set a stage dependency for this. I think we can set this to an earlier stage, like SimTK::Stage::Instance, since the parameters are just property values stored in the model. Also, we will not need integrals for this goal (see comments below):

Suggestion:

    setRequirements(0, 1, SimTK::Stage::Instance);

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 41 at r1 (raw file):

    for (int i = 0; i < getProperty_parameters().size(); i++) {
        // onylt taking the first one since they should all be the same value

Suggestion:

        // only taking the first one since they should all be the same value

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 50 at r1 (raw file):

        // get the type of the property, and element
        OPENSIM_THROW_IF_FRMOBJ(ap->isListProperty(), Exception,
            "MocoParameter does not support list properties.");

You shouldn't need this check here since it will be checked in MocoParameter.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 95 at r1 (raw file):

double MocoExpressionBasedParameterGoal::getPropertyValue(int i) const {
    OPENSIM_THROW_IF_FRMOBJ((int) m_property_refs.size() < i + 1, Exception,
            "Doesn't have that many parameters.")

nit: this could be cleaned up a bit:

Suggestion:

    OPENSIM_THROW_IF_FRMOBJ(static_cast<int>(m_property_refs.size()) <= i, Exception,
            "Property index is out of bounds.")

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 113 at r1 (raw file):

    }

    OPENSIM_THROW_FRMOBJ(Exception, "Properties not of a recognized type.");

nit:

Suggestion:

    OPENSIM_THROW_FRMOBJ(Exception, "Property at index {} is not of a "
            "recognized type.");

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 116 at r1 (raw file):

}

void MocoExpressionBasedParameterGoal::calcIntegrandImpl(

Since parameters are time-variant, we actually don't need to integrate anything for this goal. Therefore, we don't need to override calcIntegrandImpl. You can move the logic here into calcGoalImpl and assign the Lepton program evaluation directly to values[0].

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 135 at r1 (raw file):

    log_cout("        expression: {}", get_expression());
    for (int i = 0; i < getProperty_parameters().size(); ++i) {
        log_cout("        var {}: {}", get_variable_names(i),

nit:

Suggestion:

        log_cout("        variable {}: {}", get_variable_names(i),

---

OpenSim/Moco/Test/testMocoGoals.cpp line 20 at r1 (raw file):

#include "Testing.h"
#include <catch2/catch_all.hpp>

Move these back to the original location below.

---

OpenSim/Moco/Test/testMocoGoals.cpp line 1579 at r1 (raw file):

    }

    SECTION("missing parameter") {

"extra parameter"?

---

OpenSim/Moco/Test/testMocoGoals.cpp line 1593 at r1 (raw file):

        // shouldn't throw an error
        study.initCasADiSolver();

I think CHECK_NOTHROW exists for situations like this.

[AllisonJohn]

Will finish the last two comments soon!

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

---

OpenSim/Moco/MocoParameter.h line 26 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Is this include necessary?

Done.

---

OpenSim/Moco/MocoParameter.h line 145 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

To stay consistent with the property name:

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 27 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Based on some of my comments below, you should be able to remove some includes here:

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 36 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Some headings could be useful here:

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 61 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

To avoid an unnecessary copy:

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 66 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

If you add a heading to the class docs above, you could refer to that instead of "Expressions".

Will it link to the header if I put the #?

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 68 at r1 (raw file):

    Expressions. */
    void setExpression(std::string expression) {
        set_expression(expression);

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 75 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I don't think we need to pass a modifiable reference to the MocoParameter here:

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 77 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

It is preferable to use property macro append_* for this situation:

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 3 at r1 (raw file):

/* -------------------------------------------------------------------------- *
* OpenSim: MocoExpressionBasedParameterGoal.h                                 *
 * -------------------------------------------------------------------------- *

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 38 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

We should set a stage dependency for this. I think we can set this to an earlier stage, like SimTK::Stage::Instance, since the parameters are just property values stored in the model. Also, we will not need integrals for this goal (see comments below):

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 41 at r1 (raw file):

    for (int i = 0; i < getProperty_parameters().size(); i++) {
        // onylt taking the first one since they should all be the same value

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 44 at r1 (raw file):

        std::string componentPath = get_parameters(i).getComponentPaths()[0];
        const auto& component = model.getComponent(componentPath);
        const auto* ap = &component.getPropertyByName(

Could there be a case where the parameter has the same name as a parameter in the model, but it's actually not in the model? Maybe instead of adding parameters as a MocoParameter they should just pass the name.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 50 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

You shouldn't need this check here since it will be checked in MocoParameter.

ah yes... it was copied from MocoParameter

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 95 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

nit: this could be cleaned up a bit:

Can I get rid of this throw? I think I added it during debugging. There's no reason why it would be out of bounds, the methods are private

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 116 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Since parameters are time-variant, we actually don't need to integrate anything for this goal. Therefore, we don't need to override calcIntegrandImpl. You can move the logic here into calcGoalImpl and assign the Lepton program evaluation directly to values[0].

Done.

---

OpenSim/Moco/Test/testMocoGoals.cpp line 20 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Move these back to the original location below.

Done.

---

OpenSim/Moco/Test/testMocoGoals.cpp line 1579 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

"extra parameter"?

Done.

---

OpenSim/Moco/Test/testMocoGoals.cpp line 1593 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I think CHECK_NOTHROW exists for situations like this.

Done.

[AllisonJohn]

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

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 44 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I think it would be useful to add a few code examples to the class docs here. You can use the Doxygen macro @code for this:

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 85 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

We should support both "cost" and "endpoint constraint" mode for this goal. To enable support for endpoint constraint mode, you can override MocoGoal::getSupportsEndpointConstraintImpl()to return true. We should add at least one test for that mode too.

Done.

[nickbianco]

Looking good, almost there! I requested adding a Python test for this change, let me know if you have any questions about that.

Reviewed 2 of 4 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @AllisonJohn)

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 66 at r1 (raw file):

Previously, AllisonJohn wrote…

Will it link to the header if I put the #?

Hmm, that's a good question. Based on the Doxygen documentation, it looks like you can link to classes, methods, URLs, etc., but not comment headers.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 55 at r3 (raw file):

spring_goal->setExpression(fmt::format("square( p+q-{} )", STIFFNESS));
spring_goal->addParameter(*spring1_parameter, "p");
spring_goal->addParameter(*spring2_parameter, "q");

I think I would like to add a Python test for this functionality, to confirm that this pointer operation works as expected in scripting. Could you add a test to test_moco.py? You could reuse one of the C++ tests you've written.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 80 at r3 (raw file):

    /** Set the mathematical expression to minimize. Variable names should match
    the names set with addParameter(). See Creating Expressions above for an
    explanation of Expressions. */

A suggestion clarity:

Suggestion:

    the names set with addParameter(). See "Creating Expressions" in the class 
    documentation above for an explanation of how to create expressions. */

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 44 at r1 (raw file):

Could there be a case where the parameter has the same name as a parameter in the model, but it's actually not in the model?

Could you clarify this? Did you mean "parameter has the same name as a property in the model"? And if the property is in the model, how could it not "actually be in the model"?

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 95 at r1 (raw file):

Previously, AllisonJohn wrote…

Can I get rid of this throw? I think I added it during debugging. There's no reason why it would be out of bounds, the methods are private

You could change it to an assert, so that it only gets check during debug builds, which could be useful for future developers. See OpenSim::Assertion.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 124 at r3 (raw file):

    for (int i = 0; i < getProperty_variable_names().size(); ++i) {
        std::string variableName = get_variable_names(i);
        parameterVars[variableName] = getPropertyValue(i);

Can simplify here:

Suggestion:

        parameterVars[get_variable_names(i)] = getPropertyValue(i);

[AllisonJohn]

I didn't run the test because my clion isn't set up for python, so hopefully it passes ci

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

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 66 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Hmm, that's a good question. Based on the Doxygen documentation, it looks like you can link to classes, methods, URLs, etc., but not comment headers.

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 55 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

I think I would like to add a Python test for this functionality, to confirm that this pointer operation works as expected in scripting. Could you add a test to test_moco.py? You could reuse one of the C++ tests you've written.

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h line 80 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

A suggestion clarity:

Done.

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 44 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Could there be a case where the parameter has the same name as a parameter in the model, but it's actually not in the model?

Could you clarify this? Did you mean "parameter has the same name as a property in the model"? And if the property is in the model, how could it not "actually be in the model"?

Oh I guess I meant MocoProblem instead of model since the parameters are added to the problem. So I can't get the parameter from just the name. I also got getPropertyName confused with getName. So never mind this comment!

---

OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp line 124 at r3 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Can simplify here:

Done.

[nickbianco]

@AllisonJohn, the tests are currently failing because moco.i has not yet been updated to include MocoExpressionBasedParameterGoal.

[AllisonJohn]

It has been added! I think I forgot to re-request review before

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

[nickbianco]

Great job!

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