Pose Factor Test

[varunagrawal]

This PR adds a test demonstrating that the wrapping for the PoseFactor is broken.

I will also revert the PoseFactor with the deprecation notice while keeping the current PoseFactor function.

[varunagrawal]

From the CI for my first commit:

======================================================================
ERROR: test_constructor (test_factors.TestPoseFactor)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/GTDynamics/GTDynamics/python/tests/test_factors.py", line 49, in test_constructor
    pose_model, joint)
TypeError: Unable to convert function return value to a Python type! The signature was
    (wTp_key: int, wTc_key: int, q_key: int, cost_model: gtsam.gtsam.noiseModel.Base, joint: gtdynamics.gtdynamics.Joint) -> gtsam::ExpressionFactor<Eigen::Matrix<double, 6, 1, 0, 6, 1> >

----------------------------------------------------------------------
Ran 25 tests in 0.535s

[dellaert]

Removed @yetongumich as reviewer. This is not his doing, and it should not be implied it is.

[dellaert]

From the CI for my first commit:

======================================================================
ERROR: test_constructor (test_factors.TestPoseFactor)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/GTDynamics/GTDynamics/python/tests/test_factors.py", line 49, in test_constructor
    pose_model, joint)
TypeError: Unable to convert function return value to a Python type! The signature was
  (wTp_key: int, wTc_key: int, q_key: int, cost_model: gtsam.gtsam.noiseModel.Base, joint: gtdynamics.gtdynamics.Joint) -> gtsam::ExpressionFactor<Eigen::Matrix<double, 6, 1, 0, 6, 1> >

----------------------------------------------------------------------
Ran 25 tests in 0.535s

Irrelevant. We will kill the PoseFactor. No need to add unit tests to make a point.

[varunagrawal]

My code has always been unit tested. :slightly_smiling_face: The breakage is because there is a non-backwards-compatible API change and the change was not verified to work with the wrapper, so my downstream code is unrelated to this.

What I don't understand is why the insistence to remove the PoseFactor class, since the new PoseFactor function will be removed later anyway. It makes more sense to just call the function as PoseExpressionFactor and then get rid of it later as needed without the need to break any API.

[varunagrawal]

And speaking of code breaking, this situation is akin to us changing something in GTSAM such that it is no longer backwards compatible and then Zoox/Skydio/Nuro then having their code break. You can't go and blame them for not having unit tested their code because we don't have visibility to it. :)

[dellaert]

Repeating: "This was not the plan we agreed to. your code was broken because it was not unit tested, which allowed us to remove the PoseFactor without consequence. Please do not use the name “PoseFactor”: that is now reserved for the new function. Rather, for the “restored” class, use “TempPoseFactor” to make your code work as a temporary solution. We can discuss on Monday what a better solution is in keeping with constrained optimization."

This is a fire and forget comment. Unlikely to revisit/approve until this is executed upon. Too little time.

[varunagrawal]

@dellaert even if we ignore the changes in this PR, the wrapper doesn't work with the code the way it is currently, because wrapping of ExpressionFactor is not available. I recommend trying to call PoseFactor from Python so you can see what the issue is. By rushing through this, I am afraid you are missing the overall perspective. Your comment about unit testing is simply not true, because the state estimator uses GTDynamics as a depedent package, and is not part of this repo . This is why you didn't see any consequences.

[varunagrawal]

I need to reiterate this comment. Please take a minute to really understand the issue here. These changes will make GTDynamics essentially useless via Python.

And speaking of code breaking, this situation is akin to us changing something in GTSAM such that it is no longer backwards compatible and then Zoox/Skydio/Nuro then having their code break. You can't go and blame them for not having unit tested their code because we don't have visibility to it. :)

[varunagrawal]

What would be the long term implications? Would we still have the ability to create individual pose factors based on the URDF? How would that work?

[varunagrawal]

@dellaert the CI will break because of the ExpressionFactor version of PoseFactor. Using a NoiseModelFactor shared pointer has some weird behavior with type info, so a simple assertIsInstance(factor, gtd.PoseFactor) fails.