Resolve "Factors renaming"

Will keep #302 (closed) open for future improvements

added To Do (release) label

added 7 commits

• ccc7e345 - Rename FactorAutodiffDistance3d -> FactorDistance3d
• b8bcbb9c - Rename FactorOdom2d -> FactorOdom2dAutodiff
• 61b77da8 - Rename FactorOdom2dAnalytic -> FactorOdom2d
• f60905d4 - Use FactorOdom2d instead of FacytorOdom2dAutodiff
• 781002c6 - Rename FactorRelative2dAnalytic -> FactorRelativePose2d
• 9b59f5d5 - Make FactorOdom2dCloseloop inherit from FactorRelativePose2d
• 20a88e02 - Rename FactorOdom2dCloseloop -> FactorLoopclosure2d

Compare with previous version

@joanvallve could you approve/comment these changes for factor renaming?

THE MR is ready to merge if approved. Some plugins need minor adaptations, really small.

These are the basic changes

mentioned in merge request !359 (closed)

marked this merge request as ready

I revisited the issue because I didn't remember anything.

I found an agreement of removing Autodiff or Analytic from the name. If both types of factors are implemented, the autodiff one should be moved to the test/dummy folder (to be used in the test of the analytic factor).

Also, I found that @jsola proposed (don't know if was agreed) to avoid classes with sensor-based name and use geometry-based names (FactorRelativePose2d instead of FactorOdom2d for example). I think its a good idea.

I think that some day we should have the most common geometrical factors (relative, absolute, distance..?) implemented as analytic factors in core for 2D and 3D and with and without extrinsics.

Maybe we should keep the issue as open after applying these renamings. These agreements are not urgent nor critical but I think we should either reconsider them or keep them alive.

added 1 commit

• 8b3074bb - Move factor_odom_2d_autodiff.h to test/dummy/

Compare with previous version

Moved FacOdom2dAutodiff to test/dummy/

At this moment, FactorRelativePose2d is derived twice with only the topology field changed:

• FactorOdom2d : "MOTION"`
• FactorLoopclosure2d : "LOOP"

I will push and merge the MR as it is now, and update all plugins accordingly, today.

I think it's not too bad to have these two specialized factors above, but otherwise we could just make FactorRelativePose2d template so that:

FactorRelativePose2d<"MOTION">.getTopology() --> "MOTION"
FactorRelativePose2d<"LOOP">.getTopology() --> "LOOP"

We can keep the issue open, but move it to TODO (future). However, maybe it's better to discuss soon whether we want to keep it like this or not and then proceed with the changes (it's easy to do).

changed the description

assigned to @jsola

requested review from @joanvallve

merged

mentioned in commit d6c1888e

added Agreement required label