[multibody] Adds python bindings for MultibodyPlant::AddTendonConstraint()

[joemasterjohn]

Towards #22664.

---

This change is 

[joemasterjohn]

+@rpoyner-tri for feature review, please.

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

[rpoyner-tri]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: needs at least two assigned reviewers

[joemasterjohn]

Thanks, Rico!

+@sammy-tri for platform review, please.

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

[joemasterjohn]

-@sammy-tri +@SeanCurtis-TRI for platform review, please.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform)

[SeanCurtis-TRI]

Consistency between C++ and python issues.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform)

---

bindings/pydrake/multibody/plant_py.cc line 303 at r1 (raw file):

            py_rvp::reference_internal, cls_doc.AddWeldConstraint.doc)
        .def("AddTendonConstraint", &Class::AddTendonConstraint,
            py::arg("joints"), py::arg("a"), py::arg("offset") = std::nullopt,

nit The C++ api doesn't have default values. Yes, they are declared std::optional, but the invocation requires passing a std::nullopt explicitly (even if we do it via the expedient of {}). The fact that the python bindings has default values makes it inconsistent.

I can see the motivation to do it this way. Given parameter name invocation, you can omit the ones you're not sitting, rather than having to declare None on each of them. But it feels like this disagreement is unprecedented. (Certainly not in this file.)

For example, the AddBallConstraint() has the defaulted parameter p_BQ in both python and C++. Taking it a step further, every use of std::nullopt in this file is a default parameter value and correlates with the identical semantics in C++.

I think we need to unify them. The simplest solution is to remove all of the parameters, but that makes your call sites more verbose. Alternatively, the C++ gets default parameters to match. That seems relatively harmless; you've already got the C++ implementation handling the default nullopt value, so it doesn't matter whether the user passed it explicitly or via default.

---

bindings/pydrake/multibody/test/plant_test.py line 2767 at r1 (raw file):

                damping=0.))

        plant.AddTendonConstraint(

BTW There are probably mixed opinions on this front, but the bindings code has declared that some parameters have default values. I generally tend to advocate for tests pinning that down.

Perhaps add two constraints? Partition the optional parameters into two disjoint sets. One tendon uses one set of constraints, the other uses the other set. Then you simultaneously show that all the keywords work and all the omitted keywords also work.

However, your out is that I recognize that regression tests on the default values in the bindings is hardly a Drake-wide standard. So, if you want to eschew that, I defer to author preference.

[joemasterjohn]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform)

---

bindings/pydrake/multibody/plant_py.cc line 303 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit The C++ api doesn't have default values. Yes, they are declared std::optional, but the invocation requires passing a std::nullopt explicitly (even if we do it via the expedient of {}). The fact that the python bindings has default values makes it inconsistent.

I can see the motivation to do it this way. Given parameter name invocation, you can omit the ones you're not sitting, rather than having to declare None on each of them. But it feels like this disagreement is unprecedented. (Certainly not in this file.)

For example, the AddBallConstraint() has the defaulted parameter p_BQ in both python and C++. Taking it a step further, every use of std::nullopt in this file is a default parameter value and correlates with the identical semantics in C++.

I think we need to unify them. The simplest solution is to remove all of the parameters, but that makes your call sites more verbose. Alternatively, the C++ gets default parameters to match. That seems relatively harmless; you've already got the C++ implementation handling the default nullopt value, so it doesn't matter whether the user passed it explicitly or via default.

I'm up for adding the defaults to the C++ API. It's a bit murky to me where that stands vis-a-vis the stable API. Is adding new default nullopts for optional arguments just a behavioral change or something that requires deprecation.

[SeanCurtis-TRI]

bindings/pydrake/multibody/plant_py.cc line 303 at r1 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

I'm up for adding the defaults to the C++ API. It's a bit murky to me where that stands vis-a-vis the stable API. Is adding new default nullopts for optional arguments just a behavioral change or something that requires deprecation.

Given the last PR just merged, you can do an immediate follow up within the same release without infringing on our stability guarantees.

[SeanCurtis-TRI]

bindings/pydrake/multibody/plant_py.cc line 303 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Given the last PR just merged, you can do an immediate follow up within the same release without infringing on our stability guarantees.

(In fact, the time frame in which stability applies to new APIs is even looser than that. So, you're amply safe.)

[joemasterjohn]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform)

---

bindings/pydrake/multibody/plant_py.cc line 303 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

(In fact, the time frame in which stability applies to new APIs is even looser than that. So, you're amply safe.)

Ok, then maybe you could weigh in on possibly rearranging the order of the arguments as well. It was kind of touched on in the last PR, but the requirement that either lower_limit or upper_limit is finite makes things a little awkward in that a call to the API with all of the optional parameters throws:

plant.AddTendonConstraint(joints, a, {}, {}, {}, {}, {}); // Throws because lower and upper are both infinite.

There's an awkwardness because of lower and upper being optional is not mutually exclusive. If we add default nullopts then calling with defaults results in a throw:

plant.AddTendonConstraint(joints, a); // Throws because lower and upper are both infinite.

That forces the user to always specify a value for either lower_limit or upper_limit (and consequently offset because it comes before in the argument list. Very awkward. Obviously, that's a design flaw on my part. Thinking forward, it would be slightly less awkward if lower_limit and upper_limit came directly after joints and a in the arguments list. Making a valid default call:

plant.AddTendonConstraint(joints, a, lower);

or

plant.AddTendonConstraint(joints, a, {}, upper);

Maybe that is just indicating that lower/upper should not be optional parameters in the first place. IDK, WDYT?

[SeanCurtis-TRI]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform)

---

bindings/pydrake/multibody/plant_py.cc line 303 at r1 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Ok, then maybe you could weigh in on possibly rearranging the order of the arguments as well. It was kind of touched on in the last PR, but the requirement that either lower_limit or upper_limit is finite makes things a little awkward in that a call to the API with all of the optional parameters throws:

plant.AddTendonConstraint(joints, a, {}, {}, {}, {}, {}); // Throws because lower and upper are both infinite.

There's an awkwardness because of lower and upper being optional is not mutually exclusive. If we add default nullopts then calling with defaults results in a throw:

plant.AddTendonConstraint(joints, a); // Throws because lower and upper are both infinite.

That forces the user to always specify a value for either lower_limit or upper_limit (and consequently offset because it comes before in the argument list. Very awkward. Obviously, that's a design flaw on my part. Thinking forward, it would be slightly less awkward if lower_limit and upper_limit came directly after joints and a in the arguments list. Making a valid default call:

plant.AddTendonConstraint(joints, a, lower);

or

plant.AddTendonConstraint(joints, a, {}, upper);

Maybe that is just indicating that lower/upper should not be optional parameters in the first place. IDK, WDYT?

1. Re-ordering is absolutely on the table.
2. Whether they are required or not, the user can just as easily pass nullopt to both on their way to setting stiffness which would create the same error.
3. Ultimately, I like them both being optional because at provides one avenue in which the invocation is more compact (lower limit). Every other invocation would be the same.
   • Although, maybe that is an argument for non-optional; a base invocation will always look the same: joints, lower, upper. Period.
4. Making them both optional facilitates the python bindings, and we can generally expect a lot of pydrake users.

If you like optional, I can support that with the following proposal:

• Make them both optional.
• Move them forward (as they are less optional than the other optional parameters).
• In the documentation, call this out with code samples. I.e., "how to set the upper limit".

If you want to make them required, I can support that as well. (And some code examples in the docs for this as well would also help mitigate the sting.)

[jwnimmer-tri]

Ping @joemasterjohn.