Codecov Report

Merging #422 (0340abf) into ign-math6 (c52a3fa) will not change coverage.
The diff coverage is n/a.

❗ Current head 0340abf differs from pull request most recent head d26ed19. Consider uploading reports for the commit d26ed19 to get more accurate results

@@            Coverage Diff             @@
##           ign-math6     #422   +/-   ##
==========================================
  Coverage      99.83%   99.83%           
==========================================
  Files             41       41           
  Lines           4138     4138           
==========================================
  Hits            4131     4131           
  Misses             7        7           

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 c52a3fa...d26ed19. Read the comment docs.

are there tests we are missing that would confirm the intended behavior? we don't need to add it now, but I'm wondering what those tests would look like or if some of the classes already have them

are there tests we are missing that would confirm the intended behavior? we don't need to add it now, but I'm wondering what those tests would look like or if some of the classes already have them

I don't know how to test this behavior. It has to do with object lifetime and the python garbage collector, so I'm not sure if there's an easy way to test it.

are there tests we are missing that would confirm the intended behavior? we don't need to add it now, but I'm wondering what those tests would look like or if some of the classes already have them

I don't know how to test this behavior. It has to do with object lifetime and the python garbage collector, so I'm not sure if there's an easy way to test it.

ok, I don't fully understand the issue so I won't approve this, but I think it's fine to merge since it has one approval

are there tests we are missing that would confirm the intended behavior? we don't need to add it now, but I'm wondering what those tests would look like or if some of the classes already have them

I don't know how to test this behavior. It has to do with object lifetime and the python garbage collector, so I'm not sure if there's an easy way to test it.

ok, I don't fully understand the issue so I won't approve this, but I think it's fine to merge since it has one approval

I can demonstrate the issue from sdformat

import sdformat as sdf
from ignition.math import Inertiald, MassMatrix3d, Vector3d, Pose3d

link = sdf.Link()
mass = 10.0
inertial = Inertiald(MassMatrix3d(mass, Vector3d(2, 3, 4), Vector3d.ZERO),
                     Pose3d())
link.set_inertial(inertial)
print("mass from inertial:", inertial.mass_matrix().mass())
print("mass from link:", link.inertial().mass_matrix().mass())

will print

mass from inertial: 10.0
mass from link: 0.0

but we expect both to be 10. This PR fixes that.

Edit: You have forward port this to main to run that script since the SDFormat python bindings are in main.

ok, I think I see what's happening. I still think it would be best to have tests that confirm what we think the API should be doing. Here is a branch where I've added some test expectations to Inertial_TEST.py to study the behavior change introduced by this PR:

https://github.com/gazebosim/gz-math/compare/scpeters/python_inertial_tests?expand=1

• 29cd3fe: behavior before this PR, calling inertial.mass_matrix().set_mass() would change the mass in the object stored by the inertial since mass_matrix() was returning a reference (tests pass)
• 997b2c3: merge with ign-math6 including the changes from this PR (test fails since mass_matrix() no longer returns a reference
• d793089: change test expectation so it doesn't expect parent object to be changed (tests pass)

this more closely matches the c++ API; hopefully it is pythonic enough

The C++ member function Inertial::MassMatrix returns a const ref so mutation through the ref is not the intended API. As stated in the pybind11 documentation, pybind11 casts away constness in return values, so you don't get an error when doing inertial.mass_matrix().set_mass(), but I'm not sure if that's a well defined behavior. And since the intended API in C++ is to use Inerital::SetMassMatrix for mutating the mass matrix, I would propose we match that API in python.