multibody/benchmarks/acrobot/acrobot.h almost completely duplicates examples/Acrobot/AcrobotPlant.h

[RussTedrake]

Why? Please merge the best of both of these into a single location.

Related to #5408 and #6619.

[EricCousineau-TRI]

I recommend we remove examples/.../acrobot_plant.h, and replace it with either (a) something parsing benchmarks/.../acrobot.sdf or (b) benchmarks/.../acrobot/make_acrobot_plant.h. (benchmarks/.../acorobt/acrobot.h is not a LeafSystem, which is totes fine by me for the purpose of benchmarking).

I would prefer we prefer exporting useful benchmarks (potentially with a disclaimer of an unstable API) over examples, esp. if we want them to be library components (along the lines of #9645).

[amcastro-tri]

Option a) already exists: drake/multibody/acrobot. The value of examples/acrobot is that it shows how to make a system with hand-written dynamics? Probably we could make examples::acrobot::AcrobotPlant to use the math provided by multibody::benchmarks::Acrobot?. That way the benchmark remains non-system code and the examples becomes a thin wrapper around that code.

[EricCousineau-TRI]

Aye, a form of that would be fine by me.

@jwnimmer-tri When you have a chance, can you scribe down what you mentioned your preference was per the f2f among the three of us (you, Alejandro, and myself)?

[jwnimmer-tri]

My premise was thus:

1. //examples/acrobot should continue to provide the code + models that are relevant / interesting to users:
   • the analytical plant leaf system
   • how to parse the acrobot.sdf (or urdf) into MbP
   • the SDF and the URDF
2. The benchmark tests should draw from (reuse) that code to the extent doing so is practical.
   • The benchmark tests should reuse the examples SDF/URDF.
   • The benchmark tests might still need additional code, which they are welcome to write and keep locally (not in examples).
3. Code should have in-package tests to the extent necessary (so examples/acrobot is probably missing some, if it's going to be the locus for most users / tests).

The TL;DR is that examples/acrobot should be the agreed-upon and obvious place where basic acrobot stuff lives. The benchmark should draw from the example, instead of copy-pasting from it.

[EricCousineau-TRI]

SGTM. (Still sad that we'd have public library code that depends on //examples, but meh for now.)

@amcastro-tri @mitiguy @sherm1 What say ye?

[jwnimmer-tri]

Oh, right, that was the other part -- possibly the benchmarks should become internal-only visibility (and not installed).

[sherm1]

We've been working with the reverse assumption. benchmarks are a permanent part of the Drake used to make sure we're getting the right answers. examples come and go so can depend on things in benchmarks but not vice versa.

[jwnimmer-tri]

Hm, I think perhaps some of the axioms of the hallway conversation were not true. There was a claim that there was a LeafSystem analytical implementation of Acrobot in benchmarks, but I don't see that. I only see some Calc functions and then the MbP sugar.

I would be OK if there was a LeafSystem in examples, which called the Calc functions from benchmarks. I just don't want the LeafSystem code to be in benchmarks, because users will not look there.

[RussTedrake]

I agreed with @jwnimmer-tri 's statement above that //examples/acrobot currently provides code + models that are relevant / interesting to users:

• the analytical plant leaf system
• how to parse the acrobot.sdf (or urdf) into MbP
• the SDF and the URDF also
• the AcrobotPlant declares parameters (which MBP does not have yet).

I would also add that the underactuated repo builds on those classes for further examples (in python). I think the underactuated example would have to be updated, but could live on if somehow benchmarks wanted to fill that role instead. However, I do agree that users might be less likely to stumble upon them there.

[jwnimmer-tri]

Due to #6619, I should probably own this issue as well.

[jwnimmer-tri]

Actually there are three places to consolidate:

• examples/acrobot
• examples/multibody/acrobot
• multibody/benchmarks/acrobot

[jwnimmer-tri]

We've been working with the reverse assumption. benchmarks are a permanent part of the Drake used to make sure we're getting the right answers. examples come and go so can depend on things in benchmarks but not vice versa.

I think I have to disagree. There should be such a thing as a "golden example" that is well-crafted, durable, and shows off in a single location the best ways to use Drake. Not all examples will meet that bar, but I think it'd be reasonable to agree that acrobot, pendulum, and perhaps a few others should be designed with that in mind, and that in those cases it should be OK to depend on those examples from other places in the tree (especially tests).

To the extent we need some code or model to be an "oracle", we can document it as such, no matter where it lives in the tree.

[jwnimmer-tri]

Re-assigning to @mitiguy to resolve. In #13593 we're apparently adding yet another acrobot, buried inline in a unit test. That's beyond what I can retrofit.

[jwnimmer-tri]

Per https://github.com/RobotLocomotion/drake/issues/5408#issuecomment-700368685, I guess the new model is different (simpler), not identical. I'll still keep this issue assigned to @mitiguy though, as "benchmarks czar". The original reasons why I took it over for myself no longer apply.

Aside: In #5408 we talked about regression testing that the sdf and urdf data are interchangeable / identical. If that's a worthwhile check, we could do it under this issue umbrella.