Testing performance of SLOW_BUT_CORRECT_BETWEENFACTOR

[yunzc]

Hi,
Looking at the performance of BetweenFactors and wrote this extra test. @dellaert @lucacarlone told me to ping you about this. The current default does not define the SLOW_BUT_CORRECT_BETWEENFACTOR and for the test example gave an error (compared with the numerical result) with magnitude of around 0.01 with an average runtime on my laptop of around 2e-7 seconds. If we do define SLOW_BUT_CORRECT_BETWEENFACTOR and enable the exponential maps, the error magnitude goes down to around 1e-12 and runtime increased to around 3e-7.
Perhaps, since the runtime does not increase by much, SLOW_BUT_CORRECT_BETWEENFACTOR and GTSAM_ROT3_EXP should be made default? Or at least documented in readme and allow the user to set it as a cmake option? (Note that if we set SLOW_BUT_CORRECT_BETWEENFACTOR but not GTSAM_ROT3_EXP gtsam would crash due to the cayley derivatives being unimplemented currently.

---

This change is 

[dellaert]

Thanks !!! I'm definitely interested in abolishing the flag based on the discussion with Luca and your results, although it would impose derivatives on all retracts :-(, not just Pose2/Pose3. How about adding some comments in the code below? I'm not sure about what you are trying to do in lines 75-83. Are the results not always the same?

[yunzc]

@dellaert I'm super sorry for the late reply!
The code is mostly the same as the existing test in testBetweenFactor, and this is less of a test and more of a short demo to show the timing and error of evaluateError(). I just tried this with the flags on and also with the flags off to compare the error and the time of calculation. You are right in that evaluateError should give them same result in the for loop between 75-83, but what I was trying to do there was to get the timing (which could vary for each iteration).
Added some comments, let me know if you would like me to test something else.
Thanks! Sorry again for taking so long to reply

[dellaert]

I think the timing and correctness are two different things. It does not make sense to average the results if they are the same :-) I propose to move timing to a different script in timing folder? See e.g. timeRot3.cpp

[dellaert]

So, this issue/PR has been lingering for a long time, and one big reason is that SLOW_BUT_CORRECT_BETWEENFACTOR needs a derivative for the Local function of a particular type. But that does not exist for all types. Maybe this can be done for types that have this with partial specialization. However, Cayley, which we use all the time, does not have this either. Would love to get @lucacarlone 's opinion on how to resolve this, which has been nagging me for a while now.

[lucacarlone]

@dellaert: I'm thinking about this issue and 3 solutions come to mind:

1. can we add a check that uses the derivative of local whenever is available? (without a flag - I think this is the same you suggested above)
2. can we just enforce every relevant type to provide that? (in other words, how many of these derivatives are missing? can we just implement them?)
3. can we leverage autodiff when the analytic derivative is unavailable?

[varunagrawal]

I have a question about this (since I recently fixed some issues in the Cayley map implementation), no other factor seems to care about the derivatives for the traits<T>::Local call, except for the BetweenFactor.

What is the motivation for computing the Hlocal jacobian in this case? Perhaps that can help motivate the design choices we make based on @lucacarlone's suggestions.

[varunagrawal]

Also, GTSAM_ROT3_EXPMAP should be the default now, so getting rid of the SLOW_BUT_CORRECT_BETWEENFACTOR flag would make a lot of sense.

[lucacarlone]

@varunagrawal : it has been a while, but I believe the Jacobian in SLOW_BUT_CORRECT_BETWEENFACTOR uses the correct Jacobian for the factor (and in particular for the Logarithm map involved in the between factor) rather than assuming that the Logmap Jacobian is the identity (only true when the error is zero). Using the correct jacobian largely improves convergence. I'm happy to talk more if you have specific questions about that.

[varunagrawal]

@lucacarlone I see. In that case, it seems like other than fixing the convergence values for the test cases, the only derivatives we need to implement are for SO4 and Similarity3. Time to go into math mode I guess.

[lucacarlone]

@varunagrawal : let me know if you need help. Also, make sure @dellaert is ok with adding the missing Jacobians.