Refactors SAP solver implementation #21414

amcastro-tri · 2024-05-08T19:33:11Z

In preparation for the support of gradients through SAP, see #21370, this PR refactors the implementation of the solver so that common data and implementation can be used when propagating gradients.

There are no functional nor behavior changes.

There are essentially two changes:

struct SolverStats is no longer nested in SapSolver but is now refactored to SapSolverStats.
SapSolver::model_ is not longer a member. A SapModel is now created with each call to SolveWithGuess. We pass "model" as needed by argument.

This change is

amcastro-tri

+@SeanCurtis-TRI for feature review please

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

amcastro-tri

-@SeanCurtis-TRI since he's out of office

+@sammy-tri for feature review please

Reviewable status: LGTM missing from assignee sammy-tri(platform), needs at least two assigned reviewers

amcastro-tri

sorry, Sammy is also out.
-@sammy-tri
+@jwnimmer-tri for feature review please

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

jwnimmer-tri

feature.

+@sherm1 for platform review per schedule, please.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee sherm1(platform)

-- commits line 7 at r1:
nit Please keep commit messages to <80 cols for sure (<72 is conventional).

multibody/contact_solvers/sap/sap_solver.h line 185 at r1 (raw file):

// Struct used to store SAP solver statistics.
struct SapSolverStats {

BTW You have types named SapSolverStatus and SapSolverStats now, different by only 1 letter. That will probably be fine, but be aware that you've made names that might confuse (or not) at some point; I guess we'll see.

multibody/contact_solvers/sap/sap_solver.cc line 100 at r1 (raw file):

  throw std::logic_error(
      "SapSolver::SolveWithGuess(): Only T = double is supported when the set "
      "of constraints is non-empty.");

We've removed some message text here, but it seems to me like the text is still both true and important to retain for clarity. The "when" part is an important part of the support conditions to explain to a user.

multibody/contact_solvers/sap/sap_solver.cc line 542 at r1 (raw file):

  // N.B. We then define f(alpha) = −ℓ'(α)/ℓ'₀ so that f(alpha=0) = -1.
  const double dell_scale = -dell_dalpha0;
  EvalData data{*this,    model,     context, search_direction_data,

nit Weird whitespace. I wonder why clang-format isn't auto-fixing this for you.

multibody/contact_solvers/sap/test/sap_solver_test.cc line 855 at r1 (raw file):

                               const VectorXd& v_guess) const {
    // Verify Hessian obtained with sparse supernodal algebra.
    auto model = std::make_unique<SapModel<double>>(sap_problem_.get());

nit No reason to use a unique_ptr here, right? I could just be:

SapModel<double> model(sap_problem_.get());

amcastro-tri

Reviewable status: 5 unresolved discussions, LGTM missing from assignee sherm1(platform)

multibody/contact_solvers/sap/sap_solver.h line 185 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW You have types named SapSolverStatus and SapSolverStats now, different by only 1 letter. That will probably be fine, but be aware that you've made names that might confuse (or not) at some point; I guess we'll see.

Good point. What do u think about SapStatistics? that's different enough and I think still a good name for this struct

jwnimmer-tri

Reviewable status: 5 unresolved discussions, LGTM missing from assignee sherm1(platform)

multibody/contact_solvers/sap/sap_solver.h line 185 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

Good point. What do u think about SapStatistics? that's different enough and I think still a good name for this struct

That's fine by me.

sherm1

Platform pending resolution of feature review comments.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions

multibody/contact_solvers/sap/sap_solver.h line 185 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

That's fine by me.

I like SapStatistics too. Or maybe Saptistics :)

gradients. There are essentially two changes: 1. struct SolverStats is no longer nested in SapSolver but is now refactored to SapSolverStats. 2. SapSolver<T>::model_ is not longer a member. A SapModel is now created with each call to SolveWithGuess. We pass "model" as needed by argument.

amcastro-tri

Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),sherm1(platform)

multibody/contact_solvers/sap/sap_solver.cc line 542 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Weird whitespace. I wonder why clang-format isn't auto-fixing this for you.

weird indeed. I edited by hand, and the linter complained. So I run the suggested command /usr/bin/clang-format-15 -style=file -i multibody/contact_solvers/sap/sap_solver.cc and it brought this line back to this.
Somehow the linter prefers this.

multibody/contact_solvers/sap/test/sap_solver_test.cc line 855 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit No reason to use a unique_ptr here, right? I could just be:
SapModel<double> model(sap_problem_.get());

true. Done.

jwnimmer-tri

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),sherm1(platform)

multibody/contact_solvers/sap/sap_solver.cc line 542 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

weird indeed. I edited by hand, and the linter complained. So I run the suggested command /usr/bin/clang-format-15 -style=file -i multibody/contact_solvers/sap/sap_solver.cc and it brought this line back to this.
Somehow the linter prefers this.

Yes indeed. Looking more closely now, I guess it's vertically aligning model with dell_scale on the next line.

sherm1

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),sherm1(platform)

jwnimmer-tri · 2024-05-15T18:00:54Z

multibody/contact_solvers/sap/sap_solver.h line 308 at r2 (raw file):

  // @pre context is not nullptr.
  // @pre context was created via a call to model.MakeContext().
  SapSolverStatus SolveWithGuess(const SapModel<T>& model,

Sorry this slipped by me during review... maybe you can fix in the next PR in the train.

Overloading the same name for both the public function and the private helper function is extremely confusing to figure out at any given call site which one is which, or in the implementations which one is which. This function should be renamed to something like SolveWithGuessImpl or whatever.

…ocomotion#21414) There are essentially two changes: 1. struct SolverStats is no longer nested in SapSolver but is now refactored to SapSolverStats. 2. SapSolver<T>::model_ is not longer a member. A SapModel is now created with each call to SolveWithGuess. We pass "model" as needed by argument.

amcastro-tri added release notes: none This pull request should not be mentioned in the release notes priority: high labels May 8, 2024

amcastro-tri force-pushed the sap_solver_refactoring branch from a4fb0c0 to 2abd0df Compare May 8, 2024 20:04

amcastro-tri assigned SeanCurtis-TRI May 8, 2024

amcastro-tri commented May 8, 2024

View reviewed changes

amcastro-tri assigned sammy-tri and unassigned SeanCurtis-TRI May 9, 2024

amcastro-tri commented May 9, 2024

View reviewed changes

amcastro-tri assigned jwnimmer-tri and unassigned sammy-tri May 9, 2024

amcastro-tri commented May 9, 2024

View reviewed changes

jwnimmer-tri assigned sherm1 May 9, 2024

jwnimmer-tri reviewed May 9, 2024

View reviewed changes

amcastro-tri commented May 9, 2024

View reviewed changes

jwnimmer-tri reviewed May 9, 2024

View reviewed changes

sherm1 reviewed May 9, 2024

View reviewed changes

amcastro-tri force-pushed the sap_solver_refactoring branch from 2abd0df to 39fb603 Compare May 13, 2024 13:49

amcastro-tri commented May 13, 2024

View reviewed changes

jwnimmer-tri reviewed May 13, 2024

View reviewed changes

jwnimmer-tri merged commit e94c357 into RobotLocomotion:master May 13, 2024
10 checks passed

sherm1 reviewed May 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors SAP solver implementation #21414

Refactors SAP solver implementation #21414

amcastro-tri commented May 8, 2024 •

edited by sherm1

Loading

amcastro-tri left a comment

amcastro-tri left a comment

amcastro-tri left a comment

jwnimmer-tri left a comment

amcastro-tri left a comment

jwnimmer-tri left a comment

sherm1 left a comment

amcastro-tri left a comment

jwnimmer-tri left a comment

sherm1 left a comment

jwnimmer-tri commented May 15, 2024

Refactors SAP solver implementation #21414

Refactors SAP solver implementation #21414

Conversation

amcastro-tri commented May 8, 2024 • edited by sherm1 Loading

amcastro-tri left a comment

Choose a reason for hiding this comment

amcastro-tri left a comment

Choose a reason for hiding this comment

amcastro-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

amcastro-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

sherm1 left a comment

Choose a reason for hiding this comment

amcastro-tri left a comment

Choose a reason for hiding this comment

jwnimmer-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 May 15, 2024

amcastro-tri commented May 8, 2024 •

edited by sherm1

Loading