Add explicit instantiations for RigidBodyTree::doKinematics. #2610

david-german-tri · 2016-06-21T21:59:11Z

Also delete a smattering of dead code.

Contributes to #2074.

This change is

… of bespoke vector lengths. This cuts down on the number of explicit template instantiations for an irrelevant runtime cost, since BotVisualizer is already copying the state vector all over the place anyway.

jwnimmer-tri · 2016-06-21T22:19:44Z

FTR, David and I spoke offline about any performance hazards from this PR. Our estimation was that the biggest risk was that some compute_JdotV=false cases could previously be dead-code-eliminated at compile time, but now it will sometimes be a runtime branch. That seemed unlikely enough to matter that we don't think it needs pre-emptive benchmarking prior to merging this PR.

Comments from Reviewable

jwnimmer-tri · 2016-06-21T22:28:23Z

with some optional review comments.

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

drake/systems/plants/RigidBodyTree.cpp, line 2382 [r1] (raw file):

    Eigen::MatrixBase<VectorXd> const&) const;
template DRAKERBM_EXPORT KinematicsCache<double> RigidBodyTree::doKinematics(
    Eigen::MatrixBase<Eigen::Block<MatrixXd const, -1, 1, true>> const&) const;

BTW It seems like the Eigen...MatrixBase...Block...-1...etc. type appears frequently enough that a little using-statement nearby here might make it more readable? Though now that I look more closely I guess true vs false is different here and below, but I guess that could be a parameter still?

drake/systems/plants/RigidBodyTree.h, line 162 [r1] (raw file):

  /// Initializes a KinematicsCache with the given configuration @p q,
  /// computes the kinematics, and returns the cache.

BTW Consider documenting the limited set of DerivedQ types that are available for use by calling code, or perhaps just refer readers to the cc file, or otherwise caution that not all types are supported?

drake/systems/plants/RigidBodyTree.h, line 168 [r1] (raw file):

  /// Initializes a KinematicsCache with the given configuration @p q
  /// and velocity @p v, computes the kinematics, and returns the cache.

BTW Consider documenting the limited set of types...

drake/systems/plants/RigidBodyTree.h, line 174 [r1] (raw file):

      const Eigen::MatrixBase<DerivedV>& v, bool compute_JdotV = true) const;

  /// Computes the kinematics on the given @p cache.

BTW Consider documenting the limited set of types...

drake/systems/plants/RigidBodyTree.h, line 345 [r1] (raw file):

      const KinematicsCache<typename Derived::Scalar>& cache,
      Eigen::MatrixBase<Derived>& pos,
      const std::set<int>& body_idx) const;  // = emptyIntSet);

BTW Do these need a CHANGELOG entry?

Comments from Reviewable

liangfok · 2016-06-22T03:14:41Z

with some optional review comments and questions.

Reviewed 1 of 3 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

drake/systems/plants/RigidBodyTree.cpp, line 2378 [r1] (raw file):

    KinematicsCache<AutoDiffUpTo73d>&, bool) const;

// Explicit template instantiations for doKinematics(q).

BTW, how did you decide which explicit template instantiations to include? Are these the minimal set to get Drake as it stands to compile?

drake/systems/plants/RigidBodyTree.h, line 161 [r1] (raw file):

  void drawKinematicTree(std::string graphviz_dotfile_filename) const;

  /// Initializes a KinematicsCache with the given configuration @p q,

BTW, since KinematicsCache is the name of a class, consider surrounding it with back ticks so it appears in a monospace font.

drake/systems/plants/RigidBodyTree.h, line 167 [r1] (raw file):

      const Eigen::MatrixBase<DerivedQ>& q) const;

  /// Initializes a KinematicsCache with the given configuration @p q

BTW, same comment here about KinematicsCache.

drake/systems/plants/RigidBodyTree.h, line 342 [r1] (raw file):

  template <typename Derived>
  void getContactPositions(

BTW, if I understand correctly, RigidBodyTree::getContactPositions() and RigidBodyTree::getContactPositionsJac() are being removed because they are not used anywhere else within Drake, its examples, and its unit tests. Is this correct?

I'm slightly worried about removing them because I may need them to satisfy the "contact occurred" feature request mentioned in #2606 (see the third high priority item).

I suppose I could always dig into the git history and recover these methods should they be necessary. Is this the expected best practice?

This API change is related to the conversation in #2597 (comment). We need to clarify how much effort we should assert ensuring downstream users experience a gradual transition to the new API.

Comments from Reviewable

amcastro-tri · 2016-06-22T03:38:34Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

drake/systems/plants/RigidBodyTree.cpp, line 2389 [r1] (raw file):

template DRAKERBM_EXPORT KinematicsCache<AutoDiffXd>
RigidBodyTree::doKinematics(
    Eigen::MatrixBase<VectorX<AutoDiffXd>> const&) const;

Wouldn't we like to have here a specialization of doKinematics with AutoDiffUpTo73d?

drake/systems/plants/RigidBodyTree.cpp, line 2396 [r1] (raw file):

    bool) const;
template DRAKERBM_EXPORT KinematicsCache<double> RigidBodyTree::doKinematics(
    Eigen::MatrixBase<Eigen::Block<VectorXd const, -1, 1, false>> const&,

Uff, all these block and map specializations are so annoying! (complaining about templates' hell, not your solution @david-german-tri). Actually I am wondering if these arguments could be an Eigen::Ref, which could potentially cut down the number of options.
I'll play tomorrow with it and I'll let you know what I find out.

Comments from Reviewable

liangfok · 2016-06-22T03:48:24Z

I believe the CI error ld: symbol(s) not found for architecture x86_64 is real; I can replicate it on my OS X machine.

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

david-german-tri · 2016-06-22T13:58:15Z

Yeah, that's definitely real. I've pushed a possible fix; we'll see what Jenkins says.

Review status: 1 of 4 files reviewed at latest revision, all discussions resolved, some commit checks pending.

drake/systems/plants/RigidBodyTree.cpp, line 2378 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, how did you decide which explicit template instantiations to include? Are these the minimal set to get Drake as it stands to compile?

Yes.

drake/systems/plants/RigidBodyTree.cpp, line 2382 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW It seems like the Eigen...MatrixBase...Block...-1...etc. type appears frequently enough that a little using-statement nearby here might make it more readable? Though now that I look more closely I guess true vs false is different here and below, but I guess that could be a parameter still?

Yeah, I considered this, but there's enough variation that I don't think it pans out. Also, I can't figure out what the `InnerPanel` boolean template parameter means, so I'd rather not have to document it. ;-)

drake/systems/plants/RigidBodyTree.cpp, line 2389 [r1] (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Wouldn't we like to have here a specialization of doKinematics with AutoDiffUpTo73d?

Very possibly, but we don't appear to need it right now. Feel free to add it later!

drake/systems/plants/RigidBodyTree.cpp, line 2396 [r1] (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Uff, all these block and map specializations are so annoying! (complaining about templates' hell, not your solution @david-german-tri). Actually I am wondering if these arguments could be an Eigen::Ref, which could potentially cut down the number of options.
I'll play tomorrow with it and I'll let you know what I find out.

My guess is that won't work, but I'd be delighted to be proven wrong! I won't block this PR on it, though.

drake/systems/plants/RigidBodyTree.h, line 161 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, since KinematicsCache is the name of a class, consider surrounding it with back ticks so it appears in a monospace font.

Done.

drake/systems/plants/RigidBodyTree.h, line 162 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Consider documenting the limited set of DerivedQ types that are available for use by calling code, or perhaps just refer readers to the cc file, or otherwise caution that not all types are supported?

Done.

drake/systems/plants/RigidBodyTree.h, line 167 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, same comment here about KinematicsCache.

Done.

drake/systems/plants/RigidBodyTree.h, line 168 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Consider documenting the limited set of types...

Done.

drake/systems/plants/RigidBodyTree.h, line 174 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Consider documenting the limited set of types...

Done.

drake/systems/plants/RigidBodyTree.h, line 342 [r1] (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, if I understand correctly, RigidBodyTree::getContactPositions() and RigidBodyTree::getContactPositionsJac() are being removed because they are not used anywhere else within Drake, its examples, and its unit tests. Is this correct?

I'm slightly worried about removing them because I may need them to satisfy the "contact occurred" feature request mentioned in #2606 (see the third high priority item).

I suppose I could always dig into the git history and recover these methods should they be necessary. Is this the expected best practice?

This API change is related to the conversation in #2597 (comment). We need to clarify how much effort we should assert ensuring downstream users experience a gradual transition to the new API.

That's correct. Added CHANGELOG entry.

Yes, the best practice is to revive dead code from source control when you need it.

drake/systems/plants/RigidBodyTree.h, line 345 [r1] (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Do these need a CHANGELOG entry?

Good idea, done.

Comments from Reviewable

…. Delete some other dead functions.

jwnimmer-tri · 2016-06-22T14:18:14Z

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from Reviewable

amcastro-tri · 2016-06-22T14:48:09Z

@david-german-tri: why is it that I don't see the new commit with your fix? did you push -f?

amcastro-tri · 2016-06-22T15:04:06Z

never mind. super clear in reviewable.

Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

liangfok · 2016-06-22T15:05:10Z

I received an e-mail notification at 9:58 AM this morning.

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

Comments from Reviewable

Make BotVisualizer pass a VectorX to doKinematics, instead of a bunch…

9dea48d

… of bespoke vector lengths. This cuts down on the number of explicit template instantiations for an irrelevant runtime cost, since BotVisualizer is already copying the state vector all over the place anyway.

david-german-tri assigned jwnimmer-tri Jun 21, 2016

david-german-tri force-pushed the do_kinematics branch from 08cd953 to ca3107a Compare June 22, 2016 13:57

Move doKinematics out of RigidBodyTree.h using explicit instantiation…

ca3107a

…. Delete some other dead functions.

david-german-tri merged commit af7323f into RobotLocomotion:master Jun 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explicit instantiations for RigidBodyTree::doKinematics. #2610

Add explicit instantiations for RigidBodyTree::doKinematics. #2610

david-german-tri commented Jun 21, 2016 •

edited

Loading

jwnimmer-tri commented Jun 21, 2016

jwnimmer-tri commented Jun 21, 2016

liangfok commented Jun 22, 2016

amcastro-tri commented Jun 22, 2016

liangfok commented Jun 22, 2016

david-german-tri commented Jun 22, 2016

jwnimmer-tri commented Jun 22, 2016

amcastro-tri commented Jun 22, 2016

amcastro-tri commented Jun 22, 2016

liangfok commented Jun 22, 2016

Add explicit instantiations for RigidBodyTree::doKinematics. #2610

Add explicit instantiations for RigidBodyTree::doKinematics. #2610

Conversation

david-german-tri commented Jun 21, 2016 • edited Loading

jwnimmer-tri commented Jun 21, 2016

jwnimmer-tri commented Jun 21, 2016

liangfok commented Jun 22, 2016

amcastro-tri commented Jun 22, 2016

liangfok commented Jun 22, 2016

david-german-tri commented Jun 22, 2016

jwnimmer-tri commented Jun 22, 2016

amcastro-tri commented Jun 22, 2016

amcastro-tri commented Jun 22, 2016

liangfok commented Jun 22, 2016

david-german-tri commented Jun 21, 2016 •

edited

Loading