add screw mobilizer

[wf34]

Adds a 1 dof screw mobilizer that allows motion along and rotation about the z-axis of the parent frame.
Joint is also implemented already but is too much code for one PR.

Towards #12404
@mpetersen94

---

This change is 

[sherm1]

Great to see this! BTW, a screw joint (or the underlying mobilizer) is a 1-dof joint, not 2. It should have only a single associated generalized coordinate and a single generalized velocity.

[mpetersen94]

As Sherm mentioned above what you have coded up doesn't seem like a screw joint but rather a cylindrical joint. Could you please either rename this to CylindricalMobilizer or change it to be a 1-dof ScrewMobilizer where the translation and rotation are constrained to change at a fixed ratio?

Reviewed 3 of 4 files at r1.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @wf34)

[wf34]

Thank you for your kind comments.

Yes, I want screw joint, but I guess, I got sidetracked in the midst of PR preparation.
I'll rewrite it to 1 dof.

[sherm1]

A cylindrical joint can be useful also -- feel free to add one of those too if you want!

[rpoyner-tri]

Assigning +@sherm1 for either feature review, or delegation to another reviewer.

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @sherm1)

[sherm1]

@wf34 I'll hold off on the review until you've had a chance to convert this to a 1-dof screw joint. I added [WIP] to the title and set the do-not-review status for now.

In case it's helpful, this paper has a screw joint example, see Fig 1 and section 2.2. Note that you should pick either the translation or rotation as the single generalized coordinate. Then the other one is determined by the screw's pitch. [Featherstone 2008] also has a 1-dof screw joint implementation (he calls that a "helical joint"); see Table 4.1 on page 79.
+(status: do not review) +(status: do not merge)

Reviewed 1 of 4 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @sherm1 and @wf34)

---

multibody/tree/screw_mobilizer.h, line 19 at r1 (raw file):

/* This mobilizer models a screw joint between an inboard frame F and an
 outboard frame M that enables translation along with rotation abous F's z axis.

nit: abous -> about

[sherm1]

Please let us know when this is ready for another look. Thanks for doing it!

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @sherm1 and @wf34)

[wf34]

I did the conversion to 1 dof, so wip prefix is removed.
@sherm1 thank you for articles, they were helpful.

I preserve the earlier code to merge cylindrical mobilizer and joint later.

I don't seem to have rights to remove do-not-merge, do-not-review labels, sorry 🤷‍♂️

[wf34]

@mpetersen94 Dear Mark, is any more changes expected from me here prior to the review?

[sherm1]

I'll take a look today @wf34 -- sorry for the delay. I removed the "do not merge" and "do not review" labels.

[sherm1]

Checkpoint.

Reviewed 1 of 3 files at r2.
Reviewable status: 11 unresolved discussions, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @mpetersen94 and @wf34)

---

multibody/tree/screw_mobilizer.h, line 19 at r2 (raw file):

/* This mobilizer models a screw joint between an inboard frame F and an
 outboard frame M that enables translation along while rotating about F's z axis.

nit: this line and some of the lines below exceed our 80-character line length limit. Please reflow to stay within that limit.

---

multibody/tree/screw_mobilizer.h, line 28 at r2 (raw file):

 a screw pitch. The translation (z) is defined to be positive in the direction of
 frame F's z-axis axis and the rotation θ is defined to be positive according to
 the right-hand-rule with the thumb aligned in the direction of frame F's z-axis.

minor: for this mobilizer, the z axis of F remains aligned with the z axis of M. That's worth a mention since everything you say here about Fz also applies to Mz.

You may also want to mention what it means when θ=0: the F and M frames are coincident (axes are aligned and origins are co-located).

---

multibody/tree/screw_mobilizer.h, line 30 at r2 (raw file):

 the right-hand-rule with the thumb aligned in the direction of frame F's z-axis.
 The generalized velocities for this mobilizer are the rate of change of the
 coordinates, v = q̇.

nit: coordinates -> coordinate

---

multibody/tree/screw_mobilizer.h, line 39 at r2 (raw file):

  /* Constructor for a %ScrewMobilizer between an inboard frame F
   `inboard_frame_F` and an outboard frame M `outboard_frame_M` granting one

nit: redundancy not needed -- the variable names are self-explanatory. Either say just "inboard frame F" etc. or just reference the variable names.

---

multibody/tree/screw_mobilizer.h, line 50 at r2 (raw file):

      , screw_pitch_(screw_pitch)  {
    const double kEpsilon = std::sqrt(std::numeric_limits<double>::epsilon());
    DRAKE_THROW_UNLESS(screw_pitch > kEpsilon);

Is there any reason the pitch has to be positive? Negative pitch just means the screw translates with the opposite sense from θ. The only number that is questionable would be pitch==0 which makes the screw behave like a revolute joint. IMO it would be best to allow pitch to be anything, even 0, but just document what will happen.

Another question is why the pitch is double rather than T. That prevents taking derivatives with respect to pitch which might be useful in a design study or system ID where the best pitch is unknown. Any reason why it has to be double?

---

multibody/tree/screw_mobilizer.h, line 70 at r2 (raw file):

   @param[in] context The context of the model this mobilizer belongs to.
   @param[in] translation The desired translation in meters.
   @returns A constant reference to `this` mobilizer. */

minor: should note the effect this will have on the generalized coordinate θ. Also if you decide to allow pitch=0 this method should report an error in that case unless the translation is also zero.

---

multibody/tree/screw_mobilizer.h, line 75 at r2 (raw file):

      const T& translation) const;

  /* Retrieves from `context` the angle θ which describe the orientation for

nit: describe -> describes

---

multibody/tree/screw_mobilizer.h, line 102 at r2 (raw file):

   @param[in] v_FM_F The desired rate of change of `this` mobilizer's
                     translation, ż.
   @returns A constant reference to `this` mobilizer. */

minor: should note the effect this will have on the generalized velocity θ_dot. Also if you decide to allow pitch=0 this method should report an error in that case unless the translation rate is also zero.

---

multibody/tree/screw_mobilizer.h, line 126 at r2 (raw file):

   in `context`. */
  math::RigidTransform<T> CalcAcrossMobilizerTransform(
      const systems::Context<T>& context) const override;

nit: to avoid misunderstandings, all the overrides should instead be finals since the class is final.

---

multibody/tree/screw_mobilizer.h, line 198 at r2 (raw file):

  using MobilizerBase::kNq;
  using MobilizerBase::kNv;
  const double screw_pitch_;

minor: consider const T screw_pitch_;

---

multibody/tree/screw_mobilizer.h, line 203 at r2 (raw file):

  template <typename ToScalar>
  std::unique_ptr<Mobilizer<ToScalar>> TemplatedDoCloneToScalar(
      const MultibodyTree<ToScalar>& tree_clone) const;

nit: private method declarations should come before data members.

[sherm1]

Feature review pass complete, PTAL.

Reviewed 2 of 3 files at r2.
Reviewable status: 25 unresolved discussions, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @wf34)

---

multibody/tree/screw_mobilizer.h, line 213 at r2 (raw file):

template <typename T>
inline T get_screw_rotation_from_translation(const T& z, const double& screw_pitch) {

minor: Why not make these private methods of ScrewMobilizer? They can pull the screw_pitch from the class data.

---

multibody/tree/screw_mobilizer.h, line 213 at r2 (raw file):

template <typename T>
inline T get_screw_rotation_from_translation(const T& z, const double& screw_pitch) {

nit: should note in comment that these can (and are) used for position, velocity, and acceleration conversions.

---

multibody/tree/screw_mobilizer.cc, line 50 at r2 (raw file):

const T ScrewMobilizer<T>::get_translation_rate(
    const systems::Context<T>& context) const {
  const auto& v = this->get_velocities(context);

nit: this seems inconsistent with `auto q = ..." above. The return type is (I believe) a VectorBlock in either case. Should be consistent - either just "auto" to get a local VectorBlock (with copy elision), or "auto&" to keep a reference to the returned temporary.

---

multibody/tree/screw_mobilizer.cc, line 52 at r2 (raw file):

  const auto& v = this->get_velocities(context);
  DRAKE_ASSERT(v.size() == kNv);
  return get_screw_translation_from_rotation(v.coeffRef(0), screw_pitch_);

BTW wouldn't v[0] work here (and below)?

---

multibody/tree/screw_mobilizer.cc, line 89 at r2 (raw file):

  DRAKE_ASSERT(q.size() == kNq);
  Vector3<T> X_FM_translation;
  X_FM_translation << 0.0, 0.0, get_screw_translation_from_rotation(q[0], screw_pitch_);

minor: consider const Vector3<T> X_FM_translation(0,0,get_screw_...); to avoid the extra work involved with the << operator and to allow const.

---

multibody/tree/screw_mobilizer.cc, line 89 at r2 (raw file):

  DRAKE_ASSERT(q.size() == kNq);
  Vector3<T> X_FM_translation;
  X_FM_translation << 0.0, 0.0, get_screw_translation_from_rotation(q[0], screw_pitch_);

nit: line length

---

multibody/tree/screw_mobilizer.cc, line 122 at r2 (raw file):

                                             Eigen::Ref<VectorX<T>> tau) const {
  DRAKE_ASSERT(tau.size() == kNv);
  tau[0] = F_Mo_F.rotational()[2];

This doesn't look right to me. See the documentation for ProjectSpatialForce() in the base class. This should be calculating H_FMᵀ ⋅ F_Mo_F. But here H_FMᵀ = [0 0 1 0 0 pitch] so there should be two terms in the tau computation.

Please be sure to add a unit test that fails with the bug in place and succeeds after it is fixed.

---

multibody/tree/test/screw_mobilizer_test.cc, line 35 at r2 (raw file):

    mobilizer_ =
        &AddMobilizerAndFinalize(std::make_unique<ScrewMobilizer<double>>(
            tree().world_body().body_frame(), body_->body_frame(), kScrewPitch));

nit: line length (and below)

---

multibody/tree/test/screw_mobilizer_test.cc, line 48 at r2 (raw file):

  const double angle_z_first{1. * 180. / M_PI};
  const double angle_z_second{2. * 180. / M_PI};

minor: needs a unit test of the screw_pitch() method.

---

multibody/tree/test/screw_mobilizer_test.cc, line 52 at r2 (raw file):

  // context.
  mobilizer_->set_translation(context_.get(), translation_z_first);
  EXPECT_EQ(mobilizer_->get_translation(*context_), translation_z_first);

Please add tests to ensure that setting translation(rate) also sets the generalized coordinate(velocity) as expected.

---

multibody/tree/test/screw_mobilizer_test.cc, line 88 at r2 (raw file):

  // Set the state to some arbitrary non-zero value.
  mobilizer_->set_angle(context_.get(), angle_z_first);
  mobilizer_->set_angular_rate(context_.get(), angle_z_second);

minor: it's confusing to see an angle used to set an angular rate and could lead to people copying this bad practice. Please make separate angular_rate values so that the units are correct here.

---

multibody/tree/test/screw_mobilizer_test.cc, line 95 at r2 (raw file):

  // Set the "zero state" for this mobilizer, which does happen to be that of
  // zero position and velocity.

nit: I'm not sure what you mean by the comment. Isn't the "zero state" of the mobilizer defined to be zero position and velocity?

---

multibody/tree/test/screw_mobilizer_test.cc, line 128 at r2 (raw file):

  // Default behavior is to set to zero.
  mutable_mobilizer->set_random_state(*context_, &context_->get_mutable_state(),
                                      &generator);

minor: set_random_state(), etc. are const methods because they write values into the Context, not the System. (In constrast, set_default_position() writes into the System so it can be used to initialize a Context later.). So you shouldn't be using mutable_mobilizer here.

---

multibody/tree/test/screw_mobilizer_test.cc, line 226 at r2 (raw file):

  mobilizer_->ProjectSpatialForce(*context_, F_Mo_F, tau);

  const Vector1d tau_expected(torque_Mo_F[2]);

This isn't right -- missing a term.

[jwnimmer-tri]

@drake-jenkins-bot ok to test

[sherm1]

@drake-jenkins-bot ok to test

[wf34]

Is there any reason the pitch has to be positive? Negative pitch just means the screw translates with the opposite sense from θ.
The only number that is questionable would be pitch==0 which makes the screw behave like a revolute joint.
IMO it would be best to allow pitch to be anything, even 0, but just document what will happen.

My initial thinking was that a user may just rotate couterclockwise, if they need translating towards -z.
But I will agree with you on that, a screw positivity assert was removed and the comment added.

Another question is why the pitch is double rather than T. That prevents taking derivatives with respect to pitch
which might be useful in a design study or system ID where the best pitch is unknown. Any reason why it has to be double?

I've tried that initially, but ran into many convoluted compilation and linking errors; then looked up similar places in the library, where people avoid templated floating point types for mobilizer_impl (or joint) constructor arguments:

  RevoluteMobilizer(const Frame<T>& inboard_frame_F,
                    const Frame<T>& outboard_frame_M,
                    const Vector3<double>& axis_F) 

or

PlanarJoint(const std::string& name, const Frame<T>& frame_on_parent,
              const Frame<T>& frame_on_child, Vector3<double> damping)

minor: Why not make these private methods of ScrewMobilizer? They can pull the screw_pitch from the class data.

Considered it, thought it was too much coupling (for my taste). Some methods of ScrewJoint use these functions too.

This doesn't look right to me. See the documentation for ProjectSpatialForce() in the base class.

Thank you, that was the hint I needed.

Other comments were atteneded to as well.

@sherm1, thank you for the patience!

[sherm1]

CI failed with this minor lint complaint:

multibody/tree/screw_mobilizer.cc:62: Add #include for numeric_limits<> [build/include_what_you_use] [4]

Please use Reviewable (https://reviewable.io) to respond to comments in place. That allows back-and-forth discussion. If you begin with "Done" the comment will automatically be resolved.

Thanks for the changes! Feature pass #2 complete -- almost there, PTAL.

Reviewed 3 of 3 files at r3.
Reviewable status: 13 unresolved discussions, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @wf34)

---

multibody/tree/screw_mobilizer.h, line 26 at r3 (raw file):

 Zero θ defines the "zero configuration", which corresponds to frames F and
 M being coincident (axes are aligned and origins are co-located),
 see set_zero_state(). The translation (z) depends on and proportional to

nit: "and proportional" -> "and is proportional"

---

multibody/tree/screw_mobilizer.h, line 33 at r3 (raw file):

 frame F's z-axis. The frame F's z-axis and the frame M's z-axis are aligned
 at all times for this mobilizer. The generalized velocity for this mobilizer
 is the rate of change of the coordinate, ω =˙θ.

nit: Sadly the Unicode θ̇ doesn't render reliably so could be misunderstood. Please clarify, perhaps by adding "(θ_dot)" or "(d/dt θ)" after the symbol.

---

multibody/tree/screw_mobilizer.h, line 46 at r3 (raw file):

   @param[in] screw_pitch The amount of translation along F's z-axis in meters
                          for a one full revolution about F's z-axis.
                          When is set to zero, the mobilizer behaves like a

nit: "When is set" -> "When set"

---

multibody/tree/screw_mobilizer.h, line 47 at r3 (raw file):

                          for a one full revolution about F's z-axis.
                          When is set to zero, the mobilizer behaves like a
                          revolute joint, e.i. producing zero translation for

nit: "e.i." -> "i.e." (Latin "id est"=="that is")

---

multibody/tree/screw_mobilizer.h, line 48 at r3 (raw file):

                          When is set to zero, the mobilizer behaves like a
                          revolute joint, e.i. producing zero translation for
                          any value of generalized coordinate. */

nit: "value of" -> "value of the"

---

multibody/tree/screw_mobilizer.h, line 70 at r3 (raw file):

   provided by the input argument `translation`.
   The generalized coordinate θ will change proportionally, depending on
   screw_pitch.

Need to document the error condition, something like:

@throws std::exception if pitch is very near zero.

(Also for the rate method below.)

---

multibody/tree/screw_mobilizer.h, line 103 at r3 (raw file):

  /* Sets in `context` the rate of change, in meters per second, of `this`
   mobilizer's translation (see get_tranlation()) to `v_FM_F`.
   The generalized velocity ˙θ will change proportionally, depending on

nit: rendering problem with θ̇ (add clarifying note)

---

multibody/tree/screw_mobilizer.cc, line 26 at r3 (raw file):

  const double kEpsilon = std::sqrt(std::numeric_limits<double>::epsilon());
  DRAKE_THROW_UNLESS(std::fabs(screw_pitch_) > kEpsilon ||
                     abs(translation) < kEpsilon);

minor: The best way to make sure you get the right abs() overload here is this:

using std::abs;
DRAKE_THROW_UNLESS(abs(screw_pitch_) > kEpsilon ||
                   abs(translation) < kEpsilon);

That will use std::abs() for any argument that turns out to be double but look elsewhere otherwise.

This version will also continue to work if we upgrade pitch to T later.

Same comment below.

---

multibody/tree/screw_mobilizer.cc, line 64 at r3 (raw file):

  const double kEpsilon = std::sqrt(std::numeric_limits<double>::epsilon());
  DRAKE_THROW_UNLESS(std::fabs(screw_pitch_) > kEpsilon ||
                     abs(v_FM_F) < kEpsilon);

minor: using std::abs;, see comment above

---

multibody/tree/test/screw_mobilizer_test.cc, line 242 at r3 (raw file):

  EXPECT_TRUE(CompareMatrices(tau, tau_expected, kTolerance,
                              MatrixCompareType::relative));
}

minor: now needs unit tests to make sure the error condition is properly caught if someone uses the set_translation...() convenience methods but has set the pitch to zero. Search for EXPECT_THROW() or (better) DRAKE_EXPECT_THROWS_MESSAGE() to see how to do that.

[sherm1]

@mpetersen94 do you want to review this now to see if I missed anything?

Reviewable status: 13 unresolved discussions, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @wf34)

[wf34]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mpetersen94, @sherm1, and @wf34)

---

multibody/tree/test/screw_mobilizer_test.cc, line 242 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: now needs unit tests to make sure the error condition is properly caught if someone uses the set_translation...() convenience methods but has set the pitch to zero. Search for EXPECT_THROW() or (better) DRAKE_EXPECT_THROWS_MESSAGE() to see how to do that.

Found out, that DRAKE_EXPECT_THROWS_MESSAGE does not pair well with DRAKE_THROW_UNLESS, so let it be EXPECT_THROW.

matcher(err.what(), "Failure at multibody/tree/screw_mobilizer.cc:26 in set_translation(): condition 'abs(screw_pitch_) > kEpsilon || abs(translation) < kEpsilon' failed.") evaluates to false, where
err.what() evaluates to "Failure at multibody/tree/screw_mobilizer.cc:26 in set_translation(): condition 'abs(screw_pitch_) > kEpsilon || abs(translation) < kEpsilon' failed."
"Failure at multibody/tree/screw_mobilizer.cc:26 in set_translation(): condition 'abs(screw_pitch_) > kEpsilon || abs(translation) < kEpsilon' failed." evaluates to "Failure at multibody/tree/screw_mobilizer.cc:26 in set_translation(): condition 'abs(screw_pitch_) > kEpsilon || abs(translation) < kEpsilon' failed."

[wf34]

For purposes of the review I didn't squash commits, but ready to do it, when we are through.

[sherm1]

Feature pending a few minor comments.
There are currently some CI failures -- looks like a flake unrelated to this PR.
+@soonho-tri for platform review per revised rotation, please

Reviewed 4 of 4 files at r4.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee soonho-tri(platform) (waiting on @wf34)

---

multibody/tree/BUILD.bazel, line 438 at r4 (raw file):

        ":tree",
        "//common/test_utilities:eigen_matrix_compare",
        "//common/test_utilities:expect_throws_message",

minor: this is not needed if you are only using EXPECT_THROW()

---

multibody/tree/screw_mobilizer.cc, line 67 at r4 (raw file):

  const double kEpsilon = std::sqrt(std::numeric_limits<double>::epsilon());
  using std::abs;
  DRAKE_THROW_UNLESS(fabs(screw_pitch_) > kEpsilon || abs(vz) < kEpsilon);

nit: better to use abs() here rather than fabs() in case the type of screw_pitch_ changes in the future.

---

multibody/tree/test/screw_mobilizer_test.cc, line 9 at r4 (raw file):

#include "drake/common/eigen_types.h"
#include "drake/common/test_utilities/eigen_matrix_compare.h"
#include "drake/common/test_utilities/expect_throws_message.h"

minor: this include not needed if you are only using EXPECT_THROW()

[soonho-tri]

Reviewable status: 5 unresolved discussions, LGTM missing from assignee soonho-tri(platform) (waiting on @wf34)

a discussion (no related file):

[==========] Running 12 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 12 tests from ScrewMobilizerTest
[ RUN      ] ScrewMobilizerTest.ScrewPitchAccess
[       OK ] ScrewMobilizerTest.ScrewPitchAccess (6 ms)
[ RUN      ] ScrewMobilizerTest.ExceptionRaisingWhenZeroPitch
[       OK ] ScrewMobilizerTest.ExceptionRaisingWhenZeroPitch (4 ms)
[ RUN      ] ScrewMobilizerTest.StateAccess
abort: Failure at multibody/tree/screw_mobilizer.cc:72 in set_translation_rate(): condition 'false' failed.

---

---

multibody/tree/screw_mobilizer.cc, line 72 at r4 (raw file):

  DRAKE_ASSERT(v.size() == kNv);
  v[0] = get_screw_rotation_from_translation(vz, screw_pitch_);
  DRAKE_ASSERT(false);

This line should be updated. (CI fails because of it for now)

[soonho-tri]

Reviewable status: 10 unresolved discussions, LGTM missing from assignee soonho-tri(platform) (waiting on @wf34)

---

multibody/tree/screw_mobilizer.h, line 65 at r4 (raw file):

   @param[in] context The context of the model this mobilizer belongs to.
   @returns The translation (z) of the mobilizer */
  const T get_translation(const systems::Context<T>& context) const;

nit, const T => T

Here, use of const is superfluous, and can prevent valuable compiler optimizations.

---

multibody/tree/screw_mobilizer.h, line 100 at r4 (raw file):

   @param[in] context The context of the model this mobilizer belongs to.
   @returns The rate of change of the translation (ż)*/
  const T get_translation_rate(const systems::Context<T>& context) const;

nit, const T => T

---

multibody/tree/screw_mobilizer.cc, line 3 at r4 (raw file):

#include "drake/multibody/tree/screw_mobilizer.h"

#include <limits>

nit, also need #include <cmath> here.

---

multibody/tree/screw_mobilizer.cc, line 15 at r4 (raw file):

template <typename T>
const T ScrewMobilizer<T>::get_translation(

nit, const T => T

---

multibody/tree/screw_mobilizer.cc, line 54 at r4 (raw file):

template <typename T>
const T ScrewMobilizer<T>::get_translation_rate(

nit, const T => T

[soonho-tri]

Reviewable status: 18 unresolved discussions, LGTM missing from assignee soonho-tri(platform) (waiting on @wf34)

---

multibody/tree/screw_mobilizer.h, line 50 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Is there any reason the pitch has to be positive? Negative pitch just means the screw translates with the opposite sense from θ. The only number that is questionable would be pitch==0 which makes the screw behave like a revolute joint. IMO it would be best to allow pitch to be anything, even 0, but just document what will happen.

Another question is why the pitch is double rather than T. That prevents taking derivatives with respect to pitch which might be useful in a design study or system ID where the best pitch is unknown. Any reason why it has to be double?

Also, if it has to be double instead of T, then you need to make it just double instead of const double&.

---

multibody/tree/screw_mobilizer.h, line 111 at r4 (raw file):

                 translation, ż.
   @returns A constant reference to `this` mobilizer.
   @throws std::exception if the screw_pitch is very near zero. */

You can also mention the constraint on vz, |vz| < kEpsilon here.

---

multibody/tree/screw_mobilizer.h, line 141 at r4 (raw file):

This method aborts in Debug builds if `v.size()` is not one. 

Essentially, we specify the precondition of this method here. So it's better to use Doxygen's @pre tag for this. That is, => @pre v.size() == 1.

---

multibody/tree/screw_mobilizer.h, line 152 at r4 (raw file):

   velocity v from the `context` as well as the generalized accelerations
   `v̇ = dv/dt`, the rates of change of v.
   This method aborts in Debug builds if `vdot.size()` is not one. */

Ditto, => @pre vdot.size() == 1.

---

multibody/tree/screw_mobilizer.h, line 164 at r4 (raw file):

   Therefore, the result of this method is the vector of torques for
   each degree of freedom of `this` mobilizer.
   This method aborts in Debug builds if `tau.size()` is not one. */

Ditto, => @pre tau.size() == 1.

---

multibody/tree/screw_mobilizer.h, line 216 at r4 (raw file):

};

  /* `get_screw_translation_from_rotation`,

Please fix the indentation:

 /* `get_screw_translation_from_rotation`, `get_screw_rotation_from_translation`
    are used for position, velocity, and acceleration conversions.  All of these
    are governed by the same relation, depended on the `screw_pitch` of a screw
    mobilizer. 
 */

---

multibody/tree/screw_mobilizer.h, line 223 at r4 (raw file):

template <typename T>
inline T get_screw_translation_from_rotation(const T& theta,
                                             const double& screw_pitch) {

nit, const double& => double (or const T&)

---

multibody/tree/screw_mobilizer.h, line 230 at r4 (raw file):

template <typename T>
inline T get_screw_rotation_from_translation(const T& z,
                                             const double& screw_pitch) {

nit, const double& => double (or const T&)

[wf34]

Reviewable status: 10 unresolved discussions, LGTM missing from assignee soonho-tri(platform) (waiting on @soonho-tri and @wf34)

---

multibody/tree/screw_mobilizer.h, line 50 at r2 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Also, if it has to be double instead of T, then you need to make it just double instead of const double&.

Done.

---

multibody/tree/screw_mobilizer.h, line 70 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Need to document the error condition, something like:

@throws std::exception if pitch is very near zero.

(Also for the rate method below.)

Done.

---

multibody/tree/screw_mobilizer.h, line 111 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

You can also mention the constraint on vz, |vz| < kEpsilon here.

Done.

---

multibody/tree/screw_mobilizer.h, line 141 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

This method aborts in Debug builds if `v.size()` is not one. 

Essentially, we specify the precondition of this method here. So it's better to use Doxygen's @pre tag for this. That is, => @pre v.size() == 1.

Done.

---

multibody/tree/screw_mobilizer.h, line 152 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Ditto, => @pre vdot.size() == 1.

Done.

---

multibody/tree/screw_mobilizer.h, line 164 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Ditto, => @pre tau.size() == 1.

Done.

---

multibody/tree/screw_mobilizer.h, line 216 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

Please fix the indentation:

 /* `get_screw_translation_from_rotation`, `get_screw_rotation_from_translation`
    are used for position, velocity, and acceleration conversions.  All of these
    are governed by the same relation, depended on the `screw_pitch` of a screw
    mobilizer. 
 */

Done.

---

multibody/tree/screw_mobilizer.cc, line 72 at r4 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

This line should be updated. (CI fails because of it for now)

Done.

[soonho-tri]

@wf34 , I am afraid that you have not updated your branch yet.

Reviewable status: 9 unresolved discussions, LGTM missing from assignee soonho-tri(platform) (waiting on @soonho-tri and @wf34)

[soonho-tri]

OK. It's here now.

Reviewable status: 9 unresolved discussions, LGTM missing from assignee soonho-tri(platform) (waiting on @sherm1, @soonho-tri, and @wf34)

[soonho-tri]

Reviewed 4 of 4 files at r5.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee soonho-tri(platform) (waiting on @soonho-tri and @wf34)

---

multibody/tree/screw_mobilizer.h, line 112 at r5 (raw file):

                 translation, ż.
   @returns A constant reference to `this` mobilizer.
   @throws std::exception if the screw_pitch is very near zero and

nit, near zero and => near zero or

[soonho-tri]

platform

There is @sherm1 's question on the type of screw_pitch (T vs double). It would be great if a good answer can be provided for that.

Reviewable status: 3 unresolved discussions (waiting on @soonho-tri and @wf34)

[wf34]

Reviewable status: 3 unresolved discussions (waiting on @soonho-tri and @wf34)

---

multibody/tree/screw_mobilizer.h, line 112 at r5 (raw file):

Previously, soonho-tri (Soonho Kong) wrote…

nit, near zero and => near zero or

Maybe I'm wrong, but I believe it's not "or", because for screw_pitch values removed from zero it will never throw

[sherm1]

@soonho-tri I did get a satisfactory answer from Alejandro that this is typical for MbP joints but could be relaxed in the future. I'm OK with double for now as long as the code is written to be agnostic about the type of screw_pitch (e.g. abs rather than fabs).

Reviewable status: 3 unresolved discussions (waiting on @soonho-tri and @wf34)

[soonho-tri]

Reviewable status: 3 unresolved discussions (waiting on @wf34)

---

multibody/tree/screw_mobilizer.h, line 112 at r5 (raw file):

Previously, wf34 wrote…

Maybe I'm wrong, but I believe it's not "or", because for screw_pitch values removed from zero it will never throw

Right, my mistake.

---

multibody/tree/screw_mobilizer.h, line 168 at r5 (raw file):

   Therefore, the result of this method is the vector of torques for
   each degree of freedom of `this` mobilizer.
   This method aborts in Debug builds if `tau.size()` is not one. 

nit, extra whitespace at the end of this line. As a result, we have a CI failure:

multibody/tree/screw_mobilizer.h:168:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]

[soonho-tri]

OK. Got it. I've resolved that thread.

Reviewable status: 2 unresolved discussions (waiting on @wf34)

[wf34]

Reviewable status: 1 unresolved discussion (waiting on @soonho-tri and @wf34)

[soonho-tri]

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all discussions resolved, LGTM from assignees soonho-tri(platform),sherm1(platform) (waiting on @wf34)