Add MatrixX class

[chapulina]

🎉 New feature

• Addresses Add variable-sized Matrix #144
• Part of Hydrodynamic added mass gz-sim#1462

Summary

We're coming for you, Eigen! (Well, that's what a lot of people think about gz-math... 😆 )

We need a Matrix6d to implement

• Hydrodynamic added mass gz-sim#1462

Instead of copying the existing Matrix3 and Matrix4 classes, I thought it would be useful to implement a matrix class that can have any size. The size is defined at compile time and can't be changed for a given object, so the matrix can't be dynamically resized.

I only implemented very basic functionality, more functions can be added to this class as needed. I think we probably have everything that's needed to implement added mass, but if we don't I can add new functions later.

I experimented with different std algorithms to substitute the nested for loops, but they were all too confusing to use with nested std::array. I'm open to suggestions though.

TODO

• I'd like to get some high-level feedback on this PR and the added APIs before adding Python bindings for this class.

Test it

Take a look at the added tests and run the new example.

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

[scpeters]

I'm leaning against MatrixXd to be honest. I think it's not strictly necessary for hydrodynamics, and I'd rather not add to the maintenance burden of gz-math by trying to tackle Eigen's functionality.

I think we could compose a gz::math::HydrodynamicAddedMass class using a pair of MassMatrix3s for the symmetric, block-diagonal 3x3 matrices and a Matrix3 for the upper / lower block diagonal component. Defining the class using sub-matrices could also help avoid confusion about the ordering of indices (rotation vs position) by ensuring that proper names are used.

[mjcarroll]

I'm inclined to agree with @scpeters on reproducing Eigen functionality. In this case, gz-sim already has a dependency on libgz-math-eigen3, so Eigen can be used there without introducing a new dependency.

My main concern would be that we are missing out on a lot of testing/optimizations that Eigen has already gone through for multiple platforms (hardware and OS).

[chapulina]

I think we could compose a gz::math::HydrodynamicAddedMass class

That feels a bit too specific to me. Hydrodynamic added mass is one use case for exposing all the fields of a spatial mass matrix, but I can imagine other advanced users may want to directly tweak those fields as well. Drake and DART are just two examples of engines that expose the full spatial tensor without making it specific to hydrodynamics.

If a Matrix6 class is not desired, I'd suggest we just add more Matrix3 fields directly to Inertial to hold various parts of the matrix - but that feels to me more like a workaround our limited API than a careful design choice.

gz-sim already has a dependency on libgz-math-eigen3

Yeah but SDFormat doesn't, and neither does the math::Inertial class. I can hear an argument in favor of deprecating our existing vector and matrix classes completely in favor of Eigen, but that would be a longer-term task.

we are missing out on a lot of testing/optimizations that Eigen has already gone through for multiple platforms (hardware and OS).

Yeah definitely, and that's also true for all our existing vector and matrix classes.

against MatrixXd to be honest. I think it's not strictly necessary for hydrodynamics, and I'd rather not add to the maintenance burden of gz-math by trying to tackle Eigen's functionality.

I hear you. But to summarize the comments above, the choices I see here are:

1. Composing a 6x6 matrix out of 3x3 sub-matrices - which feels like a workaround for a limited API
2. Doing things the "Gazebo Math way" and adding a Matrix6 class similar to Matrix3 and Matrix4 - I'm not opposed to that, I just thought we could try to be more flexible and reduce duplication - which in itself should be better for maintenance (see i.e. Add functions from Vector3 to Vector2 and Vector4 #71).
3. Going all in on Eigen - which is not doable on Fortress, and would require more effort to make our API consistent - but I'm not opposed to if we get the funds to do it right
4. Going with MatrixX - I saw issue Add variable-sized Matrix #144 and thought this would be an opportunity to tackle it. I don't think we should try to implement various algorithms for this class, I think it could just be handy to hold data, which is what we need for added mass.

[codecov]

Codecov Report

Merging #442 (b9c4d2b) into ign-math6 (73b23d1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           ign-math6     #442   +/-   ##
==========================================
  Coverage      99.65%   99.66%           
==========================================
  Files             67       68    +1     
  Lines           6392     6477   +85     
==========================================
+ Hits            6370     6455   +85     
  Misses            22       22           

Impacted Files	Coverage Δ
include/ignition/math/MatrixX.hh	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73b23d1...b9c4d2b. Read the comment docs.

[mjcarroll]

I think it could just be handy to hold data, which is what we need for added mass.

I think in this case, having the Matrix is useful. I think, in general, it would be nice if people were using Eigen when possible to actually do computations/manipulations. I understand that we may still need some of our own datatypes for convenience in terms of serialization and interactions with other locations in our APIs, but if we can encourage "jumping out" to Eigen for math heavy operations, I think that would be the ideal mix.

[chapulina]

if we can encourage "jumping out" to Eigen for math heavy operations, I think that would be the ideal mix.

I believe we've been doing this higher up the stack. gz-physics makes heavy use of Eigen, as well as various gz-sim plugins, some gz-rendering functionality... Maybe I shouldn't have joked about Eigen on the PR description, it definitely has a place in our development, but I don't think it's a good match in this specific case.

the choices I see here are

So for completeness, I'll mention my preference would be to go with option 4, but I could compromise for 2. I don't think 1 is a good option and 3 isn't feasible at this moment. @scpeters , please let me know what you think.

[azeey]

I'm not sure I'd call (1) a workaround. If you look at the implementation of MassMatrix3, it doesn't actually store a 3x3 matrix. Instead, it stores the mass, moments of inertia, and products of inertia separately. You can get a 3x3 matrix out of it, but that's not how it's stored. In general, since the mass matrix has structure, it makes sense to decompose it into sub parts. This has advantages in memory as well as avoiding the indexing confusion that @scpeters mentioned.

I'm also in favor of encouraging the use of Eigen and using our effort to make that easier to use than to expand our set of classes with a MatrixX.

[scpeters]

1. Composing a 6x6 matrix out of 3x3 sub-matrices - which feels like a workaround for a limited API

I'm not sure I'd call (1) a workaround. If you look at the implementation of MassMatrix3, it doesn't actually store a 3x3 matrix. Instead, it stores the mass, moments of inertia, and products of inertia separately. You can get a 3x3 matrix out of it, but that's not how it's stored. In general, since the mass matrix has structure, it makes sense to decompose it into sub parts. This has advantages in memory as well as avoiding the indexing confusion that @scpeters mentioned.

I don't consider it a workaround. I'm imagining it more like an extension of the MassMatrix3 (we could call it MassMatrix6 instead of HydroDynamicAddedMass). I would propose adding this class to the eigen3 component. I've just now proposed requiring the eigen3 component in libsdformat13 for garden (gazebosim/sdformat#1054).

You've indicated that you need something that works for Fortress

3. Going all in on Eigen - which is not doable on Fortress, and would require more effort to make our API consistent - but I'm not opposed to if we get the funds to do it right

I still think there is a place for gz::math::Vector[234] and gz::math::Matrix[34]. I don't have any immediate plans to deprecate them and switch fully to Eigen.

Is Fortress a hard requirement? It will be problematic to add the hydrodynamic added mass inertial tags to SDFormat 1.9 now that an older version of libsdformat12 is in Ubuntu 22.04 without those tags. We could consider adding them as custom elements in an xmlns for use in 1.9 and as formal elements in 1.10

[chapulina]

it stores the mass, moments of inertia, and products of inertia separately. You can get a 3x3 matrix out of it, but that's not how it's stored. In general, since the mass matrix has structure, it makes sense to decompose it into sub parts.

The 6x6 added mass matrix can't be decomposed into semantically meaningful sub parts, see the Hydrodynamic Added Mass Proposal.

I would propose adding this class to the eigen3 component.

Are you proposing also moving the existing Inertial and MassMatrix3 classes to the eigen component? The idea of the Hydrodynamic Added Mass Proposal is to augment the existing class, not to create a separate implementation.

Is Fortress a hard requirement?

That can be discussed. The libsdformat12 upstream release into Ubuntu 22.04 isn't a blocker to adding new features to any part of the stack though.

[scpeters]

I would propose adding this class to the eigen3 component.

Are you proposing also moving the existing Inertial and MassMatrix3 classes to the eigen component? The idea of the Hydrodynamic Added Mass Proposal is to augment the existing class, not to create a separate implementation.

Sorry I didn't realize you were proposing to modify the existing math::Inertial class to included added mass. Won't that break ABI on ign-math6?

[chapulina]

Won't that break ABI on ign-math6?

Ouch yeah I forgot that these classes aren't PIMPL'ed. I guess I'll have to target this at Garden then.

[chapulina]

I still think there is a place for gz::math::Vector[234] and gz::math::Matrix[34]. I don't have any immediate plans to deprecate them and switch fully to Eigen.

Could you elaborate on that? What's the place for these classes when we're using Eigen throughout our stack? And why should we maintain matrix 3 and 4, but not 6 or X?

[chapulina]

I think we could compose a gz::math::HydrodynamicAddedMass class using a pair of MassMatrix3s for the symmetric, block-diagonal 3x3 matrices and a Matrix3 for the upper / lower block diagonal component. Defining the class using sub-matrices could also help avoid confusion about the ordering of indices (rotation vs position) by ensuring that proper names are used.

avoiding the indexing confusion

After giving this point a bit more consideration, I still think that this would be clunky. The MassMatrix3 class doesn't seem fit to be used this way - do we just leave the mass invalid? What do DiagonalMoments mean in this case? Instead of using proper names, we'd be giving a new meaning to different concepts. That's more confusing.

What I think you really want is a more generic SymmetricMatrix3 class. And at this point, we could go directly for a SymmetricMatrix6 or SymmetricMatrixX class that would address our use case directly. I started a draft in that direction in #447.

it doesn't actually store a 3x3 matrix. Instead, it stores the mass, moments of inertia, and products of inertia separately. You can get a 3x3 matrix out of it, but that's not how it's stored.

I get the point that a generic matrix is not the optimal storage for a symmetric matrix. I started an implementation on #447 which is called FluidAddedMass, but it's really a generic SymmetricMatrix6.

Even with that, I still feel that a Matrix6 is missing. At some point we'll need to construct a 6x6 spatial matrix from the current terms in math::Inertial, and add that to the added mass matrix. Without a generic Matrix6 representation the implementation will probably just add term by term and assign them to the physics engine, which feels a bit messy, but I can live with.

[scpeters]

since we won't be using Eigen for this; I think I would favor an explicit Matrix6 class over MatrixX. I would also suggest helper functions for getting and setting 3x3 subsets of the Matrix6 from a Matrix3 (block diagonal upper left and lower right, and block off-diagonal upper right and lower left). This would make it easy enough to map Inertial values to a spatial inertia matrix. I don't think we need to support symmetric matrices at the class level

[chapulina]

Thanks, @scpeters , I'll open a separate PR with a Matrix6 implementation.