Add python binding for branch_and_bound #13002

hongkai-dai · 2020-04-05T04:08:43Z

This change is

hongkai-dai

+@EricCousineau-TRI for feature review please, thanks!

Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

EricCousineau-TRI

First pass done, PTAL.

Reviewed 5 of 5 files at r1.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @hongkai-dai)

a discussion (no related file):
Can you add this important into pydrake.solvers.all?
https://github.com/RobotLocomotion/drake/blob/92a2bea5f7c116608f9217da2de72135b305f860/bindings/pydrake/solvers/all.py

And then can you test it here?

drake/bindings/pydrake/test/all_test.py

Line 140 in 92a2bea

# solvers

bindings/pydrake/solvers/branch_and_bound_py.cc, line 20 at r1 (raw file):

  py::module::import("pydrake.solvers.mathematicalprogram");

  py::class_<MixedIntegerBranchAndBound>(

nit Consider using the using Class = ...; constexpr auto cls_doc = ... convention mentioned here:

drake/bindings/pydrake/pydrake_doxygen.h

Lines 202 to 213 in 92a2bea

    
           @note Consider using scoped aliases to abbreviate both the usage of bound types 
        
           and the docstring structures. Borrowing from above: 
        
           ~~~{.cc} 
        
               { 
        
                 using Class = RigidTransform<T>; 
        
                 constexpr auto& cls_doc = doc.RigidTransform; 
        
                 py::class_<Class>(m, "RigidTransform", cls_doc.doc) 
        
                     .def(py::init(), cls_doc.ctor.doc_0args) 
        
                     ... 
        
               } 
        
           ~~~

bindings/pydrake/solvers/branch_and_bound_py.cc, line 22 at r1 (raw file):

  py::class_<MixedIntegerBranchAndBound>(
      m, "MixedIntegerBranchAndBound", doc.MixedIntegerBranchAndBound.doc)
      .def(py::init(

nit Can I ask why py::init<const MathematicalProgram&, const SolverId&>() does not work here?

bindings/pydrake/solvers/branch_and_bound_py.cc, line 52 at r1 (raw file):

            return self.GetSolution(mip_vars, nth_best_solution);
          },
          py::arg("mip_var"), py::arg("nth_best_solution") = 0,

nit Should this be mip_vars? (mip_var is used for the arg, which seems inconsistent?)

bindings/pydrake/solvers/branch_and_bound_py.cc, line 60 at r1 (raw file):

            return self.GetSolution(mip_vars, nth_best_solution);
          },
          py::arg("mip_var"), py::arg("nth_best_solution") = 0,

nit Should this be mip_vars? (mip_var is used for the arg, which seems inconsistent?)

hongkai-dai

Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you add this important into pydrake.solvers.all?
https://github.com/RobotLocomotion/drake/blob/92a2bea5f7c116608f9217da2de72135b305f860/bindings/pydrake/solvers/all.py

And then can you test it here?

drake/bindings/pydrake/test/all_test.py

Line 140 in 92a2bea

# solvers

Done.

bindings/pydrake/solvers/branch_and_bound_py.cc, line 22 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can I ask why py::init<const MathematicalProgram&, const SolverId&>() does not work here?

Done. I didn't know we can add python binding for constructor in this way. Thanks!

hongkai-dai

Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Done.

BTW, do I need to add a unit test for all.py? I could do

import pydrake.solvers.all
x = pydrake.solvers.all.MixedIntegerBranchAndBound

on my python console.

EricCousineau-TRI

Reviewed 2 of 2 files at r2.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Done.

Can you also have it be tested in all_test.py, linked in prior comment?

bindings/pydrake/solvers/branch_and_bound_py.cc, line 22 at r1 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Done. I didn't know we can add python binding for constructor in this way. Thanks!

OK Welcome! It's actually shown this way in the dev docs, and on pybind's website too ;)
https://drake.mit.edu/doxygen_cxx/group__python__bindings.html
https://pybind11.readthedocs.io/en/stable/classes.html#creating-bindings-for-a-custom-type

EricCousineau-TRI

Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @hongkai-dai)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you also have it be tested in all_test.py, linked in prior comment?

Ah, seems like we posted at around the same time - yeah, see the all_test.py code.

hongkai-dai

Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Ah, seems like we posted at around the same time - yeah, see the all_test.py code.

Got it, just added the test.

EricCousineau-TRI

Reviewed 1 of 1 files at r3.
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

EricCousineau-TRI

+@sammy-tri for platform review, please.

Reviewable status: LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sammy-tri)

EricCousineau-TRI

Reviewable status: LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sammy-tri)

bindings/pydrake/test/all_test.py, line 147 at r3 (raw file):

            # - ipopt
            "IpoptSolver",
            # - branch_and_bound

BTW This isn't alphabetical, but it's not particularly a huge deal.

sammy-tri

Reviewed 4 of 5 files at r1, 2 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

sammy-tri

+(status: squashing now)

Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),EricCousineau-TRI(platform)

hongkai-dai force-pushed the pybind_branch_and_bound branch 2 times, most recently from a07e36f to d0b72aa Compare April 5, 2020 04:14

Add python binding for branch_and_bound

4c48282

hongkai-dai force-pushed the pybind_branch_and_bound branch from d0b72aa to 4c48282 Compare April 5, 2020 04:43

hongkai-dai assigned EricCousineau-TRI Apr 5, 2020

hongkai-dai commented Apr 5, 2020

View reviewed changes

EricCousineau-TRI reviewed Apr 6, 2020

View reviewed changes

hongkai-dai commented Apr 7, 2020

View reviewed changes

EricCousineau-TRI reviewed Apr 7, 2020

View reviewed changes

Address Eric's comments.

b9047ac

hongkai-dai force-pushed the pybind_branch_and_bound branch from 80f89d4 to b9047ac Compare April 7, 2020 03:08

hongkai-dai commented Apr 7, 2020

View reviewed changes

EricCousineau-TRI reviewed Apr 7, 2020

View reviewed changes

EricCousineau-TRI assigned sammy-tri Apr 7, 2020

EricCousineau-TRI reviewed Apr 7, 2020

View reviewed changes

sammy-tri reviewed Apr 7, 2020

View reviewed changes

sammy-tri added the status: squashing now https://drake.mit.edu/reviewable.html#curated-commits label Apr 7, 2020

sammy-tri reviewed Apr 7, 2020

View reviewed changes

sammy-tri merged commit 88c9311 into RobotLocomotion:master Apr 7, 2020

hongkai-dai deleted the pybind_branch_and_bound branch August 12, 2020 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add python binding for branch_and_bound #13002

Add python binding for branch_and_bound #13002

hongkai-dai commented Apr 5, 2020 •

edited by sherm1

Loading

hongkai-dai left a comment

EricCousineau-TRI left a comment

hongkai-dai left a comment

hongkai-dai left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

hongkai-dai left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

sammy-tri left a comment

sammy-tri left a comment

	@note Consider using scoped aliases to abbreviate both the usage of bound types
	and the docstring structures. Borrowing from above:

	~~~{.cc}
	{
	using Class = RigidTransform<T>;
	constexpr auto& cls_doc = doc.RigidTransform;
	py::class_<Class>(m, "RigidTransform", cls_doc.doc)
	.def(py::init(), cls_doc.ctor.doc_0args)
	...
	}
	~~~

Add python binding for branch_and_bound #13002

Add python binding for branch_and_bound #13002

Conversation

hongkai-dai commented Apr 5, 2020 • edited by sherm1 Loading

hongkai-dai left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

hongkai-dai left a comment

Choose a reason for hiding this comment

hongkai-dai left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

hongkai-dai left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

sammy-tri left a comment

Choose a reason for hiding this comment

sammy-tri left a comment

Choose a reason for hiding this comment

hongkai-dai commented Apr 5, 2020 •

edited by sherm1

Loading