Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[multibody topology] Adds misc algorithms over graph and forest #21806

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

sherm1
Copy link
Member

@sherm1 sherm1 commented Aug 12, 2024

This is the 10th installment in the PR train for #20225, following #21800.

Adds simple algorithms that act on LinkJointGraphs or SpanningForests. These are needed by MultibodyPlant for the switchover to the new topology code. The current signatures are chosen for compatibility with existing MbP functions that are being replaced. These will likely be improved later (they are all internal). For example, some of them use sets unnecessarily or define ordering expectations ("sorted by BodyIndex", e.g.) that may not be necessary.

What's here

LinkJointGraph::
   // Fast functions that depend on the forest having been built.
 - FindPathFromWorld(link) -> vector<link>
 - FindFirstCommonAncestor(link1, link2) -> link
 - FindSubtreeLinks(link) -> vector<link>
 - GetSubgraphsOfWeldedLinks() -> vector<set<link>>
 - GetLinksWeldedTo(link) -> set<link>

   // Slow functions that can be used before the forest is built, i.e. pre-finalize.
 - CalcSubgraphsOfWeldedLinks() -> vector<set<link>>
 - CalcLinksWeldedTo(link) -> set<link>

SpanningForest::
 - FindPathFromWorld(mobod) -> vector<mobod>
 - FindFirstCommonAncestor(mobod1, mobod2) -> mobod
 - FindPathsToFirstCommonAncestor(mobod1, mobod2, &path1, &path2) -> mobod
 - FindSubtreeLinks(mobod) -> vector<link>

Unit tests are in link_joint_graph_test.cc and spanning_forest_test.cc, resp. I added them to existing tests since they need graphs and forests to work on.

There are no user-visible changes here; everything is internal. MultibodyPlant continues to use the old topology code.


This change is Reviewable

@sherm1 sherm1 added status: do not merge status: do not review release notes: none This pull request should not be mentioned in the release notes labels Aug 12, 2024
@sherm1 sherm1 force-pushed the misc_algorithms_for_topology branch 3 times, most recently from 8c00387 to 03b07eb Compare August 14, 2024 21:39
@sherm1 sherm1 marked this pull request as ready for review August 15, 2024 00:45
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@DamrongGuoy for feature review, please

Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers

Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW reviewers: if you want an overview of the terminology and relevant objects there is a big chunk of comments at the top of link_joint_graph.h

Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers

@sherm1 sherm1 force-pushed the misc_algorithms_for_topology branch from 03b07eb to bd38268 Compare August 15, 2024 18:47
Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkpoint. Next, I'll review the unit tests.

Reviewed 3 of 6 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sherm1)


multibody/topology/link_joint_graph.cc line 791 at r1 (raw file):

      visited[index_to_ordinal(index)] = true;
    auto* which_list = ssize(welded_links) == 1 ? &singletons : &subgraphs;
    which_list->emplace_back(std::move(welded_links));

BTW, would "if-else" be easier to read?

Suggestion:

    if (ssize(welded_links) == 1) {
      singletons.emplace_back(std::move(welded_links));
    } else {
      subgraphs.emplace_back(std::move(welded_links));
    }

multibody/topology/link_joint_graph.cc line 842 at r1 (raw file):

    // Don't reprocess if we already did the other end.
    if (result->count(welded_link_index) == 0)
      AppendLinksWeldedTo(welded_link_index, &*result);

BTW, is the & to mark output parameter, or &* a typo?

Suggestion:

      AppendLinksWeldedTo(welded_link_index, result);

multibody/topology/spanning_forest.h line 162 at r2 (raw file):

  /** Finds the highest-numbered mobilized body that is common to the paths
  from each of the given ones to World. Returns World immediately if the bodies

Question. From the code, I see it returns the common ancestor with the largest value of level(). Does highest-numbered in the doc mean the "largest value of level()" ? Or it means the "largest value of MobodIndex" ? or both? Same question for the next function FindPathsToFirstCommonAncestor.

Code quote:

  /** Finds the highest-numbered mobilized body that is common to the paths
  from each of the given ones to World. Returns World immediately if the bodies

multibody/topology/spanning_forest.h line 165 at r2 (raw file):

  are on different trees; otherwise the cost is O(ℓ) where ℓ is the length
  of the longer path from one of the bodies to the ancestor.
  @see FindPathsFromFirstCommonAncestor() */

typo?

Suggestion:

 FindPathsToFirstCommonAncestor

multibody/topology/spanning_forest.h line 177 at r2 (raw file):

  Complexity is O(ℓ) where ℓ is the length of the longer path from one of the
  bodies to the ancestor. If either or both given Mobods are the ancestor then
  one or both returned paths will be empty.

BTW, I can't say for sure from the documentation whether mobod1_index and mobod2_index will be included in the returned paths. From the code, it looks like, in most cases, they will be in the paths, except in the case that either or both are the ancestor.

Would this description help?

Suggestion:

  bodies to the ancestor. Each of the given Mobods will be included in
  its returned path except when it is the ancestor, and its path
  will be empty.

@sherm1 sherm1 force-pushed the misc_algorithms_for_topology branch from bd38268 to 3fab625 Compare August 15, 2024 23:14
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Damrong -- responses for the first batch of comments.

Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers


multibody/topology/link_joint_graph.cc line 791 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, would "if-else" be easier to read?

Done. Much better, thanks!


multibody/topology/link_joint_graph.cc line 842 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, is the & to mark output parameter, or &* a typo?

Yes, I prefer that ugly combo to leaving the output parameter unmarked.


multibody/topology/spanning_forest.h line 162 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Question. From the code, I see it returns the common ancestor with the largest value of level(). Does highest-numbered in the doc mean the "largest value of level()" ? Or it means the "largest value of MobodIndex" ? or both? Same question for the next function FindPathsToFirstCommonAncestor.

Done. Good question. The SpanningForest is required (and documented in the intro material) to use depth-first numbering for the mobods. Hence the MobodIndex values on any branch increase by 1 as the level increases by 1. So the ancestor with the highest level is also the ancestor with the highest index.

I added a note to the documentation of both functions.


multibody/topology/spanning_forest.h line 165 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

typo?

Yikes, thanks. Done.


multibody/topology/spanning_forest.h line 177 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, I can't say for sure from the documentation whether mobod1_index and mobod2_index will be included in the returned paths. From the code, it looks like, in most cases, they will be in the paths, except in the case that either or both are the ancestor.

Would this description help?

Done

Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2nd checkpoint. I've finished reviewing link_joint_graph_test.cc. I'm in the middle of spanning_forest_test.cc. I'll continue later.

Reviewed 1 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers

a discussion (no related file):
Discussion. Right now (r3), the new tests in link_joint_graph_test.cc are in the WorldOnlyTest. I think we might need a larger graph (perhaps with two or three nodes) for some functions. For example, if I had maliciously changed while to if in this code, it would still pass the tests.

diff --git a/multibody/topology/link_joint_graph.cc b/multibody/topology/link_joint_graph.cc
index 56ec056ec1..cc37f97284 100644
--- a/multibody/topology/link_joint_graph.cc
+++ b/multibody/topology/link_joint_graph.cc
@@ -712,7 +712,7 @@ std::vector<LinkIndex> LinkJointGraph::FindPathFromWorld(
   const SpanningForest::Mobod* mobod =
       &forest().mobods()[link_to_mobod(link_index)];
   std::vector<LinkIndex> path(mobod->level() + 1);
-  while (mobod->inboard().is_valid()) {
+  if (mobod->inboard().is_valid()) {
     const Link& link = links(mobod->link_ordinal());
     path[mobod->level()] = link.index();  // Active Link if composite.
     mobod = &forest().mobods(mobod->inboard());

On the other hand, I don't want a large test that is difficult to maintain. I'd be fine with selecting only a few functions for more testing. Some functions in spanning_forest_test.cc already tested larger graphs, so we don't need to test them again in link_joint_graph_test.cc.



multibody/topology/test/link_joint_graph_test.cc line 280 at r2 (raw file):

  EXPECT_EQ(ssize(graph.CalcSubgraphsOfWeldedLinks()), 1);
  EXPECT_EQ(graph.CalcSubgraphsOfWeldedLinks()[0],
            std::set<LinkIndex>{world_link_index});

BTW, could we check it in one statement instead of two statements?

Suggestion:

  EXPECT_EQ(graph.CalcSubgraphsOfWeldedLinks(),
            std::vector<std::set<LinkIndex>>{{world_link_index}});

multibody/topology/test/link_joint_graph_test.cc line 313 at r2 (raw file):

  auto world_path = graph.FindPathFromWorld(world_link_index);
  EXPECT_EQ(ssize(world_path), 1);  // Just World.
  EXPECT_EQ(world_path[0], world_link_index);

BTW, shorten to one statement?

Suggestion:

  EXPECT_EQ(graph.FindPathFromWorld(world_link_index),
            std::vector<LinkIndex>{world_link_index});

multibody/topology/test/link_joint_graph_test.cc line 318 at r2 (raw file):

  auto subtree_links = graph.FindSubtreeLinks(world_link_index);
  EXPECT_EQ(ssize(subtree_links), 1);
  EXPECT_EQ(subtree_links[0], world_link_index);

BTW, shorten to one statement?

Suggestion:

  EXPECT_EQ(graph.FindSubtreeLinks(world_link_index),
            std::vector<LinkIndex>{world_link_index});

multibody/topology/test/link_joint_graph_test.cc line 321 at r2 (raw file):

  EXPECT_EQ(ssize(graph.GetSubgraphsOfWeldedLinks()), 1);
  EXPECT_EQ(graph.GetSubgraphsOfWeldedLinks()[0],
            std::set<LinkIndex>{world_link_index});

BTW, shorten to one statement?

Suggestion:

  EXPECT_EQ(graph.GetSubgraphsOfWeldedLinks(),
            std::vector<std::set<LinkIndex>>{{world_link_index}});

multibody/topology/test/spanning_forest_test.cc line 90 at r3 (raw file):

  auto world_mobod_path = forest.FindPathFromWorld(world_mobod_index);
  EXPECT_EQ(ssize(world_mobod_path), 1);  // Just World.
  EXPECT_EQ(world_mobod_path[0], world_mobod_index);

BTW, shorten to one statement?

Suggestion:

  EXPECT_EQ(forest.FindPathFromWorld(world_mobod_index),
            std::vector<MobodIndex>{world_mobod_index});

multibody/topology/test/spanning_forest_test.cc line 93 at r3 (raw file):

  auto subtree_links = forest.FindSubtreeLinks(world_mobod_index);
  EXPECT_EQ(ssize(subtree_links), 1);
  EXPECT_EQ(subtree_links[0], world_link_index);

BTW, shorten to one statement?

Suggestion:

  EXPECT_EQ(forest.FindSubtreeLinks(world_mobod_index),
            std::vector<LinkIndex>{world_link_index});

multibody/topology/test/spanning_forest_test.cc line 746 at r3 (raw file):

      (std::vector{LinkIndex(0), LinkIndex(1), LinkIndex(2), LinkIndex(3),
                   LinkIndex(4), LinkIndex(5), LinkIndex(7), LinkIndex(6),
                   LinkIndex(8), LinkIndex(11), LinkIndex(10), LinkIndex(9)}));

I'm surprised to see the brackets (std::vector...), and when I removed them, indeed, I got compiler errors.

There's nothing to change here. I'm just asking for my education.

diff --git a/multibody/topology/test/spanning_forest_test.cc b/multibody/topology/test/spanning_forest_test.cc
index 07c471d876..7ed1796212 100644
--- a/multibody/topology/test/spanning_forest_test.cc
+++ b/multibody/topology/test/spanning_forest_test.cc
@@ -741,9 +741,9 @@ GTEST_TEST(SpanningForest, SerialChainAndMore) {
 
   EXPECT_EQ(
       graph.FindSubtreeLinks(graph.world_link().index()),
-      (std::vector{LinkIndex(0), LinkIndex(1), LinkIndex(2), LinkIndex(3),
+      std::vector{LinkIndex(0), LinkIndex(1), LinkIndex(2), LinkIndex(3),
                    LinkIndex(4), LinkIndex(5), LinkIndex(7), LinkIndex(6),
-                   LinkIndex(8), LinkIndex(11), LinkIndex(10), LinkIndex(9)}));
+                   LinkIndex(8), LinkIndex(11), LinkIndex(10), LinkIndex(9)});
 
   // Counts for generic middle Mobod.
   const SpanningForest::Mobod& mobod_for_link3 =
multibody/topology/test/spanning_forest_test.cc:746:77: error: macro "EXPECT_EQ" passed 13 arguments, but takes just 2
  746 |                    LinkIndex(8), LinkIndex(11), LinkIndex(10), LinkIndex(9)});
      |                                                                             ^
In file included from external/gtest/googlemock/include/gmock/internal/gmock-internal-utils.h:51,
                 from external/gtest/googlemock/include/gmock/gmock-actions.h:146,
                 from external/gtest/googlemock/include/gmock/gmock.h:56,
                 from multibody/topology/test/spanning_forest_test.cc:9:
external/gtest/googletest/include/gtest/gtest.h:1884: note: macro "EXPECT_EQ" defined here
 1884 | #define EXPECT_EQ(val1, val2) \
      | 
multibody/topology/test/spanning_forest_test.cc: In member function 'virtual void drake::multibody::internal::{anonymous}::SpanningForest_SerialChainAndMore_Test::TestBody()':
multibody/topology/test/spanning_forest_test.cc:742:3: error: 'EXPECT_EQ' was not declared in this scope
  742 |   EXPECT_EQ(
      |   ^~~~~~~~~

Code quote:

      (std::vector{LinkIndex(0), LinkIndex(1), LinkIndex(2), LinkIndex(3),
                   LinkIndex(4), LinkIndex(5), LinkIndex(7), LinkIndex(6),
                   LinkIndex(8), LinkIndex(11), LinkIndex(10), LinkIndex(9)}));

multibody/topology/test/spanning_forest_test.cc line 1054 at r3 (raw file):

  expected_before_subgraphs.push_back(std::set{LinkIndex(3)});
  expected_before_subgraphs.push_back(std::set{LinkIndex(9)});
  expected_before_subgraphs.push_back(std::set{LinkIndex(11)});

BTW, consider list-initialization instead of push_back, so it can be const.

Suggestion:

  const std::vector<std::set<LinkIndex>> expected_before_subgraphs{
      // {0 5 7 12} {1 4 13} {6 8 10} {2} {3} {9} {11}  (see first drawing
      // above)
      std::set{LinkIndex(0), LinkIndex(5), LinkIndex(7), LinkIndex(12)},
      std::set{LinkIndex(1), LinkIndex(4), LinkIndex(13)},
      std::set{LinkIndex(6), LinkIndex(8), LinkIndex(10)},
      std::set{LinkIndex(2)},
      std::set{LinkIndex(3)},
      std::set{LinkIndex(9)},
      std::set{LinkIndex(11)}};

@sherm1 sherm1 force-pushed the misc_algorithms_for_topology branch from 3fab625 to fbb2483 Compare August 17, 2024 19:14
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 8 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers

a discussion (no related file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Discussion. Right now (r3), the new tests in link_joint_graph_test.cc are in the WorldOnlyTest. I think we might need a larger graph (perhaps with two or three nodes) for some functions. For example, if I had maliciously changed while to if in this code, it would still pass the tests.

diff --git a/multibody/topology/link_joint_graph.cc b/multibody/topology/link_joint_graph.cc
index 56ec056ec1..cc37f97284 100644
--- a/multibody/topology/link_joint_graph.cc
+++ b/multibody/topology/link_joint_graph.cc
@@ -712,7 +712,7 @@ std::vector<LinkIndex> LinkJointGraph::FindPathFromWorld(
   const SpanningForest::Mobod* mobod =
       &forest().mobods()[link_to_mobod(link_index)];
   std::vector<LinkIndex> path(mobod->level() + 1);
-  while (mobod->inboard().is_valid()) {
+  if (mobod->inboard().is_valid()) {
     const Link& link = links(mobod->link_ordinal());
     path[mobod->level()] = link.index();  // Active Link if composite.
     mobod = &forest().mobods(mobod->inboard());

On the other hand, I don't want a large test that is difficult to maintain. I'd be fine with selecting only a few functions for more testing. Some functions in spanning_forest_test.cc already tested larger graphs, so we don't need to test them again in link_joint_graph_test.cc.

Done, thanks! I added the missing test and it revealed a bug -- I had forgotten to account for static links in the "Calc" methods that don't have access to a built forest.


@sherm1 sherm1 force-pushed the misc_algorithms_for_topology branch from fbb2483 to 7a94a19 Compare August 17, 2024 21:17
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All comments so far addressed. I found a few more places to reduce the number of lines in the tests, thanks for that idea.

Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers


multibody/topology/test/link_joint_graph_test.cc line 280 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, could we check it in one statement instead of two statements?

Done


multibody/topology/test/link_joint_graph_test.cc line 313 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, shorten to one statement?

Done


multibody/topology/test/link_joint_graph_test.cc line 318 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, shorten to one statement?

Done


multibody/topology/test/link_joint_graph_test.cc line 321 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, shorten to one statement?

Done


multibody/topology/test/spanning_forest_test.cc line 90 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, shorten to one statement?

Done


multibody/topology/test/spanning_forest_test.cc line 93 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, shorten to one statement?

Done


multibody/topology/test/spanning_forest_test.cc line 746 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

I'm surprised to see the brackets (std::vector...), and when I removed them, indeed, I got compiler errors.

There's nothing to change here. I'm just asking for my education.

diff --git a/multibody/topology/test/spanning_forest_test.cc b/multibody/topology/test/spanning_forest_test.cc
index 07c471d876..7ed1796212 100644
--- a/multibody/topology/test/spanning_forest_test.cc
+++ b/multibody/topology/test/spanning_forest_test.cc
@@ -741,9 +741,9 @@ GTEST_TEST(SpanningForest, SerialChainAndMore) {
 
   EXPECT_EQ(
       graph.FindSubtreeLinks(graph.world_link().index()),
-      (std::vector{LinkIndex(0), LinkIndex(1), LinkIndex(2), LinkIndex(3),
+      std::vector{LinkIndex(0), LinkIndex(1), LinkIndex(2), LinkIndex(3),
                    LinkIndex(4), LinkIndex(5), LinkIndex(7), LinkIndex(6),
-                   LinkIndex(8), LinkIndex(11), LinkIndex(10), LinkIndex(9)}));
+                   LinkIndex(8), LinkIndex(11), LinkIndex(10), LinkIndex(9)});
 
   // Counts for generic middle Mobod.
   const SpanningForest::Mobod& mobod_for_link3 =
multibody/topology/test/spanning_forest_test.cc:746:77: error: macro "EXPECT_EQ" passed 13 arguments, but takes just 2
  746 |                    LinkIndex(8), LinkIndex(11), LinkIndex(10), LinkIndex(9)});
      |                                                                             ^
In file included from external/gtest/googlemock/include/gmock/internal/gmock-internal-utils.h:51,
                 from external/gtest/googlemock/include/gmock/gmock-actions.h:146,
                 from external/gtest/googlemock/include/gmock/gmock.h:56,
                 from multibody/topology/test/spanning_forest_test.cc:9:
external/gtest/googletest/include/gtest/gtest.h:1884: note: macro "EXPECT_EQ" defined here
 1884 | #define EXPECT_EQ(val1, val2) \
      | 
multibody/topology/test/spanning_forest_test.cc: In member function 'virtual void drake::multibody::internal::{anonymous}::SpanningForest_SerialChainAndMore_Test::TestBody()':
multibody/topology/test/spanning_forest_test.cc:742:3: error: 'EXPECT_EQ' was not declared in this scope
  742 |   EXPECT_EQ(
      |   ^~~~~~~~~

That's because of the way macros deal with arguments. In this case EXPECT_EQ thinks arguments are separated by commas. The parentheses are needed to make it think of that comma separated list as just one object rather than as a bunch of macro arguments.


multibody/topology/test/spanning_forest_test.cc line 1054 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, consider list-initialization instead of push_back, so it can be const.

Done. So much nicer! Doesn't even need the std::sets.

Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 2 files at r4, 1 of 2 files at r5, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers


multibody/topology/test/spanning_forest_test.cc line 746 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

That's because of the way macros deal with arguments. In this case EXPECT_EQ thinks arguments are separated by commas. The parentheses are needed to make it think of that comma separated list as just one object rather than as a bunch of macro arguments.

Thank you for your explanation.

Copy link
Contributor

@DamrongGuoy DamrongGuoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r5.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Damrong! +@rpoyner-tri for platform review per revised schedule, please

Reviewable status: LGTM missing from assignee rpoyner-tri(platform)

@sherm1 sherm1 force-pushed the misc_algorithms_for_topology branch from 7a94a19 to e5a33fa Compare August 19, 2024 18:11
Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: some test coverage questiions

Reviewed 1 of 6 files at r1, 1 of 2 files at r2, 1 of 2 files at r3, 1 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 2 unresolved discussions


multibody/topology/spanning_forest.cc line 1032 at r5 (raw file):

  if (mobod1_index == world_mobod_index() ||
      mobod2_index == world_mobod_index()) {
    return world_mobod_index();

nit this branch is unreached by unit tests. Should I worry?


multibody/topology/spanning_forest.cc line 1074 at r5 (raw file):

  }
  while (branch2->level() > branch1->level()) {
    path2->push_back(branch2->index());

nit this branch is unreached by unit tests. Should I worry?

@sherm1 sherm1 force-pushed the misc_algorithms_for_topology branch from e5a33fa to 52d1ba1 Compare August 19, 2024 20:52
Copy link
Member Author

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing tests added, thanks.

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),DamrongGuoy


multibody/topology/spanning_forest.cc line 1032 at r5 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit this branch is unreached by unit tests. Should I worry?

Done


multibody/topology/spanning_forest.cc line 1074 at r5 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit this branch is unreached by unit tests. Should I worry?

Done

@sherm1 sherm1 merged commit 8535cc8 into RobotLocomotion:master Aug 19, 2024
9 checks passed
RussTedrake pushed a commit to RussTedrake/drake that referenced this pull request Dec 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants