initial bindings for pendulum example

[RussTedrake]

This change is 

[RussTedrake]

+@EricCousineau-TRI for feature review, please

---

Comments from Reviewable

[EricCousineau-TRI]

---

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/examples/BUILD.bazel, line 36 at r1 (raw file):

# `__init__.py` to simplify dependency management, meaning that
# classes are organized by their directory structure rather than
# by C++ namespace. If you want all symbols, use `all.py`.

BTW The comment about all.py may not apply here - should it be removed?

---

Comments from Reviewable

[RussTedrake]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/examples/BUILD.bazel, line 36 at r1 (raw file):

Previously, EricCousineau-TRI wrote…

BTW The comment about all.py may not apply here - should it be removed?

Done. Is that what you meant?

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: 3 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.

---

bindings/pydrake/examples/BUILD.bazel, line 36 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Done. Is that what you meant?

Yup!

---

bindings/pydrake/examples/BUILD.bazel, line 79 at r2 (raw file):

    deps = [
        ":pendulum_py",
        "//drake/bindings/pydrake/systems:systems",

BTW Is the :systems add here redundant?

---

bindings/pydrake/examples/test/pendulum_test.py, line 47 at r2 (raw file):

        self.assertFalse((state == initial_state).all())
        
        

Whitespace and comments from WIP commit?

---

Comments from Reviewable

[RussTedrake]

doh. i just force-pushed some old code and blew away some changes. will push my clean copy from home tonight.

---

Comments from Reviewable

[RussTedrake]

ok. fixed now. ptal?

---

Review status: 3 of 5 files reviewed at latest revision, 2 unresolved discussions.

---

bindings/pydrake/examples/BUILD.bazel, line 79 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

BTW Is the :systems add here redundant?

Done.

---

bindings/pydrake/examples/test/pendulum_test.py, line 47 at r2 (raw file):

Previously, EricCousineau-TRI wrote…

Whitespace and comments from WIP commit?

Done.

---

Comments from Reviewable

[RussTedrake]

+@ggould-tri for platform review.

---

Review status: 3 of 5 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[ggould-tri]

; one state access style question below.

---

Reviewed 3 of 5 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/examples/test/pendulum_test.py, line 37 at r3 (raw file):

        simulator.Initialize()
        state = simulator.get_mutable_context().get_mutable_state()\
                         .get_mutable_continuous_state().get_mutable_vector()

Given that people will frequently copy-paste from example code, is this the state access pattern we want to encourage? It seems like if more than one of your diagram's systems has state, this will be very obscure. Extracting the pendulum state in particular would be clearer, even if in this example it is unnecessary.

---

Comments from Reviewable

[RussTedrake]

Completely agree. This was a function of the systems bindings that were in place so far. Cleanup is a priority for me and will come in follow-up PRs. (No way i want state access like this in my inline textbook examples :)

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[RussTedrake]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/examples/test/pendulum_test.py, line 37 at r3 (raw file):

Previously, ggould-tri wrote…

Given that people will frequently copy-paste from example code, is this the state access pattern we want to encourage? It seems like if more than one of your diagram's systems has state, this will be very obscure. Extracting the pendulum state in particular would be clearer, even if in this example it is unnecessary.

See comment above. Cleanup coming soon.

---

Comments from Reviewable

[ggould-tri]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/examples/test/pendulum_test.py, line 37 at r3 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

See comment above. Cleanup coming soon.

Can you put in a TODO comment for now? If the cleanup gets missed or delayed, it would be good to have a breadcrumb.

---

Comments from Reviewable

[amcastro-tri]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/examples/test/pendulum_test.py, line 37 at r3 (raw file):

Previously, ggould-tri wrote…

Can you put in a TODO comment for now? If the cleanup gets missed or delayed, it would be good to have a breadcrumb.

FYI. With MBT code we've been leaning towards an API provided by the system like the one here. So that reads pendulum.SetAngle(&pendulum_context, M_PI / 3.0);. I believe that would allow future uses where setting the state in this case could depend on system parameters or even cached computations. If we fiddle with the state by hand as in this example we might limit these future extensions. Just a thought, mainly looking for your feedback @RussTedrake given our conversations on using MBT for your class.

---

Comments from Reviewable

[EricCousineau-TRI]

bindings/pydrake/examples/test/pendulum_test.py, line 37 at r3 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

FYI. With MBT code we've been leaning towards an API provided by the system like the one here. So that reads pendulum.SetAngle(&pendulum_context, M_PI / 3.0);. I believe that would allow future uses where setting the state in this case could depend on system parameters or even cached computations. If we fiddle with the state by hand as in this example we might limit these future extensions. Just a thought, mainly looking for your feedback @RussTedrake given our conversations on using MBT for your class.

If we connectivity information in each System locally, could we simplify this API further and have a system resolve it's context from any parent context (and avoid the need to call Diagram::GetSubSystemContext<>)?
This seems like it would also drastically simplify access (such that something nested deep (say 3 levels) does not have to go through a whole slew of manually changed Get*Context() calls.

(I'm not sure what information is actually stored, but this is my impression when discussing Systems Python bindings, and how to handle stale pointers, which was part of the influence for using keep_alive versus something more conservative.)

---

Comments from Reviewable

[sherm1]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/examples/test/pendulum_test.py, line 37 at r3 (raw file):

Previously, EricCousineau-TRI wrote…

If we connectivity information in each System locally, could we simplify this API further and have a system resolve it's context from any parent context (and avoid the need to call Diagram::GetSubSystemContext<>)?
This seems like it would also drastically simplify access (such that something nested deep (say 3 levels) does not have to go through a whole slew of manually changed Get*Context() calls.

(I'm not sure what information is actually stored, but this is my impression when discussing Systems Python bindings, and how to handle stale pointers, which was part of the influence for using keep_alive versus something more conservative.)

We intentionally restricted leaf systems to get only their own leaf contexts to provide a clear message and near-bulletproof guarantee that a leaf can access and modify only its own context. The intervention of a diagram is required to extract the appropriate leaf context from the diagram context. I still think that is a good idea, even if it requires some extra API calls. I think it aids clarity of code & thought.

OTOH all those chained get_mutables are awful and unnecessary!

---

Comments from Reviewable

[EricCousineau-TRI]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/examples/test/pendulum_test.py, line 37 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

We intentionally restricted leaf systems to get only their own leaf contexts to provide a clear message and near-bulletproof guarantee that a leaf can access and modify only its own context. The intervention of a diagram is required to extract the appropriate leaf context from the diagram context. I still think that is a good idea, even if it requires some extra API calls. I think it aids clarity of code & thought.

OTOH all those chained get_mutables are awful and unnecessary!

Is there a way that an explicit API entrypoint could be introduced, something like ResolveSubContext(parent_context), that will let the system rifle through whatever levels needed, to localize the information needed to get its own context from the parent, something that's less strict?
Some disclaimers could be made to say that this code should only be used external to the Systems framework, as sugar.

For the chain of get_mutable_*, totally agree! That, I could see as satisfied by Alejandro's example.
(Sorry if I'm conflating state type access with context hierarchy access, just chimed in for general convenience.)

One minor note: pybind has no mechanisms to enforce const access, as that's not something present in Python, so this could be changed to simulator.get_context().get_state().get_continuous_state().get_vector() (caveat: numpy does have enforcements for const-ness, but state is a BasicVector).

---

Comments from Reviewable

[RussTedrake]

Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.

---

bindings/pydrake/examples/test/pendulum_test.py, line 37 at r3 (raw file):

Previously, EricCousineau-TRI wrote…

Is there a way that an explicit API entrypoint could be introduced, something like ResolveSubContext(parent_context), that will let the system rifle through whatever levels needed, to localize the information needed to get its own context from the parent, something that's less strict?
Some disclaimers could be made to say that this code should only be used external to the Systems framework, as sugar.

For the chain of get_mutable_*, totally agree! That, I could see as satisfied by Alejandro's example.
(Sorry if I'm conflating state type access with context hierarchy access, just chimed in for general convenience.)

One minor note: pybind has no mechanisms to enforce const access, as that's not something present in Python, so this could be changed to simulator.get_context().get_state().get_continuous_state().get_vector() (caveat: numpy does have enforcements for const-ness, but state is a BasicVector).

great discussion. i've added the TODO. can i get an ok? :-)

---

Comments from Reviewable

[RussTedrake]

bindings/pydrake/examples/test/pendulum_test.py, line 37 at r3 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

great discussion. i've added the TODO. can i get an ok? :-)

@amcastro-tri -- regarding your pendulum.SetAngle() comment/question... here are a few quick thoughts.

• this is essentially the API that we use already for named vectors (that are autogenerated from the python scripts). so we should make sure we match the existing format.
• the only(?) advantage of putting it at the system level (instead of the vector level), I guess, is that the system knows whether to re-direct to continuous state or discrete state. but that is indeed valuable information. the disadvantage is that it increases the burden on the system author, so I think it definitely makes sense for MBP, but perhaps not for every system?

@EricCousineau-TRI -- I'm torn about walking the line with const access. I like the idea of the python code being simpler to read, but are we playing with fire? And even if python can't enforce it, how do the actual pybind bindings get away with writing to a const reference?

---

Comments from Reviewable

[amcastro-tri]

@RussTedrake. The reason I prefer the system level API is because by passing the context you have access to caching and also to parameters. The autgen vectors assume you can actually place this info in an array format. Consider the case in which the joint actually was model as a constraint for a "loop-joint". There is no angle in the state vector in that case but it is the result of a computation. Of course, in the general case we could copy the output (as we already do by the way) to a named vector with a set of useful numbers to the user, say joint angles. This is what I am doing for the energy shaping example. I'll send you a link when I push it so you can see it.