Drake pytorch

[robert-verkuil]

This is a work in progress Pull Request adding NNSystem, a Drake Python Neural Network System powered by PyTorch.

Included is:

1. NNSystem
2. Example using NNSystem as a controller for a cartpole
3. Example using NNSystem as a Cost/Constraint in optimization
4. In Progress: Example using NNSystem with a simple Policy Gradient Algorithm

---

This change is 

[EricCousineau-TRI]

FTR Trying to get Reviewable to behave...

[EricCousineau-TRI]

Test comment from Reviewable...

Reviewable status: 0 of 16 files reviewed, all discussions resolved

[EricCousineau-TRI]

Huh, seems like Reviewable won't permit assignments, but I guess it's b/c this repo isn't configured...

@RussTedrake Would you be OK if I set up Reviewable for this?

[EricCousineau-TRI]

Reviewable status: 0 of 16 files reviewed, 6 unresolved discussions (waiting on @robert-verkuil)

a discussion (no related file):
Can you make this

---

---

src/CMakeLists.txt, line 51 at r1 (raw file):

add_subdirectory(double_integrator)
add_subdirectory(double_pendulum)
add_subdirectory(drake-pytorch)

nit Can you rename this drake_pytorch?

---

src/drake-pytorch/demo_net.pth, line 0 at r1 (raw file):
Meep! This is a binary file, possibly quite large!

Can I ask how large this is?

---

src/drake-pytorch/demos/init.py, line 0 at r1 (raw file):
nit Can you add a comment to make sure that it's on-purpose that this is empty?
e.g. from Drake:
https://github.com/RobotLocomotion/drake/blob/05e900563/bindings/pydrake/multibody/__init__.py

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 27 at r1 (raw file):

# Local
from networks import *

nit How hard is it to setup the PYTHONPATH to make drake_pytorch a proper package?
In that way, you should have:

from drake_pytorch.networks import Networks

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 27 at r1 (raw file):

# Local
from networks import *

nit Avoid using import *, always, in library code / app code. It makes traceability hard.

I would suggest either

from drake_pytorch import networks as nn
from drake_pytorch import nn_systems as nnsys

or (my preference)

from drake_pytorch.networks import Symbol1, Symbol2
from drake_pytorch.nn_systems import NNSystem, NNInferenceHelper_autodiff, nn_loader

[EricCousineau-TRI]

Reviewable status: 0 of 16 files reviewed, 10 unresolved discussions (waiting on @robert-verkuil)

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 51 at r1 (raw file):

        
        # Construct a network with params T
        param_list = np.array([param.value() for param in T])

I'd be very happy to make RobotLocomotion/drake#10466 happen for this landing in underactuated.

Is it OK if we wait for few more business days to make this happen?

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 54 at r1 (raw file):

        network = kNetConstructor()
        nn_loader(param_list, network)
        out_vec, out_in_jac, out_param_jac = NNInferenceHelper_autodiff(network, x, param_vec=T, debug=debug)

nit NNInferenceHelper_{lower_case} looks odd.

Can this either be a @staticmethod, or be all lower_case?

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 57 at r1 (raw file):

           
        # Create output
        y_values = np.array([elem.value() for elem in out_vec])

I've been think of some ways of making Python np.array stuff suck less, and be more MATLAB-y.

How would y'all like it if I made a PR (to Drake or underactuated) that would allow for things like:

some_value = a_[[1, 2, 3], [4, 5, 6]] @ a_[7, 8]
other_value = ad_[[10, 12]].T # Autodiff

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 110 at r1 (raw file):

    initial_x_trajectory = \
        PiecewisePolynomial.FirstOrderHold([0., 4.],
                                       np.column_stack((initial_state,

nit np.hstack is a shorter version of this, I think?

[robert-verkuil]

Reviewable status: 0 of 16 files reviewed, 10 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

---

src/CMakeLists.txt, line 51 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can you rename this drake_pytorch?

got it!

---

src/drake-pytorch/demo_net.pth, line at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Meep! This is a binary file, possibly quite large!

Can I ask how large this is?

just 1.7/4KB or less I think!

---

src/drake-pytorch/demos/init.py, line at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can you add a comment to make sure that it's on-purpose that this is empty?
e.g. from Drake:
https://github.com/RobotLocomotion/drake/blob/05e900563/bindings/pydrake/multibody/__init__.py

got it!

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 27 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Avoid using import *, always, in library code / app code. It makes traceability hard.

I would suggest either

from drake_pytorch import networks as nn
from drake_pytorch import nn_systems as nnsys

or (my preference)

from drake_pytorch.networks import Symbol1, Symbol2
from drake_pytorch.nn_systems import NNSystem, NNInferenceHelper_autodiff, nn_loader

thanks for this advice, ended up not needing this line but changed it to your suggestion in demos/test_nn_constraint.py

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 51 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I'd be very happy to make RobotLocomotion/drake#10466 happen for this landing in underactuated.

Is it OK if we wait for few more business days to make this happen?

yeah! landing that would be really helpful if that would be possible

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 54 at r1 (raw file):

NNInferenceHelper
changed to "nn_inference_autodiff" and "nn_inference_double" let me know if you prefer @staticmethod as well

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 110 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit np.hstack is a shorter version of this, I think?

ah, it seems like column_stack does a transpose? would you prefer np.hstack(a.T, b.T)?
https://stackoverflow.com/questions/16473042/numpy-vstack-vs-column-stack
^ was looking at a head-to-head, here.

[EricCousineau-TRI]

Reviewed 16 of 18 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @robert-verkuil)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can you make this

OK Er... incomplete thought. Oops!

---

---

src/drake_pytorch/test/init.py, line 0 at r2 (raw file):
nit Can this init file be removed or updated to show the # Empty Python module bits?

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 51 at r1 (raw file):

Previously, robert-verkuil wrote…

yeah! landing that would be really helpful if that would be possible

Sweet! It may take a bit while longer - are y'all in a rush for it?

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 54 at r1 (raw file):

Previously, robert-verkuil wrote…

NNInferenceHelper
changed to "nn_inference_autodiff" and "nn_inference_double" let me know if you prefer @staticmethod as well

OK This works!

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 110 at r1 (raw file):

Previously, robert-verkuil wrote…

ah, it seems like column_stack does a transpose? would you prefer np.hstack(a.T, b.T)?
https://stackoverflow.com/questions/16473042/numpy-vstack-vs-column-stack
^ was looking at a head-to-head, here.

Ah, didn't think about that! If they're 1D vectors, then that's the same as np.array([initial_state, final_state]), but eh, either way.

---

src/drake_pytorch/assets/acrobot.sdf, line 1 at r2 (raw file):

<sdf version='1.6'>

nit Is there a way that you can use this directly from drake, rather than copy+pasta?

---

src/drake_pytorch/assets/acrobot.urdf, line 1 at r2 (raw file):

<?xml version="1.0"?>

nit Same things as above: Can you see if you can remove this?

[robert-verkuil]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 51 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Sweet! It may take a bit while longer - are y'all in a rush for it?

Would it be possible to have by end of week? not sure what Russ's timeline is?

---

src/drake_pytorch/assets/acrobot.sdf, line 1 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Is there a way that you can use this directly from drake, rather than copy+pasta?

have had a bit of trouble here with this, where
FindResourceOrThrow("drake/multibody/benchmarks/acrobot/acrobot.sdf") can't be found
FindResourceOrThrow("drake/examples/acrobot/Acrobot.sdf") has the following error:

Any idea what I might be doing wrong?

[robert-verkuil]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 51 at r1 (raw file):

Previously, robert-verkuil wrote…

Would it be possible to have by end of week? not sure what Russ's timeline is?

ah! nvm on the above, I didn't realize I had to hit the publish button and just did now

[EricCousineau-TRI]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @robert-verkuil)

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 51 at r1 (raw file):

Previously, robert-verkuil wrote…

ah! nvm on the above, I didn't realize I had to hit the publish button and just did now

No worries! Was still working out some kinks in our sim + calibration stuff in Anzu.

Just filed the PR:
RobotLocomotion/drake#10957

---

src/drake_pytorch/assets/acrobot.sdf, line 1 at r2 (raw file):

Previously, robert-verkuil wrote…

have had a bit of trouble here with this, where
FindResourceOrThrow("drake/multibody/benchmarks/acrobot/acrobot.sdf") can't be found
FindResourceOrThrow("drake/examples/acrobot/Acrobot.sdf") has the following error:

Any idea what I might be doing wrong?

Hm... We have a bad sdf! (and ermagherd, I hate duplicate models!!!)

Lemme check it out.

[EricCousineau-TRI]

Reviewable status: 12 of 17 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

---

src/drake_pytorch/assets/acrobot.sdf, line 1 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Hm... We have a bad sdf! (and ermagherd, I hate duplicate models!!!)

Lemme check it out.

(Sorry, looking into this now!)

[EricCousineau-TRI]

---

src/drake_pytorch/assets/acrobot.sdf, line 1 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

(Sorry, looking into this now!)

Filed RobotLocomotion/drake#11009
Will work on the acute solution; will clean up later.

[EricCousineau-TRI]

Reviewable status: 12 of 17 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

---

src/drake_pytorch/assets/acrobot.sdf, line 1 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Filed RobotLocomotion/drake#11009
Will work on the acute solution; will clean up later.

Working towards a solution: RobotLocomotion/drake#11011

For now, I'd be fine if this model lands just to unblock progress; however, could you add a TODO that links to drake issue 11009 to remove this once it's resolved?

Also, can you add a link to the exact SHA1 + filepath that you got this from? e.g.:
https://github.com/RobotLocomotion/drake/blob/4c62461/multibody/benchmarks/acrobot/acrobot.sdf

[EricCousineau-TRI]

Reviewable status: 12 of 17 files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

---

src/drake_pytorch/demos/nn_policy.py, line 153 at r3 (raw file):

if __name__ == "__main__":
    net = FC()
    #NNTestSetupPendulum(net)

nit Unused code?

[EricCousineau-TRI]

Reviewable status: 12 of 17 files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 51 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

No worries! Was still working out some kinks in our sim + calibration stuff in Anzu.

Just filed the PR:
RobotLocomotion/drake#10957

Have you had a chance to tinker with these changes now that they're merged in?

[robert-verkuil]

Reviewable status: 12 of 17 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 51 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Have you had a chance to tinker with these changes now that they're merged in?

Done.

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 57 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I've been think of some ways of making Python np.array stuff suck less, and be more MATLAB-y.

How would y'all like it if I made a PR (to Drake or underactuated) that would allow for things like:

some_value = a_[[1, 2, 3], [4, 5, 6]] @ a_[7, 8]
other_value = ad_[[10, 12]].T # Autodiff

might not impact me so much? but definitely looks like a really nice syntax to use in the future!

---

src/drake_pytorch/assets/acrobot.sdf, line 1 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working towards a solution: RobotLocomotion/drake#11011

For now, I'd be fine if this model lands just to unblock progress; however, could you add a TODO that links to drake issue 11009 to remove this once it's resolved?

Also, can you add a link to the exact SHA1 + filepath that you got this from? e.g.:
https://github.com/RobotLocomotion/drake/blob/4c62461/multibody/benchmarks/acrobot/acrobot.sdf

done! Added in these comments

[EricCousineau-TRI]

Reviewed 2 of 5 files at r3, 9 of 10 files at r4.
Reviewable status: 18 of 19 files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 57 at r1 (raw file):

Previously, robert-verkuil wrote…

might not impact me so much? but definitely looks like a really nice syntax to use in the future!

OK Makes sense!

---

src/drake_pytorch/NNSystem.py, line 126 at r4 (raw file):

# Helper function for loading a list of  parameters into a network.
def nn_loader(param_list, network):
    # If params are, AutoDiffXd only extract values

nit "If params are" - is there a word missing?

---

src/drake_pytorch/assets/acrobot.sdf, line 1 at r2 (raw file):

Previously, robert-verkuil wrote…

done! Added in these comments

OK Thanks!

---

src/drake_pytorch/assets/acrobot.urdf, line 1 at r4 (raw file):

<!-- TODO: remove this file once PR #11009 lands. 

nit Do you need both the SDF and URDF? Or can one be deleted?

[EricCousineau-TRI]

a discussion (no related file):
Looks like CI wants seaborn?
https://circleci.com/gh/RussTedrake/underactuated/1082

36: ImportError: No module named seaborn

---

[RussTedrake]

Reviewable status: 18 of 19 files reviewed, 11 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Looks like CI wants seaborn?
https://circleci.com/gh/RussTedrake/underactuated/1082

36: ImportError: No module named seaborn

Looks awesome! Can’t wait to play with it...

---

---

src/drake_pytorch/init.py, line 1 at r4 (raw file):

# Empty Python module

I think if it’s living in underactuated, it should just be src/pytorch

---

src/drake_pytorch/Interactive Demos.ipynb, line 0 at r4 ([raw file](https://github.com/russtedrake/underactuated/blob/ecc33e69da9fc180c45bd626ac0bda960af584ba/src/drake_pytorch/Interactive Demos.ipynb#L0)):
I don’t want to introduce all of the binary artifacts in this file to git. Can you replace it with the clean (no results) version?

Would also like to update the deprecated call to solve, but I can also do that in a follow-on

---

src/drake_pytorch/networks.py, line 13 at r4 (raw file):

# TODO: Use torch sequential, or whatever it's called?
class FC(nn.Module):

Presumably “fully connected”? Can we use the long names here and below?

---

src/drake_pytorch/NNSystem.py, line 34 at r4 (raw file):

        network parameters before each EvalOutput().

        The user can set the context parameters to be AutoDiffXd's to calculate

Only if T = Autodiffxd. Also presumably need to declare them before allocating a Context / starting a sim

---

src/drake_pytorch/NNSystem.py, line 83 at r4 (raw file):

            # TODO: there must be a more intelligent way to detect change in the context,
            # or to install some callback that will do the copy when the context is changed?
            # OnContextChange(), something like that?

It would use the system caching tools. Perhaps @sherm1 could advise?

---

src/drake_pytorch/visualizer.py, line 12 at r4 (raw file):

class PendulumVisualizer(PyPlotVisualizer):
    a1 = 0.75

Shouldn’t need this file... or it should live in the pendulum directory...

---

src/drake_pytorch/assets/acrobot.sdf, line 1 at r4 (raw file):

<!-- TODO: remove this file once PR #11009 lands. 

Right. Shouldn’t have these two either...

---

src/drake_pytorch/assets/acrobot.urdf, line 1 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Do you need both the SDF and URDF? Or can one be deleted?

Probably neither — at least not here

---

src/drake_pytorch/demos/nn_costs_and_constraints.py, line 13 at r4 (raw file):

import matplotlib.pyplot as plt
import numpy as np
import seaborn as sns

Btw - Here is the seaborn

[RussTedrake]

Reviewable status: 18 of 19 files reviewed, 11 unresolved discussions (waiting on @EricCousineau-TRI and @robert-verkuil)

---

src/drake_pytorch/init.py, line 1 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I think if it’s living in underactuated, it should just be src/pytorch

Have we thought enough about the pros/cons of having it here vs the original separate repo? You obviously started that way...

[EricCousineau-TRI]

Reviewable status: 18 of 19 files reviewed, 11 unresolved discussions (waiting on @EricCousineau-TRI, @robert-verkuil, @RussTedrake, and @sherm1)

---

src/drake_pytorch/init.py, line 1 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Have we thought enough about the pros/cons of having it here vs the original separate repo? You obviously started that way...

I'd recommend against naming a top-level package pytorch

People would see something like this:

from torch import <my torch symbols>
from pytorch import <my Drake + torch symbols>

It should be prefixed (drake_pytorch / underactuated_pytorch) or nested (underactuated.pytorch etc.) to simplicity / clarity:

from torch import ...
from drake_pytorch import ...

---

src/drake_pytorch/NNSystem.py, line 83 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

It would use the system caching tools. Perhaps @sherm1 could advise?

This would require Python bindings for Systems cache... Maybe just need an upstream issue?

[sherm1]

Reviewable status: 18 of 19 files reviewed, 11 unresolved discussions (waiting on @EricCousineau-TRI, @robert-verkuil, and @RussTedrake)

---

src/drake_pytorch/NNSystem.py, line 83 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This would require Python bindings for Systems cache... Maybe just need an upstream issue?

@RussTedrake: One possibility for caching 3rd-party mutables is to create in Drake a proxy cache entry (Sean did that successfully in SceneGraph for dealing with FCL). The idea is to create a cache entry whose value is not important (can be an int or whatever) but whose Calc() method has the side effect of writing to the 3rd party software. The proxy should be created with the appropriate set of Context dependencies. So in this case it can depend on all_parameters_ticket(). Then when a Drake parameter changes, the Calc() method gets called.

[EricCousineau-TRI]

Reviewable status: 18 of 19 files reviewed, 11 unresolved discussions (waiting on @EricCousineau-TRI, @robert-verkuil, and @RussTedrake)

---

src/drake-pytorch/demos/nn_costs_and_constraints.py, line 27 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit How hard is it to setup the PYTHONPATH to make drake_pytorch a proper package?
In that way, you should have:

from drake_pytorch.networks import Networks

To add to this, I think src/ is already more or less on the path?

[RussTedrake]

sorry this hasn't landed. am going to close it for now, but we've discussed it's existence often and certainly won't forget about it!