From 26b6749ad24f7e86285f78e899cff38e62124aba Mon Sep 17 00:00:00 2001 From: Nicholas Bianco Date: Mon, 26 Aug 2024 13:54:25 -0700 Subject: [PATCH 1/4] Formatting and documentation tweaks --- .../MocoExpressionBasedParameterGoal.cpp | 7 ++--- .../MocoExpressionBasedParameterGoal.h | 27 ++++++++++--------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp index 499379bb07..a4c0f624e0 100644 --- a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp +++ b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp @@ -67,16 +67,13 @@ void MocoExpressionBasedParameterGoal::initializeOnModelImpl(const Model& model) } // test to make sure all variables are there - try - { + try { std::map parameterVars; for (int i = 0; i < getProperty_variable_names().size(); ++i) { parameterVars[get_variable_names(i)] = getPropertyValue(i); } m_program.evaluate(parameterVars); - } - catch (Lepton::Exception& ex) - { + } catch (Lepton::Exception& ex) { std::string msg = ex.what(); std::string help = ""; if (msg.compare(0, 30, "No value specified for variable")) { diff --git a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h index 804eac87e6..1c034ebef5 100644 --- a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h +++ b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h @@ -26,18 +26,19 @@ namespace OpenSim { class Model; -/** Minimize an arithmetic expression of parameters. This goal supports any -number of MocoParameters that are combined into a single goal. The expression -string should match the Lepton (lightweight expression parser) format. +/** Minimize or constrain an arithmetic expression of parameters. + +This goal supports both "cost" and "endpoint constraint" modes and can be +defined using any number of MocoParameters. The expression string should match +the Lepton (lightweight expression parser) format. # Creating Expressions Expressions can be any string that represents a mathematical expression, e.g., "x*sqrt(y-8)". Expressions can contain variables, constants, operations, -parentheses, commas, spaces, and scientific e notation. The full list of -operations (also in Lepton::Operation) is: -sqrt, exp, log, sin, cos, sec, csc, tan, cot, asin, acos, atan, sinh, cosh, -tanh, erf, erfc, step, delta, square, cube, recip, min, max, abs, as well as +parentheses, commas, spaces, and scientific "e" notation. The full list of +operations is: sqrt, exp, log, sin, cos, sec, csc, tan, cot, asin, acos, atan, +sinh, cosh, tanh, erf, erfc, step, delta, square, cube, recip, min, max, abs, +, -, *, /, and ^. # Examples @@ -50,7 +51,7 @@ auto* spring2_parameter = mp.addParameter("spring2_stiffness", "spring2", auto* spring_goal = mp.addGoal(); double STIFFNESS = 100.0; // minimum is when p + q = STIFFNESS -spring_goal->setExpression(fmt::format("square( p+q-{} )", STIFFNESS)); +spring_goal->setExpression(fmt::format("square(p+q-{})", STIFFNESS)); spring_goal->addParameter(*spring1_parameter, "p"); spring_goal->addParameter(*spring2_parameter, "q"); @endcode @@ -75,10 +76,10 @@ class OSIMMOCO_API MocoExpressionBasedParameterGoal : public MocoGoal { set_expression(std::move(expression)); } - /** Set the mathematical expression to minimize. Variable names should match - the names set with addParameter(). See "Creating Expressions" in the class - documentation above for an explanation of how to create expressions. - */ + /** Set the mathematical expression to minimize or constraint. Variable + names should match the names set with addParameter(). See "Creating + Expressions" in the class documentation above for an explanation of how to + create expressions. */ void setExpression(std::string expression) { set_expression(std::move(expression)); } @@ -104,7 +105,7 @@ class OSIMMOCO_API MocoExpressionBasedParameterGoal : public MocoGoal { /** Get the value of the property from its index in the property_refs vector. This will use m_data_types to get the type, and if it is a Vec type, it uses - m_indices to get the element to return, both at the same index i.*/ + m_indices to get the element to return, both at the same index i. */ double getPropertyValue(int i) const; OpenSim_DECLARE_PROPERTY(expression, std::string, From d23d002ae367783d60d11733c03263be29e6f9d8 Mon Sep 17 00:00:00 2001 From: Nicholas Bianco Date: Mon, 26 Aug 2024 14:30:33 -0700 Subject: [PATCH 2/4] A few more tweaks --- .../MocoExpressionBasedParameterGoal.cpp | 28 +++++++++---------- .../MocoExpressionBasedParameterGoal.h | 12 ++++---- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp index a4c0f624e0..99811d1146 100644 --- a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp +++ b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp @@ -28,12 +28,12 @@ using namespace OpenSim; void MocoExpressionBasedParameterGoal::constructProperties() { constructProperty_expression(""); constructProperty_parameters(); - constructProperty_variable_names(); + constructProperty_variables(); } void MocoExpressionBasedParameterGoal::initializeOnModelImpl(const Model& model) const { - OPENSIM_THROW_IF_FRMOBJ(get_expression() == "", Exception, + OPENSIM_THROW_IF_FRMOBJ(get_expression().empty(), Exception, "The expression has not been set. Use setExpression().") m_program = Lepton::Parser::parse(get_expression()).optimize() .createProgram(); @@ -69,19 +69,17 @@ void MocoExpressionBasedParameterGoal::initializeOnModelImpl(const Model& model) // test to make sure all variables are there try { std::map parameterVars; - for (int i = 0; i < getProperty_variable_names().size(); ++i) { - parameterVars[get_variable_names(i)] = getPropertyValue(i); + for (int i = 0; i < getProperty_variables().size(); ++i) { + parameterVars[get_variables(i)] = getPropertyValue(i); } m_program.evaluate(parameterVars); } catch (Lepton::Exception& ex) { - std::string msg = ex.what(); - std::string help = ""; - if (msg.compare(0, 30, "No value specified for variable")) { - help = " Use addParameter() to add a parameter for this variable, " - "or remove the variable from the expression for this goal."; - } - OPENSIM_THROW_FRMOBJ(Exception, fmt::format("Expression evaluate error:" - " {}.{}", msg, help)); + const std::string msg = ex.what(); + std::string undefinedVar = msg.substr(32, msg.size() - 32); + OPENSIM_THROW_FRMOBJ(Exception, + fmt::format("Parameter variable '{}' is not defined. Use " + "addParameter() to explicitly define this variable. Or, " + "remove it from the expression.", undefinedVar)); } } @@ -107,8 +105,8 @@ double MocoExpressionBasedParameterGoal::getPropertyValue(int i) const { void MocoExpressionBasedParameterGoal::calcGoalImpl( const GoalInput& input, SimTK::Vector& values) const { std::map parameterVars; - for (int i = 0; i < getProperty_variable_names().size(); ++i) { - parameterVars[get_variable_names(i)] = getPropertyValue(i); + for (int i = 0; i < getProperty_variables().size(); ++i) { + parameterVars[get_variables(i)] = getPropertyValue(i); } values[0] = m_program.evaluate(parameterVars); } @@ -116,7 +114,7 @@ void MocoExpressionBasedParameterGoal::calcGoalImpl( void MocoExpressionBasedParameterGoal::printDescriptionImpl() const { log_cout(" expression: {}", get_expression()); for (int i = 0; i < getProperty_parameters().size(); ++i) { - log_cout(" variable {}: {}", get_variable_names(i), + log_cout(" variable {}: {}", get_variables(i), get_parameters(i).getName()); } } diff --git a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h index 1c034ebef5..91e11eae7a 100644 --- a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h +++ b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h @@ -88,9 +88,9 @@ class OSIMMOCO_API MocoExpressionBasedParameterGoal : public MocoGoal { expression string. All variables in the expression must have a corresponding parameter, but parameters with variables that are not in the expression are ignored. */ - void addParameter(const MocoParameter& parameter, std::string variableName) { + void addParameter(const MocoParameter& parameter, std::string variable) { append_parameters(parameter); - append_variable_names(std::move(variableName)); + append_variables(std::move(variable)); } protected: @@ -109,11 +109,11 @@ class OSIMMOCO_API MocoExpressionBasedParameterGoal : public MocoGoal { double getPropertyValue(int i) const; OpenSim_DECLARE_PROPERTY(expression, std::string, - "The expression string with variables q0-q9."); + "The expression string defining this cost or endpoint constraint."); OpenSim_DECLARE_LIST_PROPERTY(parameters, MocoParameter, - "MocoParameters to use in the expression."); - OpenSim_DECLARE_LIST_PROPERTY(variable_names, std::string, - "Variable names of the MocoParameters to use in the expression."); + "Parameters included in the expression."); + OpenSim_DECLARE_LIST_PROPERTY(variables, std::string, + "Variables names corresponding to parameters in the expression."); mutable Lepton::ExpressionProgram m_program; // stores references to one property per parameter From 1df173490ad282a680ff525b4e40623b67929973 Mon Sep 17 00:00:00 2001 From: Nicholas Bianco Date: Mon, 26 Aug 2024 16:24:33 -0700 Subject: [PATCH 3/4] Respond to review comment --- OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h index 91e11eae7a..2de96dd42d 100644 --- a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h +++ b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.h @@ -76,7 +76,7 @@ class OSIMMOCO_API MocoExpressionBasedParameterGoal : public MocoGoal { set_expression(std::move(expression)); } - /** Set the mathematical expression to minimize or constraint. Variable + /** Set the arithmetic expression to minimize or constrain. Variable names should match the names set with addParameter(). See "Creating Expressions" in the class documentation above for an explanation of how to create expressions. */ From d16a77712bcf1159bc3c3e7503517f83deca6723 Mon Sep 17 00:00:00 2001 From: Nicholas Bianco Date: Wed, 28 Aug 2024 15:24:53 -0700 Subject: [PATCH 4/4] Add back if-statement back for Lepton parsing checks --- OpenSim/Common/ExpressionBasedFunction.cpp | 14 +++++++++----- .../MocoGoal/MocoExpressionBasedParameterGoal.cpp | 14 +++++++++----- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/OpenSim/Common/ExpressionBasedFunction.cpp b/OpenSim/Common/ExpressionBasedFunction.cpp index 63710eb885..f92e4edefe 100644 --- a/OpenSim/Common/ExpressionBasedFunction.cpp +++ b/OpenSim/Common/ExpressionBasedFunction.cpp @@ -69,11 +69,15 @@ class SimTKExpressionBasedFunction : public SimTK::Function { } } catch (Lepton::Exception& ex) { std::string msg = ex.what(); - std::string undefinedVar = msg.substr(32, msg.size() - 32); - OPENSIM_THROW(Exception, - fmt::format("Variable '{}' is not defined. Use " - "setVariables() to explicitly define this variable. Or, " - "remove it from the expression.", undefinedVar)); + if (msg.compare(0, 30, "No value specified for variable")) { + std::string undefinedVar = msg.substr(32, msg.size() - 32); + OPENSIM_THROW(Exception, + fmt::format("Variable '{}' is not defined. Use " + "setVariables() to explicitly define this variable. " + "Or, remove it from the expression.", undefinedVar)); + } else { + OPENSIM_THROW(Exception, "Lepton parsing error: {}", msg); + } } } diff --git a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp index 56948cfe22..0e316ecf3b 100644 --- a/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp +++ b/OpenSim/Moco/MocoGoal/MocoExpressionBasedParameterGoal.cpp @@ -75,11 +75,15 @@ void MocoExpressionBasedParameterGoal::initializeOnModelImpl(const Model& model) m_program.evaluate(parameterVars); } catch (Lepton::Exception& ex) { const std::string msg = ex.what(); - std::string undefinedVar = msg.substr(32, msg.size() - 32); - OPENSIM_THROW_FRMOBJ(Exception, - fmt::format("Parameter variable '{}' is not defined. Use " - "addParameter() to explicitly define this variable. Or, " - "remove it from the expression.", undefinedVar)); + if (msg.compare(0, 30, "No value specified for variable")) { + std::string undefinedVar = msg.substr(32, msg.size() - 32); + OPENSIM_THROW_FRMOBJ(Exception, + fmt::format("Parameter variable '{}' is not defined. Use " + "addParameter() to explicitly define this variable. Or, " + "remove it from the expression.", undefinedVar)); + } else { + OPENSIM_THROW_FRMOBJ(Exception, "Lepton parsing error: {}", msg); + } } }