New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor constraint- and cost-related methods in MathematicalProgram to decouple Constraint and Binding constructors from MathematicalProgram interface #5890

Open
EricCousineau-TRI opened this Issue Apr 19, 2017 · 37 comments

Comments

Projects
None yet
6 participants
@EricCousineau-TRI
Contributor

EricCousineau-TRI commented Apr 19, 2017

EDITS:

  1. Updated to incorporated decisions from earlier conversations.
  2. Incorporating steps to execute Action Items here.
  3. Updated per next comment.
  4. Updated per f2f summary and additional discussion.
  5. Updated per f2f discussion on 2017-05-10.

Overview

Refactor constraint- and cost-related methods in MathematicalProgram to decouple Constraint and Binding constructors from each other, and as much as possible in the MathematicalProgram interface.

This will cause usage to change from:

prog.AddLinearComplementarityConstraint(M_, q_, vd_);

to

prog.AddConstraint(MakeLinearComplementarityConstraint(M_, q_), vd_);

but would help in making the role of MathematicalProgram more succinct (a registration container, not a factory).

This is more broadly scoped to address the heart of the issue, and
avoid fighting the language as in #5885.

This scope is also different from #4559 in that this focuses on MathematicalPrograms role as a generalized container for constraints and costs.

\cc @soonho-tri @hongkai-dai @sammy-tri @ggould-tri @siyuanfeng-tri @RussTedrake

Motivation

  • Simplify and decouple interfaces
  • No one appears to be externally consuming the direct interface to MathematicalProgram yet, so now may be a good (easier) time to change it
  • Additional motivations from #5885

Functionality

  • Make the newer interface as succinct as before, maybe with just a few more added symbols
  • Avoid attempting to do specializations with parameter packs like the plague
    • Sticky issues with greedy template consumption and the various references types that would normally match a template during normal inference (see #5885 for more details)

PR Plan

  1. [DONE #5931] Implement a shim for Cost to distinguish from Constraint types, while leveraging existing infrastructure. (Necessary for overloading Add(Binding<Cost>) vs Add(Binding<Constraint>).)
  2. [DONE #5950] Add dynamic container dispatch. (Leverage dynamic_cast<> as a first step, then add a new issue to supply polymorphic IDs to Cost / Constraint to avoid RTTI.)
    1. Migrate all usages of Constraint as costs to use Cost explicitly.
  3. [DONE #5972] Migrate all symbolic extraction helpers to symbolic_extraction.h, place in namespace drake::solvers::internal. Refactor usages.
    1. PR 3.5, New Substep: Reduce Binding overloads pertaining to VectorXDecisionVariable. - Not as important if we decouple constructors. Can defer to later issue.
  4. [DONE #6021] Move creation functionality from MathematicalProgram::Add.*(symbolic::...) to internal::Parse.*(symbolic::...), and make the Add.*(...) methods use this functionality.
    1. Add the Add(...) overloads to redirect to AddCost(...) / AddConstraint(...).
    2. Add methods for specialized types / constructions, such as MakeL2NormCost, MakeQuadraticErrorCost, etc.
  5. [Updated] Fully realize the new API
    1. Move Parse.*() outside of internal:: scope.
    2. Introduce BindingBundle, and move PositiveSemiDefinite creation (and part of variable creation) outside of MathematicalProgram.
    3. Add to Python API to use new interface.
    4. PR 5.5, New Substep: Ensure ParseCost(ScalarOrVector1d<Expression>...) and ParseConstraint(ScalarOrMatrixXd<Formula>...) works with all expression types.
      1. Ensure that AddCost(...) and AddConstraint(...) works well with forwarding to these methods.
  6. Refactor existing usages (C++ and Python).
    1. Remove old interfaces.
  7. Clean up:
    1. Simplify EvaluatorBase
    2. Remove CostShim stopgap measures.
    3. Clean up documentation.
    4. Rename create_[cost|constraint].cc to parse_[cost|constraint].cc
  • Old 1. Implement Step 1 from #5885 - Not useful. Overloads are sufficient. Additional registration steps can be handled manually.

Current Interface

(Copied from #5885)

Current form of functions (return types, arguments, and high-level overview):

  1. Binding<${CONSTRAINT}> AddConstraint(...)
    1. ctor_args<Binding<${CONSTRAINT}>>...
      • Construct the specific Binding (if necessary), and add to appropriate container
  2. Binding<${CONSTRAINT}> Add${CONSTRAINT}(...)
    1. ctor_args<${CONSTRAINT}>..., ctor_args<Binding<${CONSTRAINT}>>(const shared_ptr<${CONSTRAINT}>&, ...)...
      • Construct the specific Constraint (if necessary)
      • Create the specific Binding, and then call AddConstraint(...)
  3. Binding<Constraint> AddPolynomialConstraint(...)
    1. ctor_args<PolynomialConstraint>..., ctor_args<Binding<Constraint>>(const shared_ptr<Constraint>&, ...)...
  4. typename std::enable_if<..., Binding<${CONSTRAINT}>::type Add${CONSTRAINT}(...)
    1. symbolic_args...
      • Do fancy symbolic constraint inference magic, and then call one of the above methods

Types:

  • ctor_args<${CLASS}>... - Constructor arguments for a given ${CLASS}
    • ctor_args<${CLASS}>(${ARG}, ...)... - Constructor arguments for an overload beginning with certain argument types, returning all but the explicitly specified arguments
  • symbolic_args... - Arguments relating to a
  • ${CONSTRAINT} - A direct placement of Constraint or any sub-class. I see repetitions for the following types (amounts to ~1000 / ~2600 lines in mathematical_program.h, ~600 / ~1400 lines in mathematical_program.cc):
    • Constraint
    • LinearConstraint
    • LinearEqualityConstraint
    • QuadraticConstraint
    • BoundingBoxConstraint
    • LorentzConeConstraint
    • RotatedLorentzConeConstraint
    • LinearComplementarityConstraint
    • PolynomialConstraint
      • Will return Binding<Constraint>, such that it can detect affine cases and delegate to simpler constraint types.
    • PositiveSemiDefiniteConstraint
    • LinearMatrixInequalityConstraint

Suggested Interface

  • Handle simple patterns only

    • const Binding<C>& Add[Cost|Constraint](...)
      • const Binding<C>&
      • const shared_ptr<C>&, ctor_args<Binding<C>>...
        • Not decoupled from Binding, but offer some convenience
      • (new) pair<constraint_ptr<C>, Vars>&&, ctor_args<constraint_binding<C>>(Vars, ...)
        • For now, only permit the use of rvalue references && to incentivize user to take the return type directly from Create.*(Cost|Constraint)(symbolic...)
      • constraint_type<C>&&, ctor_args<constraint_binding<C>>... - if useful
    • (new) const Binding<COut>& AddCost<COut>(const ScalarOrMatrix<symbolic::Expression>&)
      • For explicit foreknowledge of functional form
      • Same for AddConstraint(...)
  • Separate Constraint and Cost classes

    • As a stopgap, could do something to the effect of:

        typename<C>
        class GenericCost : public C { };
        // etc
        using QuadraticCost = GenericCost<QuadraticConstraint>;
      
  • Creation of Constraint and Cost classes will be delegated to Make${TYPE}(numeric...) and Parse${TYPE}(symbolic...) methods.

  • If it is still strongly desired to have Add${CONSTRAINT}(...), the Binding constructor arguments could be placed first, such as:

    • Add(...)
      • ctor_args<constraint_binding<C>>..., ref<constraint_type<C>>
      • This can be handled in a more straightforward fashion for parameter packs
      • However, this looks a little less pristine:
        • Add(vd_, CreateLinearConstraint(A_, b_))

Example Transition

C++

Before:

    // C++
    // - Numeric
    prog.AddLinearConstraint(A_, b_, vd_);
    prog.AddLinearEqualityConstraint(A_, b_, vd_);
    prog.AddLinearComplementarityConstraint(M_, q_, vd_);
    prog.AddQuadraticCost(Matrix2d::Identity(), Vector2d::Zero(), vd_);
    prog.AddQuadraticErrorCost(Q_, x_desired_, vd_);
    // - Symbolic
    prog.AddLinearConstraint(A_ * x_ <= b_);
    prog.AddLinearEqualityConstraint(A_ * x_ == b_);

After:

    // - Numeric
    prog.AddConstraint(MakeLinearConstraint(A_, b_), vd_);
    prog.AddConstraint(MakeLinearEqualityConstraint(A_, b_), vd_);
    prog.AddConstraint(MakeLinearComplementarityConstraint(M_, q_), vd_);
    prog.AddCost(MakeQuadraticCost(Matrix2d::Identity(), Vector2d::Zero()), vd_);
    prog.AddCost(MakeQuadraticErrorCost(Q_, x_desired_), vd_);
    // - Symbolic
    prog.AddConstraint(A_ * x_ <= b_);
    /* alt: */ prog.AddConstraint(ParseLinearConstraint(A_ * x_ <= b_)); // NOTE: This will constrain the parsing to only linear inequalities.
    prog.AddConstraint(A_ * x_ == b_);
    /* alt: */ prog.AddConstraint(ParseLinearEqualityConstraint(A_ * x_ == b_));  // NOTE: See above.

Python

Before:

    # Python
    # - Numeric
    prog.AddLinearConstraint(A, b, vd)
    prog.AddLinearEqualityConstraint(A, b, vd)
    prog.AddLinearComplementarityConstraint(M, q, vd)
    prog.AddQuadraticCost(np.eye(2), np.zeros(2), vd)
    prog.AddQuadraticErrorCost(Q, x_desired, vd)
    # - Symbolic
    prog.AddLinearConstraint(A * x <= b)
    prog.AddLinearEqualityConstraint(A * x == b)

After:

    # Python
    from pydrake.mathematicalprogram.common import *
    # - Numeric
    prog.AddConstraint(MakeLinearConstraint(A, b), vd)
    " alt: "; prog.AddConstraint(MakeLinearConstraint(A, b), vd)
    prog.AddConstraint(MakeLinearEqualityConstraint(A, b), vd)
    prog.AddConstraint(MakeLinearComplementarityConstraint(M, q), vd)
    prog.AddCost(MakeQuadraticCost(np.eye(2), np.zeros(2)), vd)
    prog.AddCost(MakeQuadraticErrorCost(Q, x_desired), vd)
    # - Symbolic
    prog.AddConstraint(A * x <= b)
    " alt: "; prog.AddConstraint(ParseLinearConstraint(A * x <= b))
    prog.AddConstraint(A * x == b)
    " alt: "; prog.AddConstraint(ParseLinearEqualityConstraint(A * x == b))

Symbolic Overloads

Presently, we provide a suite of overloads within MathematicalProgram for Constraint constructors and symbolic expressions.

Unfortunately, trying to use the same symbols to do external (decoupled) overloads causes ambiguity in C++, such as:

class LinearConstraint : public Constraint {
    LinearConstraint(20 different numeric overloads...);
}

Binding<LinearConstraint> LinearConstraint(Formula...);

Proposal:

  • Add(...) only accepts a rigid set of non-symbolic overloads, such that it will error out if passed any symbolics directly.
  • Will accept symbolics filtered through Parse${TYPE}(...)
  • Add${TYPE}(...) will forward directly to Add(Create${TYPE}(...))
  • Introduce internal::Parse${TYPE}(symbolic...)
    • Parse${TYPE} will NOT should try not to use parameter packs to minimize any chance of ambiguity

This means that there is redundancy between two easily-reachable methods, Add(Create${TYPE}(...)) and Add${TYPE}(...), which may yield more options than needed. However, this is the lesser of many evils.

Potential Drawbacks

  • Refactoring in general
  • Auto-completion for MathematicalProgram is nowhere near as convenient
    • Followup: Meh on this point.
  • Some redundancy
    • Solution: Provide Best Practices

Suggested Best Practices

  • USE prog.AddCost(...), prog.AddConstraint(...)
    • USE prog.Add${TYPE}(Parse${FULL_TYPE}(symbolic...)), etc., if you know the functional form of your symbolic expression ahead of time

TODO

  • Ensure Best Practices are agreed upon

@EricCousineau-TRI EricCousineau-TRI self-assigned this Apr 19, 2017

@hongkai-dai

This comment has been minimized.

Show comment
Hide comment
@hongkai-dai

hongkai-dai Apr 19, 2017

Contributor

What is the return type of

prog.Add(LinearConstraint(A, b), vars);

? It seems to me now we can only return Binding<Constraint> type, instead of a Binding<LinearConstraint> object? I hope we can still get the specific type of the constraint back, since later we may need to update the constraint.

Currently one use case is

auto binding = prog.AddLinearConstraint(A, lb, ub, vars);
prog.Solve();
// Now update the constraint, and solve the problem again with the updated constraint
binding.constraint()->UpdateConstraint(new_A, new_lb, new_ub);
prog.Solve();

Can we still achieve the same "update and resolve problem" using this new interface?

Contributor

hongkai-dai commented Apr 19, 2017

What is the return type of

prog.Add(LinearConstraint(A, b), vars);

? It seems to me now we can only return Binding<Constraint> type, instead of a Binding<LinearConstraint> object? I hope we can still get the specific type of the constraint back, since later we may need to update the constraint.

Currently one use case is

auto binding = prog.AddLinearConstraint(A, lb, ub, vars);
prog.Solve();
// Now update the constraint, and solve the problem again with the updated constraint
binding.constraint()->UpdateConstraint(new_A, new_lb, new_ub);
prog.Solve();

Can we still achieve the same "update and resolve problem" using this new interface?

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 19, 2017

Contributor

Yup! The return-type would be the same as the current API. It could return constraint_binding<C>, where:

  • constraint_binding<C> = Binding<C> in the general case, to facilitate the usage you mentioned above
  • constraint_binding<PolynomialConstraint> = Binding<Constraint>, to permit the constraint to simplified if need be, as is done presently by AddPolynomialConstraint
    • NOTE: This will require run-time check when C = Constraint, but I believe this would be useful (especially in the general symbolic case) and relatively straightforward.
Contributor

EricCousineau-TRI commented Apr 19, 2017

Yup! The return-type would be the same as the current API. It could return constraint_binding<C>, where:

  • constraint_binding<C> = Binding<C> in the general case, to facilitate the usage you mentioned above
  • constraint_binding<PolynomialConstraint> = Binding<Constraint>, to permit the constraint to simplified if need be, as is done presently by AddPolynomialConstraint
    • NOTE: This will require run-time check when C = Constraint, but I believe this would be useful (especially in the general symbolic case) and relatively straightforward.
@hongkai-dai

This comment has been minimized.

Show comment
Hide comment
@hongkai-dai

hongkai-dai Apr 19, 2017

Contributor

If I understand correctly, we will not be able add the symbolic constraint in the following way any more

prog.AddConstraint(2 * x + 3 * y <= 1); // prog add a LinearConstraint

But instead we need to do

prog.Add(LinearConstraint(2 * x + 3 * y <= 1)); // The user has to tell the program, that this is a linear constraint.

is this the case?

Contributor

hongkai-dai commented Apr 19, 2017

If I understand correctly, we will not be able add the symbolic constraint in the following way any more

prog.AddConstraint(2 * x + 3 * y <= 1); // prog add a LinearConstraint

But instead we need to do

prog.Add(LinearConstraint(2 * x + 3 * y <= 1)); // The user has to tell the program, that this is a linear constraint.

is this the case?

@ggould-tri

This comment has been minimized.

Show comment
Hide comment
@ggould-tri

ggould-tri Apr 19, 2017

Contributor

Presumably it would be straightforward to write a CreateSymbolicConstraint(...) factory that dispatched to the appropriate subclass based on the expression provided?

Contributor

ggould-tri commented Apr 19, 2017

Presumably it would be straightforward to write a CreateSymbolicConstraint(...) factory that dispatched to the appropriate subclass based on the expression provided?

@hongkai-dai

This comment has been minimized.

Show comment
Hide comment
@hongkai-dai

hongkai-dai Apr 19, 2017

Contributor

It shouldn't be hard to write the factory. Sorry I did not realize the factory is the plan. If it is not too much of work, could you put the skeleton of this API for this factory here?

Contributor

hongkai-dai commented Apr 19, 2017

It shouldn't be hard to write the factory. Sorry I did not realize the factory is the plan. If it is not too much of work, could you put the skeleton of this API for this factory here?

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 19, 2017

Contributor

Yup! The factory would be something to the effect of:

// Numeric overload(s)
shared_ptr<LinearConstraint> CreateLinearConstraint(const Matrix& A, const Vector& b) {
    return make_shared<LinearConstraint>(A, b);
}
// Symbolic overload(s)
Binding<LinearConstraint> CreateLinearConstraint(Formula f) {
    tuple<Matrix, Vector, VarList> info = magic_symbolic_extract<LinearConstraint>(f);
    auto constraint = CreateLinearConstraint(info.get<0>(), info.get<1>());
    return Binding<LinearConstraint>(constraint, info.get<2>());
}

Then the usage would like:

prog.Add(CreateLinearConstraint(A, b), vars); // numeric
prog.Add(CreateLinearConstraint(A * x <= b)); // symbolic

To generalize, the factory would like something like the effect of:

shared_ptr<constraint_type<C>> Create${CONSTRAINT}(args...) {
    return make_shared<constraint_type<C>>(args...);
}

Binding<constraint_type<C>> Create${CONSTRAINT}(Formula f) {
    // Really rough view. Would not suggest trying to split tuples, TBH
    tuple<Args...> args = magic_symbolic_extract<C>(f);
    auto constraint = Create${CONSTRAINT}(args<0...N-1>...);
    return Binding<C>(constraint, args.last());
}

NOTE: Edited Overview in the orignal post to use Option 3, Create${CONSTRAINT}.

Contributor

EricCousineau-TRI commented Apr 19, 2017

Yup! The factory would be something to the effect of:

// Numeric overload(s)
shared_ptr<LinearConstraint> CreateLinearConstraint(const Matrix& A, const Vector& b) {
    return make_shared<LinearConstraint>(A, b);
}
// Symbolic overload(s)
Binding<LinearConstraint> CreateLinearConstraint(Formula f) {
    tuple<Matrix, Vector, VarList> info = magic_symbolic_extract<LinearConstraint>(f);
    auto constraint = CreateLinearConstraint(info.get<0>(), info.get<1>());
    return Binding<LinearConstraint>(constraint, info.get<2>());
}

Then the usage would like:

prog.Add(CreateLinearConstraint(A, b), vars); // numeric
prog.Add(CreateLinearConstraint(A * x <= b)); // symbolic

To generalize, the factory would like something like the effect of:

shared_ptr<constraint_type<C>> Create${CONSTRAINT}(args...) {
    return make_shared<constraint_type<C>>(args...);
}

Binding<constraint_type<C>> Create${CONSTRAINT}(Formula f) {
    // Really rough view. Would not suggest trying to split tuples, TBH
    tuple<Args...> args = magic_symbolic_extract<C>(f);
    auto constraint = Create${CONSTRAINT}(args<0...N-1>...);
    return Binding<C>(constraint, args.last());
}

NOTE: Edited Overview in the orignal post to use Option 3, Create${CONSTRAINT}.

@hongkai-dai

This comment has been minimized.

Show comment
Hide comment
@hongkai-dai

hongkai-dai Apr 19, 2017

Contributor

I think I still have the question here, is that the symbolic case of the proposal, we cannot do

prog.Add(A * x <= b);

to add a linear constraint, but the user has to tell the program that the constraint is linear, by using

prog.Add(CreateLinearConstraint(A * x <= b));

On the other hand, in the current implementation, if we call

prog.AddConstraint(A*x<=b);

It knows the newly created constraint is linear. see https://github.com/RobotLocomotion/drake/blob/master/drake/solvers/mathematical_program.cc#L584~L621. This is for AddCost, and @soonho-tri and I plan to add the similar idea to AddConstraint, such that
AddConstraint(const symbolic::Expression& e) will add different types of constraint, depending on the structure of the expression e (linear, quadratic, polynomial, etc).

Contributor

hongkai-dai commented Apr 19, 2017

I think I still have the question here, is that the symbolic case of the proposal, we cannot do

prog.Add(A * x <= b);

to add a linear constraint, but the user has to tell the program that the constraint is linear, by using

prog.Add(CreateLinearConstraint(A * x <= b));

On the other hand, in the current implementation, if we call

prog.AddConstraint(A*x<=b);

It knows the newly created constraint is linear. see https://github.com/RobotLocomotion/drake/blob/master/drake/solvers/mathematical_program.cc#L584~L621. This is for AddCost, and @soonho-tri and I plan to add the similar idea to AddConstraint, such that
AddConstraint(const symbolic::Expression& e) will add different types of constraint, depending on the structure of the expression e (linear, quadratic, polynomial, etc).

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 19, 2017

Contributor

Ah, missed that, sorry!

Yes, this system could certainly handle that case using this bottom-up approach.
That could be covered by a generic factor, CreateConstraint / CreateCost, and then use a run-time check to place the constraint in the appropriate container. My thought was to have the layout as such:

  • Specific invocations of Add<C>(...) place the constraint directly in the type-specified container, no run-time checks
  • Specialize Add<Constraint>(...) to call DoAddConstraintDynamic(Binding<Constraint>)
    • This performs a run-time check to see if a constraint can be more efficiently placed (e.g., using dynamic_cast<> on the pointer and iterating through the types to see if they're addable, or explicitly querying some ID)

Then, in the generic symbolic form of CreateConstraint (or CreateCost), any constraint type can be returned, such as:

  • return CreatePolynomialCost(e) -> Binding<Constraint>(CreatePolynomialCost(coeffs), vars_vec)
  • return CreateQuadraticCostWithMonomialToCoeffMap(...) -> Binding<Constraint>(CreateQuadraticCost(q, b), vars_vec)
  • return CreateLinearCost(c, vars_vec)

which then gets fed to Add<Constraint>(...) or Add<Cost>(...) which will place the constraint in the appropriate container at run-time, as mentioned above.

This does incur the additional run-time check, but I believe it will be negligible relative to the rest of the computations, and permits decoupling the interfaces.

I was hoping that this could possibly simplify y'all's path, so please do let me know if it throws a wrench in your plans.

EDIT:
Additionally, formulas and expressions could be used to determine constraint vs. cost: (I assume y'all may have already planned for this):

Binding<Constraint> CreateGeneric(const symbolic::Formula&) ...
Binding<Cost> CreateGeneric(const symbolic::Expression&) ...

Thus you could get

prog.Add(x.transpose() * Q * x + f.transpose()); // -> Add<Cost>(...)
prog.Add(A * x <= b); // -> Add<Constraint>(...)
Contributor

EricCousineau-TRI commented Apr 19, 2017

Ah, missed that, sorry!

Yes, this system could certainly handle that case using this bottom-up approach.
That could be covered by a generic factor, CreateConstraint / CreateCost, and then use a run-time check to place the constraint in the appropriate container. My thought was to have the layout as such:

  • Specific invocations of Add<C>(...) place the constraint directly in the type-specified container, no run-time checks
  • Specialize Add<Constraint>(...) to call DoAddConstraintDynamic(Binding<Constraint>)
    • This performs a run-time check to see if a constraint can be more efficiently placed (e.g., using dynamic_cast<> on the pointer and iterating through the types to see if they're addable, or explicitly querying some ID)

Then, in the generic symbolic form of CreateConstraint (or CreateCost), any constraint type can be returned, such as:

  • return CreatePolynomialCost(e) -> Binding<Constraint>(CreatePolynomialCost(coeffs), vars_vec)
  • return CreateQuadraticCostWithMonomialToCoeffMap(...) -> Binding<Constraint>(CreateQuadraticCost(q, b), vars_vec)
  • return CreateLinearCost(c, vars_vec)

which then gets fed to Add<Constraint>(...) or Add<Cost>(...) which will place the constraint in the appropriate container at run-time, as mentioned above.

This does incur the additional run-time check, but I believe it will be negligible relative to the rest of the computations, and permits decoupling the interfaces.

I was hoping that this could possibly simplify y'all's path, so please do let me know if it throws a wrench in your plans.

EDIT:
Additionally, formulas and expressions could be used to determine constraint vs. cost: (I assume y'all may have already planned for this):

Binding<Constraint> CreateGeneric(const symbolic::Formula&) ...
Binding<Cost> CreateGeneric(const symbolic::Expression&) ...

Thus you could get

prog.Add(x.transpose() * Q * x + f.transpose()); // -> Add<Cost>(...)
prog.Add(A * x <= b); // -> Add<Constraint>(...)
@hongkai-dai

This comment has been minimized.

Show comment
Hide comment
@hongkai-dai

hongkai-dai Apr 19, 2017

Contributor

I feel we should separate AddCost and AddConstraint, instead of combining them into Add() function. It is pretty clear what

prog.Add(x+y<=1);

means. But I think the meaning of

prog.Add(2*x);

is not clear enough, to say it means "add 2*x to the cost". So I would propose we still keep two function signatures AddCost and AddConstraint.

Contributor

hongkai-dai commented Apr 19, 2017

I feel we should separate AddCost and AddConstraint, instead of combining them into Add() function. It is pretty clear what

prog.Add(x+y<=1);

means. But I think the meaning of

prog.Add(2*x);

is not clear enough, to say it means "add 2*x to the cost". So I would propose we still keep two function signatures AddCost and AddConstraint.

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 19, 2017

Contributor

Would you be OK to have the API as follows?

Add(CreateConstraint(...)...)   -> AddConstraint(...)
Add(CreateCost(...)...)  -> AddCost(...)
AddCost(symbolic)
AddConstraint(symbolic)

Alternatively, we would have some repetition (which would be fine, just want to make sure we give that the go ahead):

 AddConstraint(CreateLinearConstraint(...)...)
 AddCost(CreateQuadraticCost(...)...)
Contributor

EricCousineau-TRI commented Apr 19, 2017

Would you be OK to have the API as follows?

Add(CreateConstraint(...)...)   -> AddConstraint(...)
Add(CreateCost(...)...)  -> AddCost(...)
AddCost(symbolic)
AddConstraint(symbolic)

Alternatively, we would have some repetition (which would be fine, just want to make sure we give that the go ahead):

 AddConstraint(CreateLinearConstraint(...)...)
 AddCost(CreateQuadraticCost(...)...)
@hongkai-dai

This comment has been minimized.

Show comment
Hide comment
@hongkai-dai

hongkai-dai Apr 19, 2017

Contributor

I like your first API better

Add(CreateConstraint(...)...)   -> AddConstraint(...) // Non symbolic case
Add(CreateCost(...)...)  -> AddCost(...)  // Non symbolic case
AddCost(symbolic) // symbolic case
AddConstraint(symbolic) // symbolic case
Contributor

hongkai-dai commented Apr 19, 2017

I like your first API better

Add(CreateConstraint(...)...)   -> AddConstraint(...) // Non symbolic case
Add(CreateCost(...)...)  -> AddCost(...)  // Non symbolic case
AddCost(symbolic) // symbolic case
AddConstraint(symbolic) // symbolic case
@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 19, 2017

Contributor

Sorry for the verbosity, but just to recap, I've listed out our three alternatives. (I needed this to have my own internal debate :P ) I will update the main post once I flesh it out a tad more.

  • Alternative 1 (Current Winner) - Yields some redundant and closely related options, but provides clarity when context may not be easily seen
  • Alternative 2 - Minimal redundancy, grammatically and semantically
  • Alternative 3 - Verbose but clear, grammatically redundant

TODO: Will need to consider options when using templates, such as being able to also specify Add(CreateCost<QuadraticCost>(symbolic)...) -> compile-time container placement.

Alternative 1

  • Add(...) only accepts a rigid set of non-symbolic overloads, such that it will error out if passed any symbolics directly.

  • Will accept symbolics filtered through Create${TYPE}(...)

  • Add${TYPE}(...) will forward directly to Add(Create${TYPE}(...))

  • Create${TYPE} will NOT use parameter packs to minimize any chance of ambiguity

      Add(x.transpose() * Q * x);
          // Compilation error
    
      Add(CreateCost(x.transpose() * Q * x));
          // Compiles fine. Run-time container determination.
    
      AddCost(x.transpose() * Q * x);
          // Compiles fine. Run-time container determination.
    
      Add(CreateQuadraticCost(x.transpose() * Q * x));
          // Compiles fine. Compile-time container determination.
    
      AddCost(CreateQuadraticCost(x.transpose() * Q * x));
          // Compiles fine. Compile-time container determination.
    

This means that there is redundancy between two easily-reachable methods, Add(Create${TYPE}(...)) and Add${TYPE}(...), which may yield more options than needed. However, this is the lesser of many evils.

Alternative 2

Instead of adding AddCost(...) and AddConstraint(...) with symbolic overloads, force the user to call Create${TYPE}(...) methods:

Add(CreateCost(symbolic)...)
Add(CreateConstraint(symbolic)...)

This makes things concise. The only drawback is, if you don't have a concise name, you may end up with something like:

class Stuff {
private:
    LinearConstraint x; // Poorly named constraint
};
...
prog.Add(x, vars); // Wait... Is this a cost or a constraint?

Granted, this is effectively the user's fault, but it also may be hard to clearly delineate overloads for Add(...).

NOTE: Alternative 1 also does not prevent this from happening; however, users have the option of explicitly calling AddConstraint(x, vars) if they so choose.

Alternative 3

Be verbose / grammatically redundant.

(We already said we did not want this, but still want to show it in the context for others if they wish to chime in.)

AddConstraint(symbolic);
AddConstraint(CreateCost(...));
AddConstraint(CreateLinearConstraint(...));
Contributor

EricCousineau-TRI commented Apr 19, 2017

Sorry for the verbosity, but just to recap, I've listed out our three alternatives. (I needed this to have my own internal debate :P ) I will update the main post once I flesh it out a tad more.

  • Alternative 1 (Current Winner) - Yields some redundant and closely related options, but provides clarity when context may not be easily seen
  • Alternative 2 - Minimal redundancy, grammatically and semantically
  • Alternative 3 - Verbose but clear, grammatically redundant

TODO: Will need to consider options when using templates, such as being able to also specify Add(CreateCost<QuadraticCost>(symbolic)...) -> compile-time container placement.

Alternative 1

  • Add(...) only accepts a rigid set of non-symbolic overloads, such that it will error out if passed any symbolics directly.

  • Will accept symbolics filtered through Create${TYPE}(...)

  • Add${TYPE}(...) will forward directly to Add(Create${TYPE}(...))

  • Create${TYPE} will NOT use parameter packs to minimize any chance of ambiguity

      Add(x.transpose() * Q * x);
          // Compilation error
    
      Add(CreateCost(x.transpose() * Q * x));
          // Compiles fine. Run-time container determination.
    
      AddCost(x.transpose() * Q * x);
          // Compiles fine. Run-time container determination.
    
      Add(CreateQuadraticCost(x.transpose() * Q * x));
          // Compiles fine. Compile-time container determination.
    
      AddCost(CreateQuadraticCost(x.transpose() * Q * x));
          // Compiles fine. Compile-time container determination.
    

This means that there is redundancy between two easily-reachable methods, Add(Create${TYPE}(...)) and Add${TYPE}(...), which may yield more options than needed. However, this is the lesser of many evils.

Alternative 2

Instead of adding AddCost(...) and AddConstraint(...) with symbolic overloads, force the user to call Create${TYPE}(...) methods:

Add(CreateCost(symbolic)...)
Add(CreateConstraint(symbolic)...)

This makes things concise. The only drawback is, if you don't have a concise name, you may end up with something like:

class Stuff {
private:
    LinearConstraint x; // Poorly named constraint
};
...
prog.Add(x, vars); // Wait... Is this a cost or a constraint?

Granted, this is effectively the user's fault, but it also may be hard to clearly delineate overloads for Add(...).

NOTE: Alternative 1 also does not prevent this from happening; however, users have the option of explicitly calling AddConstraint(x, vars) if they so choose.

Alternative 3

Be verbose / grammatically redundant.

(We already said we did not want this, but still want to show it in the context for others if they wish to chime in.)

AddConstraint(symbolic);
AddConstraint(CreateCost(...));
AddConstraint(CreateLinearConstraint(...));
@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 20, 2017

Contributor

Updated description. Going to actually do it now.

Contributor

EricCousineau-TRI commented Apr 20, 2017

Updated description. Going to actually do it now.

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 24, 2017

Contributor

Will update PRs 1 and 2 by pulling some from this prototype branch:
master...5ca783cfiles_bucket

Contributor

EricCousineau-TRI commented Apr 24, 2017

Will update PRs 1 and 2 by pulling some from this prototype branch:
master...5ca783cfiles_bucket

EricCousineau-TRI added a commit to EricCousineau-TRI/drake that referenced this issue Apr 24, 2017

EricCousineau-TRI added a commit to EricCousineau-TRI/drake that referenced this issue Apr 24, 2017

EricCousineau-TRI added a commit to EricCousineau-TRI/drake that referenced this issue Apr 24, 2017

EricCousineau-TRI added a commit to EricCousineau-TRI/drake that referenced this issue Apr 25, 2017

EricCousineau-TRI added a commit to EricCousineau-TRI/drake that referenced this issue Apr 25, 2017

EricCousineau-TRI added a commit to EricCousineau-TRI/drake that referenced this issue Apr 25, 2017

EricCousineau-TRI added a commit to EricCousineau-TRI/drake that referenced this issue Apr 25, 2017

EricCousineau-TRI added a commit to EricCousineau-TRI/drake that referenced this issue Apr 25, 2017

EricCousineau-TRI added a commit to EricCousineau-TRI/drake that referenced this issue Apr 26, 2017

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI
Contributor

EricCousineau-TRI commented Apr 26, 2017

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI
Contributor

EricCousineau-TRI commented Apr 27, 2017

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 28, 2017

Contributor

Instead of adding a separate issue for cleaning up stopgap crap, I've added it as Step 7 in the above PR Plan.

Contributor

EricCousineau-TRI commented Apr 28, 2017

Instead of adding a separate issue for cleaning up stopgap crap, I've added it as Step 7 in the above PR Plan.

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 28, 2017

Contributor

Two questions for y'all, @soonho-tri @hongkai-dai:

Do y'all have a preference on the Create.*(Cost|Constraint) names and signatures? Once concern I have is the potential for ambiguity / inconsistency:

Return Types

Create.*(Cost|Constraint) will return a shared_ptr<C> if there is no binding information, otherwise will return Binding<C>.

  • PRO: This permits forms (a) prog.Add(CreateLinearConstraint(A, b, lb, ub), vars) and (b) prog.Add(CreateLinearCost(lb <= A * x <= ub)).
  • PRO: Add(...) and Add.*(Cost|Constraint) will always return a Binding<C>.
  • CON: See above.

Options:

  1. Keep things as planned.
    • PRO: Simple and easy.
    • CON: May lead to confusion.
  2. Enforce that Create... always return a Binding<C>, and extend the corresponding constructors.
    • PRO: Consistency, more work.
    • CON: Creation functions are explicitly coupled to bindings. You could not easily associate a Constraint/Cost with multiple distinct variable bindings.

My vote: (1) Keep things as planned.

Name

  • PRO: The names are currently Create.*, to support grammar like Add(CreateCost(...)).
  • CON: Since CreateCost(...) may create a shared_ptr, it would be more in-line with the rest of C++ to use something like MakeCost(...)
    • Had been done before, but has been removed by PR 2.
  • CON: As indicated above, the return-type may be different.

Options:

  1. Keep things as planned.
    • PRO: Easy.
    • PRO: Distinguishes that the return type is sensitive to the inputs provided.
  2. Change to Make.*(Cost|Constraint)
    • PRO: Analogous to STL.
    • CON (or PRO?): Would motivate that we keep as many creators as closed to shared_ptr as possible? Or could motivate that we make everything consistent.

My vote: (1) Keep things as planned.

Contributor

EricCousineau-TRI commented Apr 28, 2017

Two questions for y'all, @soonho-tri @hongkai-dai:

Do y'all have a preference on the Create.*(Cost|Constraint) names and signatures? Once concern I have is the potential for ambiguity / inconsistency:

Return Types

Create.*(Cost|Constraint) will return a shared_ptr<C> if there is no binding information, otherwise will return Binding<C>.

  • PRO: This permits forms (a) prog.Add(CreateLinearConstraint(A, b, lb, ub), vars) and (b) prog.Add(CreateLinearCost(lb <= A * x <= ub)).
  • PRO: Add(...) and Add.*(Cost|Constraint) will always return a Binding<C>.
  • CON: See above.

Options:

  1. Keep things as planned.
    • PRO: Simple and easy.
    • CON: May lead to confusion.
  2. Enforce that Create... always return a Binding<C>, and extend the corresponding constructors.
    • PRO: Consistency, more work.
    • CON: Creation functions are explicitly coupled to bindings. You could not easily associate a Constraint/Cost with multiple distinct variable bindings.

My vote: (1) Keep things as planned.

Name

  • PRO: The names are currently Create.*, to support grammar like Add(CreateCost(...)).
  • CON: Since CreateCost(...) may create a shared_ptr, it would be more in-line with the rest of C++ to use something like MakeCost(...)
    • Had been done before, but has been removed by PR 2.
  • CON: As indicated above, the return-type may be different.

Options:

  1. Keep things as planned.
    • PRO: Easy.
    • PRO: Distinguishes that the return type is sensitive to the inputs provided.
  2. Change to Make.*(Cost|Constraint)
    • PRO: Analogous to STL.
    • CON (or PRO?): Would motivate that we keep as many creators as closed to shared_ptr as possible? Or could motivate that we make everything consistent.

My vote: (1) Keep things as planned.

@soonho-tri

This comment has been minimized.

Show comment
Hide comment
@soonho-tri

soonho-tri Apr 28, 2017

Member

For the return types, I don't like (1) but I'm afraid that (1) is the only option we can choose.

For the names, I think (1) is good.

Member

soonho-tri commented Apr 28, 2017

For the return types, I don't like (1) but I'm afraid that (1) is the only option we can choose.

For the names, I think (1) is good.

@hongkai-dai

This comment has been minimized.

Show comment
Hide comment
@hongkai-dai

hongkai-dai Apr 28, 2017

Contributor

I think (1) is good. As long as MathematicalProgram::Add(xxx) always return a Binding<C> then I think we are good.

Contributor

hongkai-dai commented Apr 28, 2017

I think (1) is good. As long as MathematicalProgram::Add(xxx) always return a Binding<C> then I think we are good.

@RussTedrake

This comment has been minimized.

Show comment
Hide comment
@RussTedrake

RussTedrake Apr 29, 2017

Contributor

Sorry to be late to the party. Just saw the proposed PR.

My initial reactions:

  • the name MathematicalProgram::Add() , and the need for a "suggested best practices" feels clunky to me. can you summarize for me why we don't just have AddCost(Create.*Cost()) and AddConstraint(Create.*Constraint()). It's a little more typing, but if it removes the redundancy in the interface and the need for a best practices doc, then I think it's warranted. It would reduce your example above, with 5 potential call sites listed above to simply
  AddCost(x.transpose() * Q * x);
      // Compiles fine. Run-time container determination.

  AddCost(CreateQuadraticCost(x.transpose() * Q * x));
      // Compiles fine. Compile-time container determination.

presumably one could also do

  AddCost(CreateCost(x.transpose() * Q * x));
      // Compiles fine. Run-time container determination.

(but that is only allowed because we would like the ability to create costs/constraints outside of mathematical program... correct?)

  • I like the idea of cleaning up Create.*Constraint to only return Bindings. Agreed it's more work, and that your current proposal/PR is a perfect first step. But could a second PR do further cleaning? Is there any use case out there that needs to define a constraint independent of the binding to a variable? Does that even make sense?
Contributor

RussTedrake commented Apr 29, 2017

Sorry to be late to the party. Just saw the proposed PR.

My initial reactions:

  • the name MathematicalProgram::Add() , and the need for a "suggested best practices" feels clunky to me. can you summarize for me why we don't just have AddCost(Create.*Cost()) and AddConstraint(Create.*Constraint()). It's a little more typing, but if it removes the redundancy in the interface and the need for a best practices doc, then I think it's warranted. It would reduce your example above, with 5 potential call sites listed above to simply
  AddCost(x.transpose() * Q * x);
      // Compiles fine. Run-time container determination.

  AddCost(CreateQuadraticCost(x.transpose() * Q * x));
      // Compiles fine. Compile-time container determination.

presumably one could also do

  AddCost(CreateCost(x.transpose() * Q * x));
      // Compiles fine. Run-time container determination.

(but that is only allowed because we would like the ability to create costs/constraints outside of mathematical program... correct?)

  • I like the idea of cleaning up Create.*Constraint to only return Bindings. Agreed it's more work, and that your current proposal/PR is a perfect first step. But could a second PR do further cleaning? Is there any use case out there that needs to define a constraint independent of the binding to a variable? Does that even make sense?
@RussTedrake

This comment has been minimized.

Show comment
Hide comment
@RussTedrake

RussTedrake Apr 29, 2017

Contributor

Another deficit in the current best practices proposal (at the top) is that AddCost can only return a Binding with the general constraint type. So your suggested best practices could not use any derived class methods if the constraints were added with a symbolic type, e.g.

auto phi = prog.AddConstraint(A*x <= b)
...
phi->UpdateConstraint(newA,newb);

This might simply be an exception to your best practices, but it's worth considering.

Also, as implemented, I don't think prog.AddCost(x.transpose()*Q*x) will actually work, sinc e the type of the terms inside will be Eigen::Matrix<symbolic::Expression> not symbolic::Expression. I would actually like to implement this method, too (and have it in a WIP branch) as @soonho-tri and i did with https://github.com/RobotLocomotion/drake/blob/master/drake/systems/trajectory_optimization/direct_trajectory_optimization.h#L120 . The reason I haven't PR'd it is because the Eigen::Ref version gives some "ambiguous" errors, and the version templated on DerivedType is (for me) not as easy to push through the pybind11 interface.

Contributor

RussTedrake commented Apr 29, 2017

Another deficit in the current best practices proposal (at the top) is that AddCost can only return a Binding with the general constraint type. So your suggested best practices could not use any derived class methods if the constraints were added with a symbolic type, e.g.

auto phi = prog.AddConstraint(A*x <= b)
...
phi->UpdateConstraint(newA,newb);

This might simply be an exception to your best practices, but it's worth considering.

Also, as implemented, I don't think prog.AddCost(x.transpose()*Q*x) will actually work, sinc e the type of the terms inside will be Eigen::Matrix<symbolic::Expression> not symbolic::Expression. I would actually like to implement this method, too (and have it in a WIP branch) as @soonho-tri and i did with https://github.com/RobotLocomotion/drake/blob/master/drake/systems/trajectory_optimization/direct_trajectory_optimization.h#L120 . The reason I haven't PR'd it is because the Eigen::Ref version gives some "ambiguous" errors, and the version templated on DerivedType is (for me) not as easy to push through the pybind11 interface.

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 30, 2017

Contributor

Glad to have your feedback!

BLOT, my takeaway action items:

  1. Update the suggested interface, remove Add()
  2. Simplify / remove best practices
  3. Add examples of explicit type propagation
  4. Add cast<> or downcast<> or dynamic_cast<> method to Binding directly.
  5. Update to consistently return Binding<C>. Update symbolic creator functions to return pair<ptr<C>, vars>, to maintain decoupling.
  6. Reduce syntactic-sugar overloads for VectorXDecisionVariable - May be useful, but no longer immediately necessary.
  7. Continue with PR plan things.

EDIT: Changed, per my next comment in this thread.

Verbose discussion stuff: (I kinda wish GitHub had a collapse-like functionality for extra stuff like this...)

Add vs Add(Cost|Constraint)

The main intent was to avoid redundancy in the words, but since there is more priority to get rid of redundant options, then I will reduce it to just AddCost and AddConstraint, and permit the redundant words to clean the interface.

Options to Downcast for Add.* methods with run-time container determination

In this case, if a user has foreknowledge on type to be added, then I believe the options are:

  1. Use the explicit form of Create.*(Cost|Constraint):

     auto phi = prog.AddConstraint(CreateLinearConstraint(A*x <= b));
     phi.constraint()->UpdateConstraint(newA,newb);
    
  2. Perform an explicit downcast, as done for run-time determination here:

         auto phi = BindingDowncast<LinearConstraint>(prog.AddConstraint(A*x <= b));
         phi.constraint()->UpdateConstraint(newA,newb);
    
    • Just realized that I mixed up downcast and upcast, thinking of the tree in reverse order - will incorporate that in the cleanup PR step...

I personally prefer Option 1, as this would also avoid the need to do any run-time checks, as the user has already specified the constraint type.

However, some form of Option 2 could be used if the type may changed at run-time:

    Binding<Constraint> phi_;
    ...        
    phi_ = prog.AddConstraint(A*x <= b);
    ...
    // This RTTI check is dirty... Some other conditional should probably be used
    if (phi_.isa<LinearConstraint>()) {
        // This is less dirty, and we can ensure safety by asserting for non-nullness
        auto linear = phi_.cast<LinearConstraint>();
    } else if (phi_.isa<QuadraticConstraint>()) {
        ...
    }

Consistency of returning Binding objects

EDIT: See next comment below in thread for new opinion.
Old opinion:
I can consolidate those interfaces. The main concern is that Binding objects also have the two and possibly more overloads if / when we wish to provide the ability to name the bindings, and this would be duplicated per Create... method.

I have not yet sifted through all use cases to create Constraint or Cost classes that are used in multiple Bindings, but given the immutability of these objects, I would like to keep that option open.

This could be done by adding another method, Make.*(Cost|Constraint)Ptr, which only returns a shared_ptr.
However, I will add this as optional for the time being. If this becomes too cumbersome, I can add it as a low-priority or backlog issue.

On that token, I will consider providing a (potentially copy-constructable) initializer to handle any VectorXDecisionVariable variants (such as std::intializer_list). I am thinking of doing something such as this (but more elegantly):

This would reduce overloads to just one type. I mentioned this to @soonho-tri, and he somewhat likes the idea, but is concerned about permitting recursive initializer lists. I believe I have an idea for preventing this, and will pursue it.

EDIT 2: Sorry I forgot to capture your earlier comment about 1x1 MatrixBase<> values! I will add the overload form that you and Soonho had.

Contributor

EricCousineau-TRI commented Apr 30, 2017

Glad to have your feedback!

BLOT, my takeaway action items:

  1. Update the suggested interface, remove Add()
  2. Simplify / remove best practices
  3. Add examples of explicit type propagation
  4. Add cast<> or downcast<> or dynamic_cast<> method to Binding directly.
  5. Update to consistently return Binding<C>. Update symbolic creator functions to return pair<ptr<C>, vars>, to maintain decoupling.
  6. Reduce syntactic-sugar overloads for VectorXDecisionVariable - May be useful, but no longer immediately necessary.
  7. Continue with PR plan things.

EDIT: Changed, per my next comment in this thread.

Verbose discussion stuff: (I kinda wish GitHub had a collapse-like functionality for extra stuff like this...)

Add vs Add(Cost|Constraint)

The main intent was to avoid redundancy in the words, but since there is more priority to get rid of redundant options, then I will reduce it to just AddCost and AddConstraint, and permit the redundant words to clean the interface.

Options to Downcast for Add.* methods with run-time container determination

In this case, if a user has foreknowledge on type to be added, then I believe the options are:

  1. Use the explicit form of Create.*(Cost|Constraint):

     auto phi = prog.AddConstraint(CreateLinearConstraint(A*x <= b));
     phi.constraint()->UpdateConstraint(newA,newb);
    
  2. Perform an explicit downcast, as done for run-time determination here:

         auto phi = BindingDowncast<LinearConstraint>(prog.AddConstraint(A*x <= b));
         phi.constraint()->UpdateConstraint(newA,newb);
    
    • Just realized that I mixed up downcast and upcast, thinking of the tree in reverse order - will incorporate that in the cleanup PR step...

I personally prefer Option 1, as this would also avoid the need to do any run-time checks, as the user has already specified the constraint type.

However, some form of Option 2 could be used if the type may changed at run-time:

    Binding<Constraint> phi_;
    ...        
    phi_ = prog.AddConstraint(A*x <= b);
    ...
    // This RTTI check is dirty... Some other conditional should probably be used
    if (phi_.isa<LinearConstraint>()) {
        // This is less dirty, and we can ensure safety by asserting for non-nullness
        auto linear = phi_.cast<LinearConstraint>();
    } else if (phi_.isa<QuadraticConstraint>()) {
        ...
    }

Consistency of returning Binding objects

EDIT: See next comment below in thread for new opinion.
Old opinion:
I can consolidate those interfaces. The main concern is that Binding objects also have the two and possibly more overloads if / when we wish to provide the ability to name the bindings, and this would be duplicated per Create... method.

I have not yet sifted through all use cases to create Constraint or Cost classes that are used in multiple Bindings, but given the immutability of these objects, I would like to keep that option open.

This could be done by adding another method, Make.*(Cost|Constraint)Ptr, which only returns a shared_ptr.
However, I will add this as optional for the time being. If this becomes too cumbersome, I can add it as a low-priority or backlog issue.

On that token, I will consider providing a (potentially copy-constructable) initializer to handle any VectorXDecisionVariable variants (such as std::intializer_list). I am thinking of doing something such as this (but more elegantly):

This would reduce overloads to just one type. I mentioned this to @soonho-tri, and he somewhat likes the idea, but is concerned about permitting recursive initializer lists. I believe I have an idea for preventing this, and will pursue it.

EDIT 2: Sorry I forgot to capture your earlier comment about 1x1 MatrixBase<> values! I will add the overload form that you and Soonho had.

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 30, 2017

Contributor

On the Consistency on returning Binding objects, I believe I may change my opinion on this. I am still generally uncomfortable with the increased coupling between Binding and Create.*(Cost|Constraint), and I feel that requiring these functions to return Bindings may artificially require more information than needed, especially if a Binding may mean more than just constraint and variables (e.g., context within a MathematicalProgram, such as its name).

This issue was initially meant to minimize that coupling, where before, if / when the Binding constructor is updated, we would have to update each individual Add.*(Cost|Constraint) method to reflect this, on top of all of the other changes that would need to be made. (Though the title initially states that it would remove the coupling from the MathematicalProgram, I believe the priority should be given to minimizing the coupling between the most number of moving parts, which in this case are the Constraints / Costs.)

If I enforce that each Create.*(Cost|Constraint) method enforce this, then that reintroduces the per-constraint/cost implementation coupling.
I'd like to possibly use template parameter packs, but (a) those can't transitively bind initializer_lists used for syntactic sugar, and (b) would have to be used everywhere, forcing all of those interface implementations to be available in the header file.

I would like to minimize as much input, even at the risk of inconsistent return types (which only occurs due to the fact that symbolic expressions have more information embedded in them). To this end, I would suggest that instead we have the following return types for Create.*(Cost|Constraint):

  • Numeric -> std::shared_ptr<C>
  • Symbolic -> std::pair< std::shared_ptr<C>, VectorXDecisionVariable >.

This does have inconsistent return types, but they are not immediately usable as it would be with a Binding, so it would incentivize users to either (a) always pipe it to MathematicalProgram::Add.*(Cost|Constraint) to get the correct Binding (consolidating the Binding constructor coupling), or (b) know that they are getting additional information given how they formulate their constraints.

This also (most importantly) completely decouples constraint creation from Binding constructors, such that they can be changed.

For now, I will update my PR Plan to reflect this. However, if you still feel strongly that they should have a consistent return type, please let me know.

Contributor

EricCousineau-TRI commented Apr 30, 2017

On the Consistency on returning Binding objects, I believe I may change my opinion on this. I am still generally uncomfortable with the increased coupling between Binding and Create.*(Cost|Constraint), and I feel that requiring these functions to return Bindings may artificially require more information than needed, especially if a Binding may mean more than just constraint and variables (e.g., context within a MathematicalProgram, such as its name).

This issue was initially meant to minimize that coupling, where before, if / when the Binding constructor is updated, we would have to update each individual Add.*(Cost|Constraint) method to reflect this, on top of all of the other changes that would need to be made. (Though the title initially states that it would remove the coupling from the MathematicalProgram, I believe the priority should be given to minimizing the coupling between the most number of moving parts, which in this case are the Constraints / Costs.)

If I enforce that each Create.*(Cost|Constraint) method enforce this, then that reintroduces the per-constraint/cost implementation coupling.
I'd like to possibly use template parameter packs, but (a) those can't transitively bind initializer_lists used for syntactic sugar, and (b) would have to be used everywhere, forcing all of those interface implementations to be available in the header file.

I would like to minimize as much input, even at the risk of inconsistent return types (which only occurs due to the fact that symbolic expressions have more information embedded in them). To this end, I would suggest that instead we have the following return types for Create.*(Cost|Constraint):

  • Numeric -> std::shared_ptr<C>
  • Symbolic -> std::pair< std::shared_ptr<C>, VectorXDecisionVariable >.

This does have inconsistent return types, but they are not immediately usable as it would be with a Binding, so it would incentivize users to either (a) always pipe it to MathematicalProgram::Add.*(Cost|Constraint) to get the correct Binding (consolidating the Binding constructor coupling), or (b) know that they are getting additional information given how they formulate their constraints.

This also (most importantly) completely decouples constraint creation from Binding constructors, such that they can be changed.

For now, I will update my PR Plan to reflect this. However, if you still feel strongly that they should have a consistent return type, please let me know.

@RussTedrake

This comment has been minimized.

Show comment
Hide comment
@RussTedrake

RussTedrake Apr 30, 2017

Contributor

RE: Add vs Add[Cost|Constraint]. I'm happy with using just Add[Cost|Constraint], and removing Add. I think all calls would be unambiguous without the extra characters, but feel that this just adds clarity. Do @soonho-tri or @hongkai-dai agree?

Re: 1x1 MatrixBase values. Thanks. I think that's an obvious and helpful addition. Again, ideally I would like it to work for the python bindings, too.

Re: Derived constraint classes and returning Bindings. Perhaps we need to flesh out the connection between Systems and Constraints (the other major use case) to really decide this? My thoughts:

I think we're agreed we want at least

Binding<Cost> MathematicalProgram::AddCost(symbolicExpression&)
Binding<Constraint> MathematicalProgram::AddConstraint(symbolicFormula&)

as the simplest interface to create the (correct type of) constraint, bind it to the variables, add it to the program, and return a Binding. We need to support users adding constraints manually (no symbolic) for derived constraint classes, and support getting derived class pointers from the symbolic expression pathways, with a more streamlined interface that we have currently. And we want to support binding constraints to variables outside of mathematical program (e.g. to elements of a context for system constraints).

I'm not actually sure that we need to make new factories to accomplish this (e.g. your Create* methods). We have a few class types floating around, and their constructors should do what we want?

  1. Constraint, LinearConstraint, BoundingBoxConstraint, ...
  2. DecisionVariable, Context, ...
  3. Binding<Constraint>, Binding<LinearConstraint>, ...
  4. MathematicalProgram, System, ...

The thing that I am unclear on right now is whether we will instantiate a context templated on DecisionVariables, or somehow Bind a constraint to the context more directly?

But for the mathematicalprogram use case, it seems like implementing, e.g.

MathematicalProgram::AddConstraint(Binding<LinearConstraint, DecisionVariable>)

and possibly also (as sugar)

Binding<Constraint> AddConstraint(shared_ptr<LinearConstraint>, DecisionVariable)

accomplishes the non-symbolic part of that. Updating constraints could then be accomplished by e.g.

auto phi = make_shared<LinearConstraint>(A,b);
prog->AddConstraint(phi,x);
phi->UpdateConstraint(newA,newb);

I also wouldn't worry about making explicit downcast mechanisms yet, as long as this approach is not explicitly discouraged by some best-practices/style guide. We can always add more sugar later.

Finally, for getting derived constraint classes from symbolic, it seems like a symbolic[Expression|Formula] exists fundamentally at the Binding level. So we just need methods to get Binding<LinearConstraint,DecisionVariable> from the symbolicFormula. A Binding is essentially a std::pair<Contraint, DecisionVariable>, so I wouldn't add that extra type flying around?

Contributor

RussTedrake commented Apr 30, 2017

RE: Add vs Add[Cost|Constraint]. I'm happy with using just Add[Cost|Constraint], and removing Add. I think all calls would be unambiguous without the extra characters, but feel that this just adds clarity. Do @soonho-tri or @hongkai-dai agree?

Re: 1x1 MatrixBase values. Thanks. I think that's an obvious and helpful addition. Again, ideally I would like it to work for the python bindings, too.

Re: Derived constraint classes and returning Bindings. Perhaps we need to flesh out the connection between Systems and Constraints (the other major use case) to really decide this? My thoughts:

I think we're agreed we want at least

Binding<Cost> MathematicalProgram::AddCost(symbolicExpression&)
Binding<Constraint> MathematicalProgram::AddConstraint(symbolicFormula&)

as the simplest interface to create the (correct type of) constraint, bind it to the variables, add it to the program, and return a Binding. We need to support users adding constraints manually (no symbolic) for derived constraint classes, and support getting derived class pointers from the symbolic expression pathways, with a more streamlined interface that we have currently. And we want to support binding constraints to variables outside of mathematical program (e.g. to elements of a context for system constraints).

I'm not actually sure that we need to make new factories to accomplish this (e.g. your Create* methods). We have a few class types floating around, and their constructors should do what we want?

  1. Constraint, LinearConstraint, BoundingBoxConstraint, ...
  2. DecisionVariable, Context, ...
  3. Binding<Constraint>, Binding<LinearConstraint>, ...
  4. MathematicalProgram, System, ...

The thing that I am unclear on right now is whether we will instantiate a context templated on DecisionVariables, or somehow Bind a constraint to the context more directly?

But for the mathematicalprogram use case, it seems like implementing, e.g.

MathematicalProgram::AddConstraint(Binding<LinearConstraint, DecisionVariable>)

and possibly also (as sugar)

Binding<Constraint> AddConstraint(shared_ptr<LinearConstraint>, DecisionVariable)

accomplishes the non-symbolic part of that. Updating constraints could then be accomplished by e.g.

auto phi = make_shared<LinearConstraint>(A,b);
prog->AddConstraint(phi,x);
phi->UpdateConstraint(newA,newb);

I also wouldn't worry about making explicit downcast mechanisms yet, as long as this approach is not explicitly discouraged by some best-practices/style guide. We can always add more sugar later.

Finally, for getting derived constraint classes from symbolic, it seems like a symbolic[Expression|Formula] exists fundamentally at the Binding level. So we just need methods to get Binding<LinearConstraint,DecisionVariable> from the symbolicFormula. A Binding is essentially a std::pair<Contraint, DecisionVariable>, so I wouldn't add that extra type flying around?

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI Apr 30, 2017

Contributor

Re^2: Derived constraint classes and returning Bindings

Perhaps we need to flesh out the connection between Systems and Constraints (the other major use case) to really decide this?

I had only only spoken with @soonho-tri a bit about this, at the level that he had intended to separate out the symbolic parsing functionality (Soonho, please correct me if I am wrong). From this, I wanted to see if I can subsume at least a portion of that into this refactor. My assumption was the System framework may consume Bindings produced by the symbolic parsing, but I could definitely be wrong in this part.

I'll start reviewing a bit of #5297 (physical/non-physical state constraints) and #4998 (MTL / STL).
Are there any other discussions / design docs that would be pertinent?

I'm not actually sure that we need to make new factories to accomplish this (e.g. your Create* methods). We have a few class types floating around, and their constructors should do what we want?

This is true when the factory method has a direct 1:1 with the type's constructor. However, it is most useful in the case of (a) symbolic variables (as you mentioned), and somewhat useful for the case of (b) different variants of the same type (e.g., (1) AddLinearConstraint(Vector, double, double), (2) AddL2NormCost, etc.), which means, at present, the constructors do not do everything we want.

Example of extracted factory function: CreateL2NormCost (WIP)

Part (1) of (b) could be handled by constructor overloads, and we could move those creators there. For part (2) of (b), constructions that may need different names (L2NormCost, QuadraticErrorCost, ...), we could consider making those either (i) derived from the base type, possibly with more overloads (easiest, most succinct), or (ii) static factory methods of the respective constraint (QuadraticCost::L2Norm(...)).

For (b.2), I prefer a derived type, option (i).

Finally, for getting derived constraint classes from symbolic, it seems like a symbolic[Expression|Formula] exists fundamentally at the Binding level.

If the Create.*[Cost|Constraint] methods could be eliminated for situation (b), then only (a) remains, where we would then only be returning a constraint bound to its variables, which is what you mentioned above.

This would cause the C++ and Python APIs to deviate just a little. In C++, you would have:

prog_.AddCost(make_shared<QuadraticErrorCost>(Q_, x_desired_), vars_);
system_.AddConstraint(make_shared<LinearConstraint>(A_, b_), vars_);

while in Python / MATLAB:

prog.AddCost(QuadraticErrorCost(Q, x_desired), vars);
system.AddConstraint(LinearConstraint(A, b), vars);

which does seem acceptable. However, this would be the only way to add numeric constraints that are not already bound. I can't say I like this end-result interface more than the other, but it does clean up implementation and make the interface more consistent.

Would you like me to pursue this route, then?

A Binding is essentially a std::pair<Contraint, DecisionVariable>, so I wouldn't add that extra type flying around?

True, but my fear is that we may have a desire to further overload whatever the symbolic factory functions are to return a "complete" Binding if we add an identifier such as a name (per #5827).
However, I do plan to accept "incomplete" Bindings (those without names), and assign the name when the binding is added, so the name could be assigned by a signature such as:

AddCost(Binding<Cost>, string name = "")

In this case, I may consider throwing an error if the name is specified, but the Binding already has a non-default name, etc., which is doable.

Given this, I will avoid introducing the new type, pair< shared_ptr<C>, Vars > for now.

Contributor

EricCousineau-TRI commented Apr 30, 2017

Re^2: Derived constraint classes and returning Bindings

Perhaps we need to flesh out the connection between Systems and Constraints (the other major use case) to really decide this?

I had only only spoken with @soonho-tri a bit about this, at the level that he had intended to separate out the symbolic parsing functionality (Soonho, please correct me if I am wrong). From this, I wanted to see if I can subsume at least a portion of that into this refactor. My assumption was the System framework may consume Bindings produced by the symbolic parsing, but I could definitely be wrong in this part.

I'll start reviewing a bit of #5297 (physical/non-physical state constraints) and #4998 (MTL / STL).
Are there any other discussions / design docs that would be pertinent?

I'm not actually sure that we need to make new factories to accomplish this (e.g. your Create* methods). We have a few class types floating around, and their constructors should do what we want?

This is true when the factory method has a direct 1:1 with the type's constructor. However, it is most useful in the case of (a) symbolic variables (as you mentioned), and somewhat useful for the case of (b) different variants of the same type (e.g., (1) AddLinearConstraint(Vector, double, double), (2) AddL2NormCost, etc.), which means, at present, the constructors do not do everything we want.

Example of extracted factory function: CreateL2NormCost (WIP)

Part (1) of (b) could be handled by constructor overloads, and we could move those creators there. For part (2) of (b), constructions that may need different names (L2NormCost, QuadraticErrorCost, ...), we could consider making those either (i) derived from the base type, possibly with more overloads (easiest, most succinct), or (ii) static factory methods of the respective constraint (QuadraticCost::L2Norm(...)).

For (b.2), I prefer a derived type, option (i).

Finally, for getting derived constraint classes from symbolic, it seems like a symbolic[Expression|Formula] exists fundamentally at the Binding level.

If the Create.*[Cost|Constraint] methods could be eliminated for situation (b), then only (a) remains, where we would then only be returning a constraint bound to its variables, which is what you mentioned above.

This would cause the C++ and Python APIs to deviate just a little. In C++, you would have:

prog_.AddCost(make_shared<QuadraticErrorCost>(Q_, x_desired_), vars_);
system_.AddConstraint(make_shared<LinearConstraint>(A_, b_), vars_);

while in Python / MATLAB:

prog.AddCost(QuadraticErrorCost(Q, x_desired), vars);
system.AddConstraint(LinearConstraint(A, b), vars);

which does seem acceptable. However, this would be the only way to add numeric constraints that are not already bound. I can't say I like this end-result interface more than the other, but it does clean up implementation and make the interface more consistent.

Would you like me to pursue this route, then?

A Binding is essentially a std::pair<Contraint, DecisionVariable>, so I wouldn't add that extra type flying around?

True, but my fear is that we may have a desire to further overload whatever the symbolic factory functions are to return a "complete" Binding if we add an identifier such as a name (per #5827).
However, I do plan to accept "incomplete" Bindings (those without names), and assign the name when the binding is added, so the name could be assigned by a signature such as:

AddCost(Binding<Cost>, string name = "")

In this case, I may consider throwing an error if the name is specified, but the Binding already has a non-default name, etc., which is doable.

Given this, I will avoid introducing the new type, pair< shared_ptr<C>, Vars > for now.

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI May 1, 2017

Contributor

Per our discussion at the meeting, these are my takeaways:
(please let me know if there was anything that I did not capture accurately.)

  • At present, there will be no other consumers of Binding aside from MathematicalProgram
    • It is fine to keep this type, and the factory methods, custom-tailored for or contained in MathematicalProgram.
  • If a user knows what functional form of a symbolic expression they have, they may specify this with Binding<${TYPE}> AddCost<${TYPE}>(const ScalarOrMatrix<Expression>& e) (or AddConstraint)
  • There will be no methods of the form shared_ptr<C> Create.*[Cost|Constraint](...). The user should call make_shared<...>(...) themselves.
    • There may be internal implementations for Create.*[Cost|Constraint](const ScalarOrMatrix<symbolic::Expression>&), which would be called if the template-specialized AddCost<C>(const ScalarOrMatrix<symbolic::Expression>&) (or AddConstraint) is called.

I will go ahead and update the above description, example usages, and best practices.

\cc @RussTedrake @soonho-tri @hongkai-dai @jadecastro

Contributor

EricCousineau-TRI commented May 1, 2017

Per our discussion at the meeting, these are my takeaways:
(please let me know if there was anything that I did not capture accurately.)

  • At present, there will be no other consumers of Binding aside from MathematicalProgram
    • It is fine to keep this type, and the factory methods, custom-tailored for or contained in MathematicalProgram.
  • If a user knows what functional form of a symbolic expression they have, they may specify this with Binding<${TYPE}> AddCost<${TYPE}>(const ScalarOrMatrix<Expression>& e) (or AddConstraint)
  • There will be no methods of the form shared_ptr<C> Create.*[Cost|Constraint](...). The user should call make_shared<...>(...) themselves.
    • There may be internal implementations for Create.*[Cost|Constraint](const ScalarOrMatrix<symbolic::Expression>&), which would be called if the template-specialized AddCost<C>(const ScalarOrMatrix<symbolic::Expression>&) (or AddConstraint) is called.

I will go ahead and update the above description, example usages, and best practices.

\cc @RussTedrake @soonho-tri @hongkai-dai @jadecastro

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI May 1, 2017

Contributor

Per a f2f with @soonho-tri, I plan to implement specialized types (e.g., L2Norm, QuadraticError) as types derived the base type, to enable a syntax such as:

prog_.AddCost(make_shared<L2NormCost>(A_, b_), vars_);
Contributor

EricCousineau-TRI commented May 1, 2017

Per a f2f with @soonho-tri, I plan to implement specialized types (e.g., L2Norm, QuadraticError) as types derived the base type, to enable a syntax such as:

prog_.AddCost(make_shared<L2NormCost>(A_, b_), vars_);
@hongkai-dai

This comment has been minimized.

Show comment
Hide comment
@hongkai-dai

hongkai-dai May 1, 2017

Contributor

Is L2NormCost a derived class of QuadraticCost?

Contributor

hongkai-dai commented May 1, 2017

Is L2NormCost a derived class of QuadraticCost?

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI May 1, 2017

Contributor

Yup, such that it would be placed in the correct container.

The only caveat is that A_ and b_ would still imply the original QuadraticCost's members. This could be fixed by providing a shadowing (non-virtual) overload of UpdateConstraint().

What do you think?

Caveat: This is fine when the signature of UpdateConstraint(...) is coincident between the base and the derived class (which is the case with QuadraticError and L2Norm with QuadraticCost, but may not always be the case)...

Contributor

EricCousineau-TRI commented May 1, 2017

Yup, such that it would be placed in the correct container.

The only caveat is that A_ and b_ would still imply the original QuadraticCost's members. This could be fixed by providing a shadowing (non-virtual) overload of UpdateConstraint().

What do you think?

Caveat: This is fine when the signature of UpdateConstraint(...) is coincident between the base and the derived class (which is the case with QuadraticError and L2Norm with QuadraticCost, but may not always be the case)...

@hongkai-dai

This comment has been minimized.

Show comment
Hide comment
@hongkai-dai

hongkai-dai May 1, 2017

Contributor

My concern is that we can have an explosion of derived class, if we go this way, as we can further have other derived class of QuadraticCost, for example, WeightedL2NormCost. L2NormCost is just a special form form of quadratic cost 0.5 * x' * Q * x + b' * x + c, where Q = Identity, b = 0, c = 0. Does this concern make sense?

Contributor

hongkai-dai commented May 1, 2017

My concern is that we can have an explosion of derived class, if we go this way, as we can further have other derived class of QuadraticCost, for example, WeightedL2NormCost. L2NormCost is just a special form form of quadratic cost 0.5 * x' * Q * x + b' * x + c, where Q = Identity, b = 0, c = 0. Does this concern make sense?

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI May 1, 2017

Contributor

Yes, that is most certainly a valid concern. In that case, maybe a static method could be used, such as:

class QuadraticCost {
...
    static shared_ptr<QuadraticCost> MakeError(...);
    static shared_ptr<QuadraticCost> MakeL2Norm(...);
};

// Example
prog_.AddCost(make_shared<QuadraticCost>(...), vars_);
prog_.AddCost(QuadraticCost::L2Norm(...), vars_);

Alternatively, this could be made into external functions, such as MakeQuadraticErrorCost(...) and MakeL2NormCost(...), but this would circle things back to the original design, and have a little bit if incongruence in that only special types have the Make...Cost(...) functional form.

Contributor

EricCousineau-TRI commented May 1, 2017

Yes, that is most certainly a valid concern. In that case, maybe a static method could be used, such as:

class QuadraticCost {
...
    static shared_ptr<QuadraticCost> MakeError(...);
    static shared_ptr<QuadraticCost> MakeL2Norm(...);
};

// Example
prog_.AddCost(make_shared<QuadraticCost>(...), vars_);
prog_.AddCost(QuadraticCost::L2Norm(...), vars_);

Alternatively, this could be made into external functions, such as MakeQuadraticErrorCost(...) and MakeL2NormCost(...), but this would circle things back to the original design, and have a little bit if incongruence in that only special types have the Make...Cost(...) functional form.

@hongkai-dai

This comment has been minimized.

Show comment
Hide comment
@hongkai-dai

hongkai-dai May 1, 2017

Contributor

I think both these two designs (static method and external functions) are good, that they are just some functions to construct a QuadraticCost in a special form.

Contributor

hongkai-dai commented May 1, 2017

I think both these two designs (static method and external functions) are good, that they are just some functions to construct a QuadraticCost in a special form.

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI May 1, 2017

Contributor

OK. To keep things somewhat decoupled (and keep the class definition less noisy), I will create separate Make.*[Cost|Constraint]() methods, possibly near the original classes. This would foster the ability to keep the syntax consistent if a user wanted to introduce an additional method or overload in their own context.

Contributor

EricCousineau-TRI commented May 1, 2017

OK. To keep things somewhat decoupled (and keep the class definition less noisy), I will create separate Make.*[Cost|Constraint]() methods, possibly near the original classes. This would foster the ability to keep the syntax consistent if a user wanted to introduce an additional method or overload in their own context.

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI May 1, 2017

Contributor

I've justed updated the description. Please let me know if you have any comments / concerns.

Contributor

EricCousineau-TRI commented May 1, 2017

I've justed updated the description. Please let me know if you have any comments / concerns.

@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI May 1, 2017

Contributor

Did a quick test, template specialized overloads (e.g. AddCost(symbolic...), AddCost<C>(symbolic...) work as expected:

Output snippet:

>>> c.AddConstraint(e_a).GetName()
compile-time { Binding<Constraint> }, run-time { Binding<AConstraint> }

>>> c.AddConstraint<AConstraint>(e_a).GetName()
compile-time { Binding<AConstraint> }, run-time { Binding<AConstraint> }
Contributor

EricCousineau-TRI commented May 1, 2017

Did a quick test, template specialized overloads (e.g. AddCost(symbolic...), AddCost<C>(symbolic...) work as expected:

Output snippet:

>>> c.AddConstraint(e_a).GetName()
compile-time { Binding<Constraint> }, run-time { Binding<AConstraint> }

>>> c.AddConstraint<AConstraint>(e_a).GetName()
compile-time { Binding<AConstraint> }, run-time { Binding<AConstraint> }
@EricCousineau-TRI

This comment has been minimized.

Show comment
Hide comment
@EricCousineau-TRI

EricCousineau-TRI May 12, 2017

Contributor

Updated description per f2f conversation yesterday.

Contributor

EricCousineau-TRI commented May 12, 2017

Updated description per f2f conversation yesterday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment