Replace NoiseModelFactor1 in with NoiseModelFactorN in pre-made factors

[gchenfc]

This is a follow-up to #947 which applies this "new style" NoiseModelFactorN to existing factors in GTSAM (e.g. PriorFactor, BetweenFactor, ...) which, after #947 is merged, they will be using deprecated versions of NoiseModelFactor123456.

I decided to separate this into a separate PR to keep file changes cleaner and because I think it might warrant more discussion (*sigh) and I don't want to drag out the other one if it doesn't have to.

Hiccup

In the process, I realized that this will introduce some backward compatibility issues. Specifically:

• For new usages / new factors, everything is fine.
• But for existing factors, once we change them from inheriting from NoiseModelFactor2 (deprecated) to NoiseModelFactorN, users will lose access to the key2() functions and the ::X2 type aliases (inherited from NoiseModelFactor2) since the new usage is now key<2>() and ::ValueType<2>.

So, for example (I will use this example for the rest of this description, but all pre-made factors in GTSAM have similar issues):

template <typename T>
class BetweenFactor : public NoiseModelFactor2<T, T> { ... }; // <-- change to NoiseModelFactorN<T, T>

BetweenFactor<Pose3> factor(X(1), X(2), Pose3(), noise);
EXPECT( X(1), factor.key1() ); // <-- key1() needs to change to key<1>()
EXPECT( X(2), factor.key2() ); // <-- key2() needs to change to key<2>()
using T1 = typename BetweenFactor<Pose3>::X1; // <-- `::X1` needs to change to `::ValueType<1>`
static_assert(std::is_same<T1, Pose3>::value, "X1 type incorrect");

Proposed Solutions

Allowing backward compatible access to key2() is not too ugly, so I just moved those into NoiseModelFactorN and deprecated them (fully backward compatible, but deprecated).

But allowing backward compatible access to ::X2 is quite ugly (see here and below), so I just left these out (implemented in NoiseModelFactor2, but not accessible from BetweenFactor anymore). I see 3 most reasonable options:

1. Break backward compatibility and, if anyone uses BetweenFactor::X2, they should change to BetweenFactor::ValueType<2>. Use of ::X2 seems pretty uncommon and didn't appear ever inside gtsam codebase including examples & tests (all unit tests pass).
2. Implement deprecated versions of X1 and X2 inside BetweenFactor (and for every pre-made factor in GTSAM). This is fine, but will just take a while and might add a lot of clutter.
3. Implement the ugly version in NoiseModelFactorN (see below).

gtsam/gtsam/nonlinear/NonlinearFactor.h

Lines 401 to 411 in 84e873e

	template <class... VALUES>
	class NoiseModelFactorN
	: public NoiseModelFactor,
	public detail::AliasX<VALUES...>, // using X = VALUE1
	public detail::AliasX1<VALUES...>, // using X1 = VALUE1
	public detail::AliasX2<VALUES...>, // using X2 = VALUE2
	public detail::AliasX3<VALUES...>, // using X3 = VALUE3
	public detail::AliasX4<VALUES...>, // using X4 = VALUE4
	public detail::AliasX5<VALUES...>, // using X5 = VALUE5
	public detail::AliasX6<VALUES...> // using X6 = VALUE6
	{

Changes / notes:

• NoiseModelFactor123456 -> NoiseModelFactorN
• BACKWARD COMPATIBILTY BREAKAGE: (see above) previously X2 is now ValueType<2>. e.g. any instances of BetweenFactor<Pose3>::X2 need to be changed to BetweenFactor::ValueType<2>. This applies to all pre-made factors in GTSAM.
• move the deprecated key1() functions from NoiseModelFactor1 to NoiseModelFactorN. Although we had previously discussed that it would be cleaner to be in NoiseModelFactor1, this will avoid breaking backward compatibility when changing pre-made factors to inherit from NoiseModelFactorN.
• Thing I recently noticed: when we have a templated factor class that inherits from NoiseModelFactorN, some calls (inside the class only) get ugly due to dependent types. e.g. template keyword is required here:

  gtsam/gtsam/slam/BetweenFactor.h

  Lines 91 to 92 in a3e314f

  	<< keyFormatter(this->template key<1>()) << ","
  	<< keyFormatter(this->template key<2>()) << ")\n";

  
  This only affects calls inside the class and is similar to why we need to sometimes use typename. More details here. I know this is standard template syntax, but I do remember a couple years ago the first time I saw this syntax reading code I was really confused because it looks so weird, so I would like to minimize its use if possible to avoid confusing people.

[dellaert]

@gchenfc , I like solution 2 the best: just add using X1 = in the factors within GTSAM. Also, for some reason, this PR did not change its base to develop when I merged #947 , so you’ll have to change it manually…

[varunagrawal]

I was wondering for a second why am I suddenly getting so many deprecation warnings. 😅

[varunagrawal]

Can we land this quickly please? Debugging is hard with the crazy amount of deprecation messages...

[dellaert]

Can we land this quickly please? Debugging is hard with the crazy amount of deprecation messages...

Yeah, I screwed up, did not see the other PR was targeting #947

[dellaert]

I will approve so we can land to suppress the warnings, but I did not see
“solution 2” implemented anywhere. Is it no longer needed?

[dellaert]

I will let @gchenfc merge - maybe solution 2 is still in the works, although I would also support doing this in a different PR.

[gchenfc]

Sounds good, yes solution 2 can be a separate PR. Might not be able to get to it until tomorrow night, but this PR can stand on its own before solution 2.

Merging now.