Deprecate CalcCenterOfMassPosition() in favor of CalcCenterOfMassPositionInWorld(). #14538

mitiguy · 2021-01-15T23:49:42Z

Resolves issue #14537

This change is

…tionInWorld().

mitiguy

Feature review +@EricCousineau-TRI
Hi Eric -- I was hoping you might review this hopefully straight-forward PR.
It is mostly a "cut and paste", with a small effect on python, so I was hoping you might take a look.

Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

EricCousineau-TRI

Yup, sounds good! Just needs Python deprecations.

Reviewed 8 of 9 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @mitiguy)

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

        // overload that (a) services MBP directly and (b) uses body
        // association that is less awkward than implicit BodyNodeIndex.
        .def("CalcCenterOfMassPositionInWorld",

This is not backwards compatible; it requires deprecation in Python too.

See:
https://drake.mit.edu/code_review_checklist.html#is-the-code-the-minimal-set-of-what-you-want

https://drake.mit.edu/doxygen_cxx/group__python__bindings.html#PydrakeDeprecation

mitiguy

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

bindings/pydrake/multibody/plant_py.cc, line 299 at r2 (raw file):

            py::arg("context"), py::arg("model_instances"),
            cls_doc.CalcCenterOfMassPosition.doc_2args)
#pragma GCC diagnostic pop

@EricCousineau-TRI

I looked at the deprecation_example, but it was not immediately obvious how to apply it to what might be a simpler problem. Here is my first guess, but it does not pass CI and it is unclear how many guesses I will make before converging.

I just talked to Sherm and he said you might be able to help get me back on track.

Do you have a simple example in which a single method (rather than parts of an entire class) are deprecated? Or perhaps you can point me to a tiny PR that accomplishes what I am trying to do.

Note: Perhaps a simpler deprecation example would be helpful to understand what needs to be added to the file (perhaps some #includes) and the strategy to place some #pragmas, and any other pitfalls. It is difficult to see how this specific problem gets fixed as the example addresses a more complicated problem.

Once I get the simplest concept down, and see how to extend it a little, I usually can extrapolate from there. I have a more difficult time trying to a generalization and making it more specific. I blame my elementary math teachers who taught me how to add 3 + 5 rather than matrices -- hopefully your Mathworks background made you smile :).

EricCousineau-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

bindings/pydrake/multibody/plant_py.cc, line 299 at r2 (raw file):
Did you see these two examples and the preceding comments, and review the documentation that exists regarding docstrings (e.g. .doc_deprecated)?

drake/bindings/pydrake/common/test/deprecation_example/cc_module_py.cc

Lines 62 to 78 in f20d676

    
               // Bind deprecated overload. 
        
           #pragma GCC diagnostic push 
        
           #pragma GCC diagnostic ignored "-Wdeprecated-declarations" 
        
               cls.def("overload", 
        
                   WrapDeprecated(cls_doc.overload.doc_deprecated, 
        
                       py::overload_cast<int>(&ExampleCppClass::overload)), 
        
                   cls_doc.overload.doc_deprecated); 
        
           #pragma GCC diagnostic pop 
        
               // Bind entirely deprecated method. 
        
           #pragma GCC diagnostic push 
        
           #pragma GCC diagnostic ignored "-Wdeprecated-declarations" 
        
               cls.def("DeprecatedMethod", &ExampleCppClass::DeprecatedMethod, 
        
                   cls_doc.DeprecatedMethod.doc_deprecated); 
        
           #pragma GCC diagnostic pop 
        
               DeprecateAttribute( 
        
                   cls, "DeprecatedMethod", cls_doc.DeprecatedMethod.doc_deprecated);

[...] is unclear how many guesses I will make before converging.

The fact that your write "guess" given that docs and code exist is part of the problem ;)

But yeah, let's address the general readability of this example in a separate PR that I will start, and then you can take lessons learned and apply it here.

Sound good?

EricCousineau-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

bindings/pydrake/multibody/plant_py.cc, line 299 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Did you see these two examples and the preceding comments, and review the documentation that exists regarding docstrings (e.g. .doc_deprecated)?

drake/bindings/pydrake/common/test/deprecation_example/cc_module_py.cc

Lines 62 to 78 in f20d676

// Bind deprecated overload.

#pragma GCC diagnostic push

#pragma GCC diagnostic ignored "-Wdeprecated-declarations"

cls.def("overload",

WrapDeprecated(cls_doc.overload.doc_deprecated,

py::overload_cast<int>(&ExampleCppClass::overload)),

cls_doc.overload.doc_deprecated);

#pragma GCC diagnostic pop

// Bind entirely deprecated method.

#pragma GCC diagnostic push

#pragma GCC diagnostic ignored "-Wdeprecated-declarations"

cls.def("DeprecatedMethod", &ExampleCppClass::DeprecatedMethod,

cls_doc.DeprecatedMethod.doc_deprecated);

#pragma GCC diagnostic pop

DeprecateAttribute(

cls, "DeprecatedMethod", cls_doc.DeprecatedMethod.doc_deprecated);

[...] is unclear how many guesses I will make before converging.

The fact that your write "guess" given that docs and code exist is part of the problem ;)

But yeah, let's address the general readability of this example in a separate PR that I will start, and then you can take lessons learned and apply it here.

Sound good?

Correction: [...] given that docs, code, and compiler errors exist [...]

EricCousineau-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

bindings/pydrake/multibody/plant_py.cc, line 299 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Correction: [...] given that docs, code, and compiler errors exist [...]

See minimum-effort PR here for easier-to-cargo-cult examples:
#14546

EricCousineau-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

bindings/pydrake/multibody/plant_py.cc, line 299 at r2 (raw file):

hopefully your Mathworks background made you smile :)

And hehe, yup!

… MultibodyPlant::CalcCenterOfMassPosition().

mitiguy

@EricCousineau-TRI Thanks for taking time for the f2f. I updated multibody_plant.h.
Thanks for agreeing to showing me how to deprecate the API in the Python/C++ bindings.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

EricCousineau-TRI

Reviewed 1 of 9 files at r1, 1 of 2 files at r3.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

bindings/pydrake/multibody/plant_py.cc, line 299 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

hopefully your Mathworks background made you smile :)

And hehe, yup!

See PR that fixes the deprecation:
https://github.com/mitiguy/drake/pull/4/files

multibody/plant/test/multibody_plant_com_test.cc, line 24 at r3 (raw file):

  auto context_ = plant.CreateDefaultContext();
  DRAKE_EXPECT_THROWS_MESSAGE(
      plant.CalcCenterOfMassPositionInWorld(*context_),

This file still needs to do a basic test of the deprecated API.

Please test that the deprecated API still exists within a #pragma... block.

EricCousineau-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

bindings/pydrake/multibody/plant_py.cc, line 299 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

See PR that fixes the deprecation:
https://github.com/mitiguy/drake/pull/4/files

(You'll want to merge it in here, of course!)

EricCousineau-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

a discussion (no related file):
FYI Requires a deps update:
https://drake-cdash.csail.mit.edu/test/109660810

mitiguy

@EricCousineau-TRI
Ugh -- I am sure I am doing something silly, but this does not pass CI.
From the error stack on CI, it seems like the error points to:

File: plant_test.py, line 76 which is:
from pydrake.common.test_utilities.deprecation import catch_drake_warnings

ModuleNotFoundError: No module named 'pydrake.common.test_utilities.deprecation'

I looked for ModuleNotFoundError with Drake and found these issues -- but am unsure if they are relevant.
#11005
https://stackoverflow.com/questions/62292888/building-and-using-pydrake-from-source

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

multibody/plant/test/multibody_plant_com_test.cc, line 24 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This file still needs to do a basic test of the deprecated API.

Please test that the deprecated API still exists within a #pragma... block.

Done.

EricCousineau-TRI

Just missing Bazel deps:
61d53483f8#diff-e7881d2775f3e47f7b244280446b48d080e23710d79a218171114ad447578870R477

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

FYI Requires a deps update:
https://drake-cdash.csail.mit.edu/test/109660810

@mitiguy Did you see this comment?

Per prior PR:

EricCousineau-TRI

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @mitiguy)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

@mitiguy Did you see this comment?

Per prior PR:

OK Oops. Sorry, meant to strike out this message.

EricCousineau-TRI

Reviewed 3 of 3 files at r4.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mitiguy)

…is is probably insufficient/wrong.

mitiguy

@EricCousineau-TRI
This is nearing completion and may (or may not) just have a single typo.
Thank you for your help thus far. PTAL.

Reviewed 5 of 9 files at r1, 1 of 2 files at r3, 1 of 3 files at r4, 1 of 1 files at r5, 2 of 2 files at r6.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

bindings/pydrake/multibody/plant_py.cc, line 299 at r6 (raw file):

                overload_cast_explicit<Vector3<T>, const Context<T>&,
                    const std::vector<ModelInstanceIndex>&>(
                    &Class::CalcCenterOfMassPositionInWorld)),

@EricCousineau-TRI
I cut and paste from your very helpful example from:
https://github.com/mitiguy/drake/pull/4/files

I was wondering if the previous line should instead be:
&Class::CalcCenterOfMassPosition)),

EricCousineau-TRI

+@sammy-tri for platform review, please!

Reviewed 1 of 1 files at r5, 2 of 2 files at r6.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mitiguy and @sammy-tri)

bindings/pydrake/multibody/plant_py.cc, line 299 at r6 (raw file):

Previously, mitiguy (Mitiguy) wrote…

@EricCousineau-TRI
I cut and paste from your very helpful example from:
https://github.com/mitiguy/drake/pull/4/files

I was wondering if the previous line should instead be:
&Class::CalcCenterOfMassPosition)),

Ah, yup, that was a typo on my part! It should be the deprecated overload, yes.

mitiguy

@sammy-tri I fixed typo and if this passes CI, it is ready for your platform review.

Reviewed 1 of 1 files at r7.
Reviewable status: LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sammy-tri)

EricCousineau-TRI

Reviewed 1 of 1 files at r7.
Reviewable status: LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sammy-tri)

sammy-tri

Reviewed 5 of 9 files at r1, 1 of 2 files at r3, 1 of 3 files at r4, 2 of 2 files at r6, 1 of 1 files at r7.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

sammy-tri

+(status: squashing now)

Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),EricCousineau-TRI(platform)

…tionInWorld(). (RobotLocomotion#14538) Fixes RobotLocomotion#14537

jwnimmer-tri · 2021-05-16T14:36:47Z

Remember to round up all deprecation dates to the first of the month, otherwise we tend to miss when they come due (#15050).

EricCousineau-TRI · 2021-05-17T11:38:20Z

Ah, sorry for missing that!

Deprecate CalcCenterOfMassPosition() in favor of CalcCenterOfMassPosi…

e1dab8b

…tionInWorld().

mitiguy added type: cleanup unused team: dynamics priority: medium component: multibody plant MultibodyPlant and supporting code labels Jan 15, 2021

mitiguy mentioned this pull request Jan 15, 2021

Deprecate CalcCenterOfMassPosition() in favor of CalcCenterOfMassPositionInWorld(). #14537

Closed

mitiguy assigned EricCousineau-TRI Jan 16, 2021

mitiguy commented Jan 16, 2021

View reviewed changes

mitiguy mentioned this pull request Jan 16, 2021

Create missing method Multibody::CalcCenterOfMassTranslationalVelocity() #14268

Closed

EricCousineau-TRI reviewed Jan 16, 2021

View reviewed changes

First guess at deprecating in python.

b76d382

mitiguy commented Jan 19, 2021

View reviewed changes

EricCousineau-TRI reviewed Jan 20, 2021

View reviewed changes

EricCousineau-TRI mentioned this pull request Jan 20, 2021

pydrake: Make explicit / orthogonal unittest for catch_drake_warnings #14546

Merged

EricCousineau-TRI reviewed Jan 20, 2021

View reviewed changes

mitiguy added 2 commits January 21, 2021 12:17

Guess/try to fix CI error.

3c953ca

Add missing C++ DRAKE_DEPRECATED for one of two deprecated methods in…

d8612d3

… MultibodyPlant::CalcCenterOfMassPosition().

mitiguy commented Jan 21, 2021

View reviewed changes

EricCousineau-TRI mentioned this pull request Jan 21, 2021

fix deprecation mitiguy/drake#4

Closed

EricCousineau-TRI reviewed Jan 21, 2021

View reviewed changes

mitiguy added 2 commits January 22, 2021 14:58

Merge some of the changes suggested by Eric Cousineau.

f2b2f53

Add deprecation tests in C++. Attempt to update plant_py.cc to pass CI.

fb92c35

EricCousineau-TRI reviewed Jan 25, 2021

View reviewed changes

mitiguy commented Jan 25, 2021

View reviewed changes

EricCousineau-TRI reviewed Jan 25, 2021

View reviewed changes

Add dependency drake/common/drake_deprecated.h in plant_py.cc, but th…

de42df5

…is is probably insufficient/wrong.

rpoyner-tri mentioned this pull request Jan 25, 2021

Document deprecation tools and how-to #10605

Open

mitiguy added 2 commits January 25, 2021 19:26

Bad guesses at how to make this build.

cbeed67

With help from Sean Curtis and a few changes.

0ae86b8

mitiguy commented Jan 27, 2021

View reviewed changes

EricCousineau-TRI assigned sammy-tri Jan 27, 2021

EricCousineau-TRI reviewed Jan 27, 2021

View reviewed changes

Fix typo.

74b699d

mitiguy commented Jan 27, 2021

View reviewed changes

EricCousineau-TRI reviewed Jan 27, 2021

View reviewed changes

sammy-tri reviewed Jan 27, 2021

View reviewed changes

sammy-tri added the status: squashing now https://drake.mit.edu/reviewable.html#curated-commits label Jan 27, 2021

sammy-tri reviewed Jan 27, 2021

View reviewed changes

sammy-tri merged commit 0ac940e into RobotLocomotion:master Jan 27, 2021

josephsnyder pushed a commit to EricCousineau-TRI/drake that referenced this pull request Mar 16, 2021

Deprecate CalcCenterOfMassPosition() in favor of CalcCenterOfMassPosi…

3d19f25

…tionInWorld(). (RobotLocomotion#14538) Fixes RobotLocomotion#14537

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate CalcCenterOfMassPosition() in favor of CalcCenterOfMassPositionInWorld(). #14538

Deprecate CalcCenterOfMassPosition() in favor of CalcCenterOfMassPositionInWorld(). #14538

mitiguy commented Jan 15, 2021 •

edited by sherm1

Loading

mitiguy left a comment

EricCousineau-TRI left a comment

mitiguy left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

mitiguy left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

mitiguy left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

EricCousineau-TRI left a comment

mitiguy left a comment

EricCousineau-TRI left a comment

mitiguy left a comment

EricCousineau-TRI left a comment

sammy-tri left a comment

sammy-tri left a comment

jwnimmer-tri commented May 16, 2021

EricCousineau-TRI commented May 17, 2021

	// Bind deprecated overload.
	#pragma GCC diagnostic push
	#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
	cls.def("overload",
	WrapDeprecated(cls_doc.overload.doc_deprecated,
	py::overload_cast<int>(&ExampleCppClass::overload)),
	cls_doc.overload.doc_deprecated);
	#pragma GCC diagnostic pop

	// Bind entirely deprecated method.
	#pragma GCC diagnostic push
	#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
	cls.def("DeprecatedMethod", &ExampleCppClass::DeprecatedMethod,
	cls_doc.DeprecatedMethod.doc_deprecated);
	#pragma GCC diagnostic pop
	DeprecateAttribute(
	cls, "DeprecatedMethod", cls_doc.DeprecatedMethod.doc_deprecated);

Deprecate CalcCenterOfMassPosition() in favor of CalcCenterOfMassPositionInWorld(). #14538

Deprecate CalcCenterOfMassPosition() in favor of CalcCenterOfMassPositionInWorld(). #14538

Conversation

mitiguy commented Jan 15, 2021 • edited by sherm1 Loading

mitiguy left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

mitiguy 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

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

mitiguy 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

mitiguy 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

mitiguy left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

mitiguy left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

sammy-tri left a comment

Choose a reason for hiding this comment

sammy-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented May 16, 2021

EricCousineau-TRI commented May 17, 2021

mitiguy commented Jan 15, 2021 •

edited by sherm1

Loading