Add deprecated aliases for X1 = ValueType<1>, ...

[gchenfc]

Recap

Following up on / recapping #1344,

• upon changing all the pre-existing Factor classes to inherit from the new NoiseModelFactorN instead of NoiseModelFactor1-6, the aliases X1, X2, ... were no longer available in pre-existing factor classes. Therefore,
• Replace NoiseModelFactor1 in with NoiseModelFactorN in pre-made factors #1344 presented 3 options to maintain backward compatibility, of which @dellaert liked the 2nd option the best:
  • each class that was converted to the new NoiseModelFactorN now has the deprecated aliases defined in its class.

Summary

I created a macro ADD_NOISE_MODEL_FACTOR_N_DEPRECATED_TYPEDEFS(classname, number_of_types) in NonlinearFactor.h to add all the using X2 = ... aliases since I figured this might be cleaner, but I can change it to just manually defining them all (without the macro) if that's more readable.

Then in every class that I changed to inherit from NoiseModelFactorN, I added the ADD_NOISE_MODEL_FACTOR_N_DEPRECATED_TYPEDEFS macro at the beginning.

Sanity Check

As a quick sanity check that all the files changed in #1344 have been addressed, #1344 has 64 .h files while this only has 54. The missing ones are:

• Testable.h, types.h, graph-inl.h - none of these define factors and the changes were things like updating comments
• TransformBtwRobotsUnaryFactor.h - just a comment update
• tests/ simulated2D.h, simulated2Doriented.h, simulated3D.h, smallExample.h - since these are unit tests, they don't need these backward compatible alias definitions.
• BoundingConstraint.h - this file already typedef'd X and X1 and X2 so I didn't need to typedef them again.
• MultiProjectionFactor.h - I don't even know how this factor compiled before I changed key1() and key2() since it inherits from NoiseModelFactor which doesn't have a key1() or key2() function (didn't raise compile error until you called evaluateError in cpp file?!), but in any case, this file doesn't need the aliased definitions.

And that makes all 10!

[dellaert]

LGTM, but Windows CI fails.

[dellaert]

@gchenfc lets fix windows CI and merge?

[dellaert]

So, BTW, I’m re-evaluating whether we should deprecate the old classes. Thoughts? Pros/cons?

[gchenfc]

So, BTW, I’m re-evaluating whether we should deprecate the old classes. Thoughts? Pros/cons?

My short answer is that, yes, I think this makes sense. And then I think we might revert #1344. New factors can use the new NoiseModelFactorN, but there's not much downside to keeping old factors they way they are (see pros and cons).

However, I'm actually going to propose something slightly different:

1. move (and un-deprecated) the shorthands X1-X6 and key1()-key6() into NoiseModelFactorN.
2. Then, we can do #define NoiseModelFactor1 NoiseModelFactorN, since NoiseModelFactorN's API will be a true "superset" of NoiseModelFactor1-6's
3. Then, I see no real cons of using NoiseModelFactor1-6 so no reason to deprecate.

Some background

In a previous discussion, we presented 2 options:

1. Implement X1-6 and key1-6 in NoiseModelFactorN and then do #define NoiseModelFactor1 NoiseModelFactorN
2. Don't implement the shorthands, and then do

   struct NoiseModelFactor1 : public NoiseModelFactorN {
     // define shorthands
   };

We opted for the latter, since it's easier to read, but I'm now thinking maybe we should switch back to the former, for reasons outlined in the next sections. See also these threads:
#947 (comment)
#947 (comment)

Pros/Cons of moving aliases X1-6, key1-6() to NoiseModelFactorN

I think X1 and key1() are indeed convenient shorthands/aliases, which I think could be helpful to keep/add (un-deprecated) in NoiseModelFactorN.

(Currently, key1-6() are available (deprecated) in NoiseModelFactorN but X1-6 are not - this PR is implementing them manually in each individual factor.)

Pros:

• Makes migrating from NoiseModelFactor1-6 to NoiseModelFactorN way easier, since no calling-side code has to change at all. Just find-all-replace : public NoiseModelFactor[1-6] -> : public NoiseModelFactorN
• keep things shorter/more readable. MyFactor::X3 is (subjectively) cleaner than MyFactor::ValueType<3>.
• Still extensible: in the event someone needs e.g. X7, then they can just use ValueType<7>.
• There's one particular case that makes ValueType<1> annoying to use: when you are defining a templated factor and you want to use ValueType<1> inside the definition of the templated factor, you need to do typename MyFactor::template ValueType<1> instead of just ValueType<1>. So in these cases, it's much nicer to just use X1.

Cons:

• clutters up implementation of NoiseModelFactorN making it more difficult to read, especially X1-6.
• since we would do #define NoiseModelFactor1 NoiseModelFactorN, some compilers may not support deprecating the old classes.

Not deprecating NoiseModelFactor1-6

If we do the above proposed change, then there's really no downside to keeping NoiseModelFactor1-6 un-deprecated, since it will literally be:

#define NoiseModelFactor1 NoiseModelFactorN

because NoiseModelFactorN's API will be a superset of NoiseModelFactor1's.

Otherwise (if we do not do the above proposed change), then:

Pros:

• existing codebases won't get tons of deprecation warnings
• less confusion

Cons:

• additional maintainability requirements (but shouldn't be difficult, and NonlinearFactor.h rarely changes anyway)
• serialization is hacky due to backward compatibility

[dellaert]

I do like that solution. But would using rather than #define` not work better?

[gchenfc]

I do like that solution. But would using rather than #define not work better?

There's a weird edge-case backward compatibility issue where using doesn't work right with templates and class name injection. See #947 (comment)

TL;DR - normally you can do this:

class MyFactor : public NoiseModelFactor2<Pose3, Point3> {
  using This = NoiseModelFactor2;
};

and it will deduce the template arguments. But if you do something like

template <typename T1, typename T2>
using NoiseModelFactor2 = NoiseModelFactorN<T1, T2>;

then the previous code block will no longer be able to deduce the template arguments and will throw a compile-time error.

I have a prototype almost ready - I'll create a separate PR since it's getting to be quite different than this PR, and if we like that we can close this PR.