[CENIC] ICF params store minimum (fixed-step) tolerance

[vincekurtz]

Addresses trailing issue from #23747 (#23782).

Now:

• The IcfSolverParams.min_tolerance parameter does influence error control mode, as a lower bound on kappa * accuracy
• The actual tolerance is passed to SolveWithGuess, so the integrator doesn't change the parameters as it goes

---

This change is 

[vincekurtz]

+a:@amcastro-tri for mini-review please!

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

[amcastro-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @vincekurtz)

---

multibody/contact_solvers/icf/icf_solver.h line 34 at r1 (raw file):

  This provides a lower bound on the actual tolerance used, which is specified
  explicitly when calling the solver. CENIC uses this minimum tolerance in fixed
  step mode, and relaxes it in error-controlled mode based on the desired

I see what your arer trying to do. Consider this lands in Drake.
SInce you want to allow users to specify IcSolverfParams for CenicIntegrator, that means that this class, IcSolverfParms is public. However, this whole thing is internal::.
How do you consolidate that? Maybe we consider the ConvexIntegrator::set_parameters() and internal::) API ony? But you can see how this still has the same problems. Obviously it gets the job done, but from an API design perspective something went wrong.

I do like this, and I think we should merge it. But still, I believe we might want a separate public struct for CenicIntegrator params.
WDYT?

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @vincekurtz)

---

multibody/contact_solvers/icf/icf_solver.h line 34 at r1 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

I see what your arer trying to do. Consider this lands in Drake.
SInce you want to allow users to specify IcSolverfParams for CenicIntegrator, that means that this class, IcSolverfParms is public. However, this whole thing is internal::.
How do you consolidate that? Maybe we consider the ConvexIntegrator::set_parameters() and internal::) API ony? But you can see how this still has the same problems. Obviously it gets the job done, but from an API design perspective something went wrong.

I do like this, and I think we should merge it. But still, I believe we might want a separate public struct for CenicIntegrator params.
WDYT?

The struct that's used by end-users should be defined its own separate header file, in a non-internal namespace.

[vincekurtz]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @vincekurtz)

---

multibody/contact_solvers/icf/icf_solver.h line 34 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The struct that's used by end-users should be defined its own separate header file, in a non-internal namespace.

I think we should expose all of these parameters to the end user, so how about we just make IcfSolverParameters public (move to a separate file, namespace, etc.)?

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @vincekurtz)

---

multibody/contact_solvers/icf/icf_solver.h line 34 at r1 (raw file):

Previously, vincekurtz (Vince Kurtz) wrote…

I think we should expose all of these parameters to the end user, so how about we just make IcfSolverParameters public (move to a separate file, namespace, etc.)?

Perhaps to state the obvious, but yes I agree. This is waiting on @amcastro-tri to weigh in.

[vincekurtz]

multibody/contact_solvers/icf/icf_solver.h line 34 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Perhaps to state the obvious, but yes I agree. This is waiting on @amcastro-tri to weigh in.

I just went ahead and moved it. Should this have doxygen-style comments (///, /**) now that it's public?

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee amcastro-tri, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @vincekurtz)

---

multibody/contact_solvers/icf/icf_solver.h line 34 at r1 (raw file):

Previously, vincekurtz (Vince Kurtz) wrote…

I just went ahead and moved it. Should this have doxygen-style comments (///, /**) now that it's public?

Yes; typically /**.