SpatialInertia constructors check invariants in release builds

[amcastro-tri]

SpatialInertia::CheckInvariants() is called from within constructors even during release builds. This is an expensive operation we cannot afford doing within performance critical multibody computations. However, there is value on checking those invariants considering that within Drake this is probably the only place where anyone checks if inertias coming from say, an SDF model, do actually make sense. That is, if we remove the check during release builds, we loose that nice check in our parsers.

A possible solution would by to add an additional bool unsafe = false flat at the end of each constructor. Advanced users not wanting the expensive check can pass unsafe = true.

Any other ideas recommendations?

[jwnimmer-tri]

Maybe you can point to some uses where it's slow in practice? Be more specific?

If the problem is, say, converting one exiting Drake type to a SpatialInertia, where we know the invariants will hold across the conversion because they were already true in the originating type, then we can make release builds always skip the check during that conversion, without us having to annotate unsafe=true in our code.

There might also be nothing wrong with adding specific validity checks in, say, the parsing code, or the AddBody method or whatever, and then turning off the invariant check in release builds.

[amcastro-tri]

Maybe you can point to some uses where it's slow in practice? Be more specific?

Yes, MBP calls SpatialInertia::cast() to retrieve the default value of an inertia matrix. cast() is calling one of the ctors which do the invariant check. That's why I proposed the extra boolean. It'd allow us to call it from within cast() (and maybe others) without changing the contract we have right now.

There might also be nothing wrong with adding specific validity checks in, say, the parsing code

true, but someone has to do it, while right now those already happen.

[jwnimmer-tri]

Oh. Yes, we already have several examples of private constructors that skip the invariants when it's a self-call. See, for example, private: RotationMatrix(const Matrix3<T>& R, bool). It's an absolutely good and uncontroversial to change intra-class calls to skip redundant checks. I thought you meant that, say, the MBT or MBP code would add bools to their call sites, to opt-out of checking.

[sherm1]

Per Slack discussion, my preference would just be to make this DRAKE_ASSERT_VOID(CheckInvariants()); like many other checks in that class. Can you explain why you don't like that, Alejandro?

[sherm1]

OTOH if you are proposing just to add a private constructor with an extra bool, I'm totally happy with that. Since you mentioned (on Slack) the "contract" I assumed you were talking about a user-visible API.

[sherm1]

BTW we could make the sdf parser explicitly check for bad inertias using the APIs that just do that. We could likely produce a better error message that way than just having it blow up in SpatialInertia's constructor. For example, we could report the body whose inertia was bad.

[amcastro-tri]

Can you explain why you don't like that, Alejandro?

Because IMO is changing a contract that apparently we've been giving to users for a while. IMO that would require some deprecation process, no?

[sherm1]

IMO that would require some deprecation process, no?

I don't think so. All that would happen is some things that used to throw in Release would only throw in Debug. No one would have to change their program unless they were putting a try/catch around that and doing something with it. Seems very unlikely!

[jwnimmer-tri]

As of #13682, we have a skip_validity_check option in on the SpatialInertia constructor. Is that sufficient to close this issue?

[amcastro-tri]

It looks resolved.