[pydrake/multibody] Add fix_inertia tool

[rpoyner-tri]

This change is 

[rpoyner-tri]

+@trowell-tri for eventual feature review -- not yet.
+(status: do not merge) +(status: do not review)

This is a reorg of code started as #19463, to accommodate testing, and to be compatible with distribution formats.

Motivating issue is #18773 .

Reviewable status: LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

[rpoyner-tri]

I believe r2 is substantially correct and has decent test coverage. Documentation is still sorely lacking and/or stale.

Reviewable status: LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

---

multibody/parsing/parser.h line 194 at r2 (raw file):

  bool GetAutoRenaming() const { return enable_auto_rename_; }

  /// Cause all subsequent Add*Model*() operations to replace physically

These entry points should be marked "advanced" or "internal use only" or some such.

[rpoyner-tri]

As always, writing bold assertions about correctness is the best way to find bugs. r3 fixes at least one bug, adds some but not all docs, and adds some previously missing tests.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

---

multibody/parsing/parser.h line 194 at r2 (raw file):

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

These entry points should be marked "advanced" or "internal use only" or some such.

I did some skimming of the PR, but it's not yet clear to me why this feature is related to the tool. I assume there is some reason why the rewriting tool wants to enable this, but I couldn't tease out what it is?

[trowell-tri]

Reviewed 2 of 5 files at r1, 12 of 14 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 19 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

---

multibody/parsing/detail_common.h line 288 at r3 (raw file):

// @param spoil_invalid_inertia  If true, return NaN-filled inertia rather than
//                                some valid approximation when the input values
//                                are physically invalid.

btw elsewhere you note that the returned SpatialInertia is default-constructed and I wonder if the most clear phrasing is something like "default-constructed (NaN-filled) SpatialInertia". Or are the differing levels of technical detail provided on purpose?

---

multibody/parsing/detail_common.cc line 270 at r3 (raw file):

    bool spoil_invalid_inertia) {
  SpatialInertia<double> invalid_inertia_replacement;
  const Vector3d& p_BoBcm_B = X_BBi.translation();

btw this got yanked way up here but no code other than the original stanza seems to use it? Detritus from a prior refactoring?

---

multibody/parsing/detail_common.cc line 277 at r3 (raw file):

      (std::isfinite(mass) && mass > 0.0) ? mass : 1.0;
  if (spoil_invalid_inertia) {
    // The goal here is to transmit mass, to the extent we have a valid one,

btw maybe "a valid value" instead of "a valid one" since it's not, like, an object.

---

multibody/parsing/detail_common.cc line 300 at r3 (raw file):

    // Bcm to Bo.  Note: It is unwise to directly create a solid sphere about
    // Bo as this does not guarantee that the spatial inertia about Bcm is
    // valid. Bcm (B's center of mass) is the ground-truth point for validity

nit you seem to care about two spaces after a period but missed this one. (I don't care, although I would try to be consistent, and other comments hereabouts are consistently using a single space.)

---

multibody/parsing/detail_sdf_parser.cc line 1374 at r3 (raw file):

  drake::log()->trace("sdf_parser: Add nested models");
  // auto resolver = workspace.collision_resolver;
  // auto package_map = workspace.package_map;

nit looks like refactoring debris?

---

multibody/parsing/parser.h line 184 at r3 (raw file):

  /// Cause all subsequent Add*Model*() operations to use strict parsing;
  /// warnings will be treated as errors.
  void SetStrictParsing() { is_strict_ = true; }

btw the strict parsing option is now the only one (of three) options with no corresponding getter; is that purposeful or should we add one?

---

multibody/parsing/parser.h line 196 at r3 (raw file):

  /// Cause all subsequent Add*Model*() operations to replace physically
  /// invalid inertia configurations with default-constructed SpatialInertia.
  void SpoilInvalidInertia() { spoil_invalid_inertia_ = true; }

btw I don't mind these new method names but I note that the other options are Set/Get pairs (well, at least "SetFoo" with one "GetFoo"); should we keep that convention? If that convention is more Drake-wide then we probably should…

---

multibody/parsing/parser.h line 199 at r3 (raw file):

  /// Cause all subsequent Add*Model*() operations to replace physically
  /// invalid inertia configurations with default-constructed SpatialInertia.

btw elsewhere you note that the returned SpatialInertia is NaN-filled and I wonder if the most clear phrasing is something like "default-constructed (NaN-filled) SpatialInertia". Or are the differing levels of technical detail provided on purpose?

---

bindings/pydrake/multibody/_inertia_fixer.py line 163 at r3 (raw file):

    Args:
        parent: the ElementFacts that will get new synthetic children
        names: a list of XML element names to maybe insert

btw maybe "to insert if missing"?

---

bindings/pydrake/multibody/_inertia_fixer.py line 167 at r3 (raw file):

    assert parent.children, parent
    kid_names = [x.name for x in parent.children]
    kid0 = parent.children[0]

nit refactoring debris?

---

bindings/pydrake/multibody/_inertia_fixer.py line 185 at r3 (raw file):

    if is_synth_element(facts):
        # Empty, synthetic, "pseudo" element.
        return facts.end.index

This doesn't seem like it can be right, both by itself and in comparison to the top-level is_synth_element (which makes sense) and it looks like it isn't tested, so … an error?

---

bindings/pydrake/multibody/_inertia_fixer.py line 187 at r3 (raw file):

        return facts.end.index

    if input_text[facts.end.index:].startswith(f"</{facts.name}"):

btw I assume that the expat parser leaves the index pointing at the start of the next tag when it consumes an element (because this works for files with inertia tags on separate lines) but could this fail for inertia blocks where there is an XML comment in the wrong place?

---

bindings/pydrake/multibody/_inertia_fixer.py line 482 at r3 (raw file):

    def associate_plant_models(self, plant: MultibodyPlant):
        """Match body indices to inertial elements in the input text.

nit should we echo the return information here as well?

---

bindings/pydrake/multibody/_inertia_fixer.py line 506 at r3 (raw file):

        input_text = self._input_text
        input_text_index = 0
        for body_index, new_inertia in sorted(new_inertias_mapping.items()):

btw how do body indices work for included files? It doesn't seem like we can be supporting them in this utility, but I wanted to confirm.

Originally I was trying to make sure that BodyIndex was monotonically increasing with file position (looks that way to me) and wondering why we didn't sort our mapping by element start, but from what I've read it seems like this should be fine.

---

bindings/pydrake/multibody/fix_inertia.py line 2 at r3 (raw file):

r"""
XXX docs!!

Flagging this so so no one forgets this later.

---

bindings/pydrake/multibody/fix_inertia.py line 34 at r3 (raw file):

    parser.add_argument(
        "--invalid_only", action="store_true",
        help="only fix physically invalid inertias.")

nit Capitalize first letter

---

bindings/pydrake/multibody/fix_inertia.py line 37 at r3 (raw file):

    parser.add_argument(
        "--in_place", action="store_true",
        help="modify the input file in-place. Any output_file argument"

nit Capitalize first letter

---

bindings/pydrake/multibody/test/fix_inertia_test.py line 321 at r3 (raw file):

    def test_massless_sphere_urdf(self):
        # See above comments about sdformat inertia processing; here we
        # get a magic mass of 1 from nowhere.

Copypasta, or does this really apply to URDF as well? (The zero mass below makes me think maybe not?)

[rpoyner-tri]

Reviewable status: 12 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

---

multibody/parsing/detail_common.cc line 270 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

btw this got yanked way up here but no code other than the original stanza seems to use it? Detritus from a prior refactoring?

It needs to have broader scope than just the else block now -- it gets used again at the very end.

---

multibody/parsing/detail_common.cc line 300 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

nit you seem to care about two spaces after a period but missed this one. (I don't care, although I would try to be consistent, and other comments hereabouts are consistently using a single space.)

I prefer the one-space -- the two-space text got pasted from another author. Will resolve to one-space.

---

multibody/parsing/parser.h line 194 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I did some skimming of the PR, but it's not yet clear to me why this feature is related to the tool. I assume there is some reason why the rewriting tool wants to enable this, but I couldn't tease out what it is?

The difficulty is in implementing --invalid_only -- the tool never attempts to hand-build inertias from XML , so forwarding the "invalid in source file" bit is necessary. There a other ways we could do this, but I was unpersuaded by the idea of writing yet-another xml-to-math stack in python.

---

multibody/parsing/parser.h line 184 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

btw the strict parsing option is now the only one (of three) options with no corresponding getter; is that purposeful or should we add one?

Meh? There are tests that detect the effect of strict parsing, so the getter didn't seem so important.

---

multibody/parsing/parser.h line 196 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

btw I don't mind these new method names but I note that the other options are Set/Get pairs (well, at least "SetFoo" with one "GetFoo"); should we keep that convention? If that convention is more Drake-wide then we probably should…

I don't really want this to be part of supported API, so meh?

---

multibody/parsing/parser.h line 199 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

btw elsewhere you note that the returned SpatialInertia is NaN-filled and I wonder if the most clear phrasing is something like "default-constructed (NaN-filled) SpatialInertia". Or are the differing levels of technical detail provided on purpose?

The nan proposal didn't work out. These should get resolved to document the later implementation.

---

bindings/pydrake/multibody/_inertia_fixer.py line 482 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

nit should we echo the return information here as well?

I think the annotations on the interface are wrong -- working.

---

bindings/pydrake/multibody/_inertia_fixer.py line 506 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

btw how do body indices work for included files? It doesn't seem like we can be supporting them in this utility, but I wanted to confirm.

Originally I was trying to make sure that BodyIndex was monotonically increasing with file position (looks that way to me) and wondering why we didn't sort our mapping by element start, but from what I've read it seems like this should be fine.

We don't support bodies from included files. It could be that there is a test missing.

---

bindings/pydrake/multibody/test/fix_inertia_test.py line 321 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

Copypasta, or does this really apply to URDF as well? (The zero mass below makes me think maybe not?)

Done.

[jwnimmer-tri]

Reviewable status: 12 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

---

multibody/parsing/parser.h line 194 at r2 (raw file):

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

The difficulty is in implementing --invalid_only -- the tool never attempts to hand-build inertias from XML , so forwarding the "invalid in source file" bit is necessary. There a other ways we could do this, but I was unpersuaded by the idea of writing yet-another xml-to-math stack in python.

Ah, okay.

I'm not convinced that --invalid_only is useful in the first place. Can you explain the use case? It seems like scope creep.

My nominal expectation is that:

(1) Users know how to use source control, and are capable of taking whatever parts of the diffs they prefer and leaving the rest behind.

(2) Files with an invalid inertia are typically all wrong, not just 10% wrong.

[rpoyner-tri]

Reviewable status: 12 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes

---

bindings/pydrake/multibody/_inertia_fixer.py line 187 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

btw I assume that the expat parser leaves the index pointing at the start of the next tag when it consumes an element (because this works for files with inertia tags on separate lines) but could this fail for inertia blocks where there is an XML comment in the wrong place?

Perhaps there is a test missing? I'm not sure I understand the misplaced comment scenario.

[rpoyner-tri]

Reviewable status: 12 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes

---

multibody/parsing/parser.h line 194 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ah, okay.

I'm not convinced that --invalid_only is useful in the first place. Can you explain the use case? It seems like scope creep.

My nominal expectation is that:

(1) Users know how to use source control, and are capable of taking whatever parts of the diffs they prefer and leaving the rest behind.

(2) Files with an invalid inertia are typically all wrong, not just 10% wrong.

If we drop invalid-only, then the whole Parser subdir set of mods can go. I like that.

[rpoyner-tri]

Reviewable status: 8 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes

---

bindings/pydrake/multibody/_inertia_fixer.py line 185 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

This doesn't seem like it can be right, both by itself and in comparison to the top-level is_synth_element (which makes sense) and it looks like it isn't tested, so … an error?

I'm not sure what about it looks wrong? It could be that I could delete this path and fall through to the (implementation-identical) final return statement. If I add assert False here, it definitely gets hit by the test suite.

FWIW, somewhere (perhaps the other PR) Nimmer said he would be fine with file/text level tests, perhaps at the expense of unit tests on every class and function.

---

bindings/pydrake/multibody/_inertia_fixer.py line 482 at r3 (raw file):

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

I think the annotations on the interface are wrong -- working.

This returns None. I've changed the method name to clarify that is not implementing the FormatDriver interface.

[trowell-tri]

Reviewed 1 of 8 files at r4, 14 of 14 files at r5, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

---

bindings/pydrake/multibody/_inertia_fixer.py line 185 at r3 (raw file):

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

I'm not sure what about it looks wrong? It could be that I could delete this path and fall through to the (implementation-identical) final return statement. If I add assert False here, it definitely gets hit by the test suite.

FWIW, somewhere (perhaps the other PR) Nimmer said he would be fine with file/text level tests, perhaps at the expense of unit tests on every class and function.

Whoops, never mind; I completely misread this stanza to mean something entirely different and I have no idea why. Retracted.

---

bindings/pydrake/multibody/_inertia_fixer.py line 187 at r3 (raw file):

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

Perhaps there is a test missing? I'm not sure I understand the misplaced comment scenario.

The (very contrived) error corner case would be:

<inertial>
   <some useful stuff>
   <!--A comment-->
</inertial>

Maybe expat just skips the comment in this case?

---

bindings/pydrake/multibody/_inertia_fixer.py line 506 at r3 (raw file):

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

We don't support bodies from included files. It could be that there is a test missing.

Ah, I see now. I had missed how the association is built just from links we've read from our immediate input file, not from the entire plant. Given that, I don't see how there's a useful test case here.

[rpoyner-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes

---

bindings/pydrake/multibody/_inertia_fixer.py line 187 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

The (very contrived) error corner case would be:

<inertial>
   <some useful stuff>
   <!--A comment-->
</inertial>

Maybe expat just skips the comment in this case?

In that case, the expat end index would still end up at the start of </inertial>. I think that this is in fact a very common case, given that nesting (of anything) is more-or-less the point of XML.

Perhaps the comment is misleading, because it looks like it only applies to one-line elements?

[rpoyner-tri]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes

a discussion (no related file):
In stress testing, I have found at least one file that still induces text-editing errors: examples/multibody/four_bar/four_bar.sdf. Investigating.

FWIW, stress testing is (so far) manual, and involves consuming all of the model files within Drake. Once that yields correct results (some are deliberately made to produce parser errors), I will expand stress testing to model files found in the wild.

---

[trowell-tri]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

---

bindings/pydrake/multibody/_inertia_fixer.py line 187 at r3 (raw file):

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

In that case, the expat end index would still end up at the start of </inertial>. I think that this is in fact a very common case, given that nesting (of anything) is more-or-less the point of XML.

Perhaps the comment is misleading, because it looks like it only applies to one-line elements?

I don't think the comment was misleading, I just haven't read the expat docs. If any comment is needed then maybe a quick precis of what the expat is behavior could be added somewhere higher up, especially if there are non-obvious things you rely on. But on the other hand, perhaps it's silly for a future maintainer to try to work on this without reading those docs. Although if there are non-obvious behaviors you've discovered in expat (I don't mean this unless it isn't documented) then I would note them down somewhere.

I'm still not sure how I feel about it skipping comments. It's handy, but what if you want to process them separately? Anyway that's super off-topic.

[rpoyner-tri]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes

---

bindings/pydrake/multibody/_inertia_fixer.py line 187 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

I don't think the comment was misleading, I just haven't read the expat docs. If any comment is needed then maybe a quick precis of what the expat is behavior could be added somewhere higher up, especially if there are non-obvious things you rely on. But on the other hand, perhaps it's silly for a future maintainer to try to work on this without reading those docs. Although if there are non-obvious behaviors you've discovered in expat (I don't mean this unless it isn't documented) then I would note them down somewhere.

I'm still not sure how I feel about it skipping comments. It's handy, but what if you want to process them separately? Anyway that's super off-topic.

The comments are handled by the underlying parser by triggering different comment-specific callbacks. I don't currently use those, but I suppose we could if there were sufficient reason. My overall goal is to just preserve what comments I can (with some caveats) -- that is done by limiting how much text is replaced, and just copying the rest through from the input text.

[trowell-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

---

bindings/pydrake/multibody/_inertia_fixer.py line 187 at r3 (raw file):

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

The comments are handled by the underlying parser by triggering different comment-specific callbacks. I don't currently use those, but I suppose we could if there were sufficient reason. My overall goal is to just preserve what comments I can (with some caveats) -- that is done by limiting how much text is replaced, and just copying the rest through from the input text.

Ah, that makes sense.

[rpoyner-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes

a discussion (no related file):

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

In stress testing, I have found at least one file that still induces text-editing errors: examples/multibody/four_bar/four_bar.sdf. Investigating.

FWIW, stress testing is (so far) manual, and involves consuming all of the model files within Drake. Once that yields correct results (some are deliberately made to produce parser errors), I will expand stress testing to model files found in the wild.

The offending file contains non-ascii UTF8 characters, which throw off the string-slicing math.

rico@torg:~/checkout/drake$ grep -P '[^\x00-\x7f]' examples/multibody/four_bar/four_bar.sdf 
   Wx  <- - - ·
        <!-- Rotation around Ax by 90° = π/2 radians ≈ 1.57079632679 -->
        <!-- Rotation around Bx by 90° = π/2 radians ≈ 1.57079632679 -->
        <!-- Rotation around Cx by 90° = π/2 radians ≈ 1.57079632679 -->
      <!-- This frame is rotated relative from link B, around Bx, by -90° = -π/2 radians ≈ -1.57079632679 -->
      <!-- This frame is rotated relative from link C, around Cx, by -90° = -π/2 radians ≈ -1.57079632679 -->

I'll have to study expat's behavior and see what I can do to fix things.

---

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

a discussion (no related file):

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

The offending file contains non-ascii UTF8 characters, which throw off the string-slicing math.

rico@torg:~/checkout/drake$ grep -P '[^\x00-\x7f]' examples/multibody/four_bar/four_bar.sdf 
   Wx  <- - - ·
        <!-- Rotation around Ax by 90° = π/2 radians ≈ 1.57079632679 -->
        <!-- Rotation around Bx by 90° = π/2 radians ≈ 1.57079632679 -->
        <!-- Rotation around Cx by 90° = π/2 radians ≈ 1.57079632679 -->
      <!-- This frame is rotated relative from link B, around Bx, by -90° = -π/2 radians ≈ -1.57079632679 -->
      <!-- This frame is rotated relative from link C, around Cx, by -90° = -π/2 radians ≈ -1.57079632679 -->

I'll have to study expat's behavior and see what I can do to fix things.

BTW Maybe expat is using bytes indexing instead of str indexing? Possibly doing the text-replacement in bytes land would resolve this.

---

[rpoyner-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Maybe expat is using bytes indexing instead of str indexing? Possibly doing the text-replacement in bytes land would resolve this.

Yes, expat math is in bytes. I'll change the input-text handling to use bytes.

---

[rpoyner-tri]

-(status: do not merge) -(status: do not review)

I've got more doc and hopefully one more look from Todd, and then I'll nominate a platform reviewer.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

a discussion (no related file):

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

Yes, expat math is in bytes. I'll change the input-text handling to use bytes.

r6 clears up unicode difficulties and adds tests.

---

---

bindings/pydrake/multibody/fix_inertia.py line 2 at r3 (raw file):

Previously, trowell-tri (Todd Rowell) wrote…

Flagging this so so no one forgets this later.

I need to write more here; keeping it Working for now.

[rpoyner-tri]

I worry the unit tests may be floating-point brittle; let's see:

@drake-jenkins-bot mac-arm-monterey-clang-bazel-experimental-release please.

[rpoyner-tri]

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

a discussion (no related file):

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

I worry the unit tests may be floating-point brittle; let's see:

@drake-jenkins-bot mac-arm-monterey-clang-bazel-experimental-release please.

Fears confirmed. I'll have to figure out a reasonable python float rounding strategy, or rework the tests to avoid text comparison of floats.

---

[rpoyner-tri]

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

Fears confirmed. I'll have to figure out a reasonable python float rounding strategy, or rework the tests to avoid text comparison of floats.

@drake-jenkins-bot mac-arm-monterey-clang-bazel-experimental-release please.

[rpoyner-tri]

r7 is looking pretty good. Mac CI issues addressed, docs largely done.

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

[trowell-tri]

Reviewed 5 of 7 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee trowell-tri, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @rpoyner-tri)

---

bindings/pydrake/multibody/fix_inertia.py line 2 at r7 (raw file):

r"""Utility for fixing invalid inertia data in URDF or SDFormat files, by
writing a new, complete, modified file.

BTW consider choosing a word other than "modified" because it has a commonly-used meaning wrt files that conflicts with "new".

[trowell-tri]

Reviewed 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @rpoyner-tri)

[rpoyner-tri]

+@sherm1 for Monday platform review

Reviewable status: LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @rpoyner-tri)

[rpoyner-tri]

+(release notes: feature)

Reviewable status: LGTM missing from assignee sherm1(platform)

[sherm1]

Woo hoo! That's quite an undertaking. Platform with a few minor comments

Reviewed 1 of 5 files at r1, 11 of 14 files at r5, 3 of 7 files at r6, 2 of 2 files at r7, 1 of 2 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: 8 unresolved discussions

---

bindings/pydrake/multibody/_inertia_fixer.py line 72 at r9 (raw file):

# This function has an aggressively short name for embedding into f-strings.
def gfmt(x: float) -> str:
    """Convert a floating point number to a string, using some rounding to

BTW in the C++ styleguide we would require descriptive "Converts" rather than imperative "Convert" in the function descriptions. Does Python have a different convention?

---

bindings/pydrake/multibody/_inertia_fixer.py line 112 at r9 (raw file):

            input_bytes: full contents of the input file
            inertial_facts: facts about the element to reformat
            pose: Pose of the center-of-mass frame relative to the body frame,

BTW consider calling that the "central inertia frame" rather than the "center of mass" frame since it has both the COM location (as the frame origin) and the basis vectors in which the inertia is expressed. Also if you give the frame a symbol here (say "C") then you can say that moments and products are "expressed in C".

---

bindings/pydrake/multibody/_inertia_fixer.py line 217 at r9 (raw file):

    d(int) -> str
        indents n levels in addition to the current depth of the element
         described by `facts`.

BTW did you want this 1-space indent?

---

bindings/pydrake/multibody/_inertia_fixer.py line 512 at r9 (raw file):

            new_inertias_mapping: a dictionary whose keys are BodyIndex values,
               and whose values are a tuple of the body's spatial inertia and
               6D inertial pose as [x, y, z, r, p, y].

BTW the meaning of "inertial pose" is unclear to me. Does it mean the pose relative to the link frame L of the central inertia frame C (i.e. X_LC?) in which the inertia was expressed? And then is the returned spatial inertia expressed in C? Can you make this more precise?

---

bindings/pydrake/multibody/_inertia_fixer.py line 584 at r9 (raw file):

                                     M_BBo_B_one.get_unit_inertia())

        # TODO: rotate to zero out products of inertia, and return both inertia

BTW consider attaching a name or issue number to this TODO

---

bindings/pydrake/multibody/fix_inertia.py line 44 at r9 (raw file):

    may not be exactly suited to the purpose at hand. Be sure to examine the
    model file differences, and use `model_visualizer` to examine the resulting
    inertial ellipses.

nit: ellipses -> ellipsoids

---

bindings/pydrake/multibody/test/fix_inertia_test.py line 245 at r9 (raw file):

      <inertia ixx="0" ixy="0" ixz="0" iyy="0" iyz="0" izz="0"/>
      <mass value="0"/>
      <origin rpy="0 0 0" xyz="0 0 0"/>

BTW why does a massless urdf sphere stay massless while the sdf one above gets inertias? Consider adding an explanation of this oddity for the naive reader.

---

bindings/pydrake/multibody/test/fix_inertia_test.py line 442 at r9 (raw file):

        # Repeated processing of the output gives the same result. This step
        # also ensures XML well-formed-ness and Drake parsing compatibility.
        self.assert_files_equal(direct_result, repeat_result)

BTW nice! I was going to ask about idempotence. Can you guarantee idempotence for any already-processed file? If so consider whether to document that.

[rpoyner-tri]

Reviewable status: 2 unresolved discussions

---

bindings/pydrake/multibody/_inertia_fixer.py line 512 at r9 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW the meaning of "inertial pose" is unclear to me. Does it mean the pose relative to the link frame L of the central inertia frame C (i.e. X_LC?) in which the inertia was expressed? And then is the returned spatial inertia expressed in C? Can you make this more precise?

I'm giving this a go, but we may need a few iterations to get things precise.

---

bindings/pydrake/multibody/test/fix_inertia_test.py line 245 at r9 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW why does a massless urdf sphere stay massless while the sdf one above gets inertias? Consider adding an explanation of this oddity for the naive reader.

sigh this is a way in which the format specs differ -- URDF explicitly says "all quantities not stated are 0" and SDFormat just makes something up in absence of data. I believe the SDFormat behavior is documented -- I'll try to find a reference.

---

bindings/pydrake/multibody/test/fix_inertia_test.py line 442 at r9 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW nice! I was going to ask about idempotence. Can you guarantee idempotence for any already-processed file? If so consider whether to document that.

I don't think I could prove idempotence, but I think this test already supplies some evidence. I'm not super-committed to promising it, given the nature of the tool ("always inspect your output!") and the likely effort required to deliver a guarantee.

[rpoyner-tri]

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

---

bindings/pydrake/multibody/_inertia_fixer.py line 512 at r9 (raw file):

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

I'm giving this a go, but we may need a few iterations to get things precise.

Did my best -- PTAL.

[sherm1]

Nice!

[RussTedrake]

FWIW -- I was just trying to point a student to this tool, but it took me a few minutes (finally looking here) to find it. I think we need to do something to improve it's discoverability? Probably mention it in the authoring_multibody tutorial?

BTW -- since it uses AddModels, can it load an OBJ directly, and spit out an SDF? That seems like potentially a very common/useful version of the workflow?

[jwnimmer-tri]

BTW -- since it uses AddModels, can it load an OBJ directly, and spit out an SDF? That seems like potentially a very common/useful version of the workflow?

For that use case, users should try mesh_to_model instead.