RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.33k stars 1.26k forks source link

Allow auto-differentiation with respect to Joint parameters #2235

Closed david-german-tri closed 5 years ago

david-german-tri commented 8 years ago

Joints in Drake 2.0 Design Doc

david-german-tri commented 8 years ago

As reported on #1312, sharing Google Docs from tri.global to contributors outside TRI is currently broken. We're trying to fix it, but in the meantime that link won't work for you.

tkoolen commented 8 years ago

Should we incorporate rigid body parameters in this issue as well?

I still don't have access to the document by the way. I've requested access; could you approve? Thanks for looking into it.

amcastro-tri commented 8 years ago

I'd say we take a step at a time. Let's start with Joint and keep the PR as small as possible. I'd start with a .h and .cc for Joint and one or two specializations (maybe revolute and prismatic). Then a .cc google test file for this new implementation (no RBT or systems in this unit test, just joints and simple setups to test automatic differentiation and API). So this PR should include 5 to 7 new files. I would also start placing this new implementation into a separate directory (something like [drake-distro]/multibody_dynamics/joints supporting the idea in #2225), would that be possible or too difficult considering our current build system? Also we are moving towards all-lower-case namespaces so consider placing Joints 2.0 within drake::multibody_dynamics::joints as suggested in the Google style guide

tkoolen commented 8 years ago

Unfortunately we can't only do Joint in the pull request without breaking existing functionality (autodiffing w.r.t. states).

tkoolen commented 8 years ago

I still can't access the google doc, so I'll voice a minor concern here: the frictionTorque method will in the long run need to be moved to a different location, since its implementation for 1-dof joints uses abs, so it wouldn't be possible to instantiate those joints for TrigPoly scalar type.

amcastro-tri commented 8 years ago

That's why I was suggesting placing the new code in a new folder and new namespaces (we are moving towards new namespaces naming as we add "new" implementations). So if Joint 2.0 leaves in namespace drake::multibody_dynamics it should not cause any trouble. I suggest testing Joint 2.0 independently from the rest of the code before actually instantiating it in the RBT (which will also need to be templated making a truly gigantic PR). Those tests should tell us a lot for instance about the right API. A first implementation of Joint 2.0 wouldn't necessarily have to have full functionality, just to prove that the new API is actually useful before moving to a full implementation. Does it make sense?

tkoolen commented 8 years ago

OK, do you guys want me to just go ahead and do this? I estimate it'll take me about 3 hours to rework my original pull request without writing tests. Add another hour or so for testing.

david-german-tri commented 8 years ago

@tkoolen, when I ran the overall approach by @RussTedrake yesterday, he expressed concerns about performance when the scalar type is AutoDiffScalar. He was planning to sync up with you on that, since @amcastro-tri and I have no idea. So if you haven't already had that conversation, you might want to wait for it.

tkoolen commented 8 years ago

I talked to Russ about his performance concerns. I told him I don't think there should be any issue, but we'll do a benchmark before and after.

david-german-tri commented 8 years ago

Awesome! In that case, I say go for it. As @amcastro-tri said, incremental PRs would be much appreciated if possible - easier for us to digest and review than the whole migration in one shot. In exchange, I promise to send comments promptly.

By the way, if you want to be reviewing any of the System2 stuff, just let me know and I'll ping you on those PRs before merging them.

amcastro-tri commented 8 years ago

Awesome! thumbs up to the benchmarks!

tkoolen commented 8 years ago

Just putting this link here so that I don't have to fish it out of my gmail every time (it's apparently not the same as the one in the issue description, which I still can't access): https://docs.google.com/a/trinst.com/document/d/16fxMgXaqdM0Raa1UbAvE8NpozzdCBcgx9egJjM6Rr94/edit?usp=sharing

david-german-tri commented 8 years ago

Corrected the description, sorry about that.

RussTedrake commented 5 years ago

I think this issue could now be rephrased as adding support for including Joint parameters as parameters in the context of the multibodyplant. Super important, and on the roadmap.

Will close this one as it's unlikely to move forward in it's current form.