Add ExternallyAppliedSpatialForceMultiplexer #18171

EricCousineau-TRI · 2022-10-24T23:16:12Z

Resolves #13139
Relates #16923

py plant:

Remove unneeded qualifier for ExternallyAppliedSpatialForce
test_propeller: Use body name rather than magic number

adder_test:

Add check for discrete number of states
Use std::make_unique instead of .reset()

This change is

py plant: - Remove unneeded qualifier for ExternallyAppliedSpatialForce - test_propeller: Use body name rather than magic number adder_test: - Add check for discrete number of states - Use std::make_unique instead of .reset()

EricCousineau-TRI

+@RussTedrake Might you have time for quick feature review?

Reviewable status: LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @RussTedrake)

RussTedrake

with just a few comments below. thanks!
+@sherm1 for platform review, please.

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @EricCousineau-TRI and @sherm1)

multibody/plant/externally_applied_spatial_force_multiplexer.h line 16 at r1 (raw file):

@system
name: ExternallyAppliedSpatialForceMultiplexer

nit: can we make it clear that the inputs are still std::vector (as opposed to just ExternallyAppliedSpatialForce elements by themselves)

multibody/plant/externally_applied_spatial_force_multiplexer.h line 41 at r1 (raw file):

  */

  explicit ExternallyAppliedSpatialForceMultiplexer(int num_inputs);

nit: strange whitespace (between the comment and the method, but then no whitespace after...?

EricCousineau-TRI

Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake and @sherm1)

multibody/plant/externally_applied_spatial_force_multiplexer.h line 16 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

nit: can we make it clear that the inputs are still std::vector (as opposed to just ExternallyAppliedSpatialForce elements by themselves)

Done, I think?
Though admittedly, this name is now super long.

Should I just revert, and denote input port types instead?

multibody/plant/externally_applied_spatial_force_multiplexer.h line 41 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

nit: strange whitespace (between the comment and the method, but then no whitespace after...?

Done. Oops!

This reverts commit 81623e4.

This reverts commit 45639a8.

EricCousineau-TRI

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake and @sherm1)

a discussion (no related file):
Working: Mark revision.

EricCousineau-TRI

Russ / Sherm, if y'all have a chance, please look at base..r2 and base..r3 and let me know which names you prefer.

Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake and @sherm1)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Mark revision.

Done. Restored shorter name, relied on docs.

RussTedrake

Reviewed 1 of 5 files at r3.
Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @sherm1)

multibody/plant/externally_applied_spatial_force_multiplexer.h line 16 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done, I think?
Though admittedly, this name is now super long.

Should I just revert, and denote input port types instead?

I prefer the shorter name. I just want it to be clear in the docs.

RussTedrake

Reviewed 4 of 5 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @sherm1)

EricCousineau-TRI

Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @sherm1)

multibody/plant/externally_applied_spatial_force_multiplexer.h line 16 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I prefer the shorter name. I just want it to be clear in the docs.

OK Thanks!

sherm1

I also prefer the shorter name.

"Combining" forces usually means adding them together, which is what I expected here at first. I see that what is actually happening is concatenation, rather than combining. To avoid confusion, please consider saying that explicitly here and naming the output port "concatenated" rather than "combined".

Platform pending a few minor things.

Reviewed 2 of 7 files at r1, 5 of 5 files at r3, all commit messages.
Reviewable status: 5 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @EricCousineau-TRI)

multibody/plant/externally_applied_spatial_force_multiplexer.h line 13 at r3 (raw file):

/**
Combines multiple std::vector<>'s of ExternallyAppliedSpatialForce<T>.

BTW Combines->Concatenates

Also consider promising that the inputs are concatenated in order of input port number, or say explicitly that the concatenation order is undefined. (I mildly prefer the former because I think that's what users would expect.)

multibody/plant/externally_applied_spatial_force_multiplexer.h line 22 at r3 (raw file):

- u(N-1)
output_ports:
- combined

BTW consider "concatenated" rather than "combined" per comment above

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 16 at r3 (raw file):

namespace {

using ListType = std::vector<ExternallyAppliedSpatialForce<double>>;

BTW better to tie this to the dut. Consider:

Code snippet:

using ListType = 
    ExternallyAppliedSpatialForceMultiplexer<double>::ListType;

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 75 at r3 (raw file):

// Tests that the system computes the correct sum.
TEST_F(ExternallyAppliedSpatialForceMultiplexerTest, AddTwoVectors) {

nit: sum -> concatenation, AddTwoVectors -> ConcatenateTwoVectors

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 94 at r3 (raw file):

// Asserts that adders have direct-feedthrough from all inputs to the output.
TEST_F(ExternallyAppliedSpatialForceMultiplexerTest, AdderIsDirectFeedthrough) {

nit: adders, Adder -> multiplexers, Multiplexer

jwnimmer-tri · 2022-10-26T16:29:45Z

multibody/plant/externally_applied_spatial_force_multiplexer.h line 34 at r3 (raw file):

  using ValueType = ExternallyAppliedSpatialForce<T>;
  using ListType = std::vector<ValueType>;

nit If we're going to publish these as public types, then we need to Doxygen-document them.

(My own preference would be to bury them in 'private' anyway. They don't appear to be useful to users at all.)

jwnimmer-tri · 2022-10-26T16:33:46Z

multibody/plant/externally_applied_spatial_force_multiplexer.h line 22 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW consider "concatenated" rather than "combined" per comment above

FWIW my 2c would be just y0 for consistency since that's what https://drake.mit.edu/doxygen_cxx/classdrake_1_1systems_1_1_multiplexer.html calls it and it likewise that system also does concatenation.

EricCousineau-TRI

Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake and @sherm1)

multibody/plant/externally_applied_spatial_force_multiplexer.h line 13 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW Combines->Concatenates

Also consider promising that the inputs are concatenated in order of input port number, or say explicitly that the concatenation order is undefined. (I mildly prefer the former because I think that's what users would expect.)

Done.

multibody/plant/externally_applied_spatial_force_multiplexer.h line 22 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FWIW my 2c would be just y0 for consistency since that's what https://drake.mit.edu/doxygen_cxx/classdrake_1_1systems_1_1_multiplexer.html calls it and it likewise that system also does concatenation.

Done, going for y0

multibody/plant/externally_applied_spatial_force_multiplexer.h line 34 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit If we're going to publish these as public types, then we need to Doxygen-document them.

(My own preference would be to bury them in 'private' anyway. They don't appear to be useful to users at all.)

Done.

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 16 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW better to tie this to the dut. Consider:

OK Buried privately, but thanks!

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 75 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: sum -> concatenation, AddTwoVectors -> ConcatenateTwoVectors

Done.

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 94 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: adders, Adder -> multiplexers, Multiplexer

Done. Removed qualifier (sufficiently informative, IMO)

jwnimmer-tri · 2022-10-26T20:15:47Z

multibody/plant/externally_applied_spatial_force_multiplexer.cc line 22 at r4 (raw file):

    }
    this->DeclareAbstractOutputPort(
        "combined",

typo

Suggestion:

"y0"

EricCousineau-TRI

Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake and @sherm1)

multibody/plant/externally_applied_spatial_force_multiplexer.cc line 22 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

typo

Done. D'oh, thanks! Used kUseDefaultName

sherm1

Reviewed 2 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @EricCousineau-TRI)

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 75 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done.

Still says "sum" in the comment above.

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 94 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done. Removed qualifier (sufficiently informative, IMO)

Still says "adder" in the comment above

RussTedrake

Reviewed 2 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @EricCousineau-TRI)

EricCousineau-TRI

Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @RussTedrake and @sherm1)

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 75 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Still says "sum" in the comment above.

Done.

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 94 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Still says "adder" in the comment above

Done. Sorry!

sherm1

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @EricCousineau-TRI)

multibody/plant/test/externally_applied_spatial_force_multiplexer_test.cc line 94 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done. Sorry!

No worries!

Add ExternallyAppliedSpatialForceMultiplexer

62383a4

py plant: - Remove unneeded qualifier for ExternallyAppliedSpatialForce - test_propeller: Use body name rather than magic number adder_test: - Add check for discrete number of states - Use std::make_unique instead of .reset()

EricCousineau-TRI assigned RussTedrake Oct 24, 2022

EricCousineau-TRI commented Oct 24, 2022

View reviewed changes

RussTedrake assigned sherm1 Oct 26, 2022

RussTedrake reviewed Oct 26, 2022

View reviewed changes

address review

45639a8

EricCousineau-TRI force-pushed the issue-12401 branch from 56714f7 to 45639a8 Compare October 26, 2022 12:23

rename files

81623e4

EricCousineau-TRI commented Oct 26, 2022

View reviewed changes

EricCousineau-TRI added 3 commits October 26, 2022 08:35

better docs

7be80e7

Revert "rename files"

6427f33

This reverts commit 81623e4.

Revert "address review"

334891c

This reverts commit 45639a8.

EricCousineau-TRI commented Oct 26, 2022

View reviewed changes

playback subset of review

a688546

EricCousineau-TRI commented Oct 26, 2022

View reviewed changes

RussTedrake reviewed Oct 26, 2022

View reviewed changes

EricCousineau-TRI commented Oct 26, 2022

View reviewed changes

sherm1 reviewed Oct 26, 2022

View reviewed changes

EricCousineau-TRI commented Oct 26, 2022

View reviewed changes

review

ec621cd

EricCousineau-TRI force-pushed the issue-12401 branch from b082bb8 to ec621cd Compare October 26, 2022 20:22

EricCousineau-TRI commented Oct 26, 2022

View reviewed changes

sherm1 reviewed Oct 26, 2022

View reviewed changes

RussTedrake reviewed Oct 26, 2022

View reviewed changes

oops

0f129a1

EricCousineau-TRI commented Oct 26, 2022

View reviewed changes

sherm1 reviewed Oct 26, 2022

View reviewed changes

EricCousineau-TRI added status: squashing now https://drake.mit.edu/reviewable.html#curated-commits release notes: feature This pull request contains a new feature labels Oct 26, 2022

EricCousineau-TRI merged commit 8fde778 into RobotLocomotion:master Oct 26, 2022

EricCousineau-TRI mentioned this pull request Oct 26, 2022

proposal: ConcatenateStdVector : public LeafSystem #16923

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ExternallyAppliedSpatialForceMultiplexer #18171

Add ExternallyAppliedSpatialForceMultiplexer #18171

EricCousineau-TRI commented Oct 24, 2022 •

edited

Loading

EricCousineau-TRI left a comment

RussTedrake left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

RussTedrake left a comment

RussTedrake left a comment

EricCousineau-TRI left a comment

sherm1 left a comment

jwnimmer-tri commented Oct 26, 2022

jwnimmer-tri commented Oct 26, 2022

EricCousineau-TRI left a comment

jwnimmer-tri commented Oct 26, 2022

EricCousineau-TRI left a comment

sherm1 left a comment

RussTedrake left a comment

EricCousineau-TRI left a comment

sherm1 left a comment

Add ExternallyAppliedSpatialForceMultiplexer #18171

Add ExternallyAppliedSpatialForceMultiplexer #18171

Conversation

EricCousineau-TRI commented Oct 24, 2022 • edited Loading

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

sherm1 left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Oct 26, 2022

jwnimmer-tri commented Oct 26, 2022

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Oct 26, 2022

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

sherm1 left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

sherm1 left a comment

Choose a reason for hiding this comment

EricCousineau-TRI commented Oct 24, 2022 •

edited

Loading