NoiseModelFactorN - fixed-number of variables >6 #947

gchenfc · 2021-12-01T21:10:26Z

Although this doesn't have an immediate use-case, I just wanted to work on this as a de-stressing exercise. Will not feel bad if it is rejected.

Currently there is NoiseModelFactor1, NoiseModelFactor2, ..., NoiseModelFactor6 but nothing higher, so if you want to implement n-way factors where n is known at compile time but greater than 6, you have to derive from NoiseModelFactor and implement unwhitenedError. This is fine, but I thought it'd be nice to have an n-way version that still has the convenient evaluateError syntax.

This might also be nice to use in place of existing NoiseModelFactor1-6 classes in the future since, if the number of arguments for a given factor ever changes, it requires fewer changes (N is figured out by the compiler rather than hard-coded).

varunagrawal · 2021-12-02T00:56:40Z

Dude, I love this!

dellaert

I love this. Is there a way to replace some of the earlier factors with a typedef to this new factor? has to be 100% compatible of course.

gchenfc · 2021-12-03T02:59:15Z

Thanks!

I love this. Is there a way to replace some of the earlier factors with a typedef to this new factor? has to be 100% compatible of course.

I did consider this but decided to wait what you thought first. Glad you feel similarly :)

I think the only extra things that wouldn't be possible with the typedef would be:

inline Key key1() const { return keys_[0]; } etc.
typedef VALUE1 X1; etc.

Because these have literal numbers in their names (and macros are processed before templates so we can't use macro magic to get these names).

After fixing the CI failure, I'll draft a potential solution and see what you think.

dellaert · 2021-12-03T04:26:19Z

We can just add key1 to key6 and have it fail if N< less than six :-) There might even be template magic to only instantiate these methods up to N.

…ypes/funcs in NoiseModelFactorN

gchenfc · 2021-12-03T12:53:26Z

Oh true I forgot about that!

I implemented that - it's mostly done, except that the using syntax:

template <class VALUE1, class VALUE2, ...., class VALUE6>
using NoiseModelFactor6 = NoiseModelFactorN<VALUE1, VALUE2, VALUE3, VALUE4, VALUE5, VALUE6>;

is causing some serialization issues. Here's a MWE:
https://godbolt.org/z/WjrbfWMnT
It's totally expected that this doesn't work since BaseAlias is not a real type without specifying template arguments, but what is perplexing is that if you change using BaseAlias = Base<T>; to struct BaseAlias : Base<T> {}; it works, even though BaseAlias is still not a type without giving it template parameters. Or even if you use Base which is also templated (and how it is currently in GTSAM) it still works. Strange.

I'll think about this more later and maybe ask @ProfFan or @varunagrawal for help.

dellaert · 2021-12-07T17:49:25Z

Ping @gchenfc - I prefer you ask help from someone not Varun or Fan - both racing to deadlines.

Reasoning is given as comments.

gtsam/nonlinear/NonlinearFactor.h

dellaert · 2021-12-09T06:23:01Z

gtsam/nonlinear/NonlinearFactor.h

@@ -272,503 +307,306 @@ class GTSAM_EXPORT NoiseModelFactor: public NonlinearFactor {


 /* ************************************************************************* */
+/* We need some helper structs to help us with NoiseModelFactorN - specifically


A great exercise in meta-programming which I’d love to see disappear, see comment below :-)

gtsam/nonlinear/NonlinearFactor.h

gtsam/mainpage.dox

gtsam/nonlinear/NonlinearFactor.h

jlblancoc · 2021-12-09T12:07:13Z

Might be doable with the typename... Args with clearer syntax? See e.g. #444
Just a first guess, perhaps it's not possible in this case for some reason.

gchenfc · 2021-12-09T15:05:48Z

Might be doable with the typename... Args with clearer syntax? See e.g. #444 Just a first guess, perhaps it's not possible in this case for some reason.

Thanks so much for the tip @jlblancoc! Would you mind clarifying which aspects you were thinking to include this? It's currently being used quite a bit here, e.g.

gtsam/gtsam/nonlinear/NonlinearFactor.h

Lines 404 to 406 in 11fd861

    
           template <class... VALUES> 
        
           class NoiseModelFactorN 
        
               : public NoiseModelFactor,

gtsam/gtsam/nonlinear/NonlinearFactor.h

Lines 519 to 520 in 11fd861

    
           virtual Vector evaluateError(const VALUES&... x, 
        
                                        optional_matrix_type<VALUES>... H) const = 0;

but I'm hung up on all the "custom" additions for backwards compatibility, e.g. Key key1 const { return keys_[0]; } and typedef X2 VALUE2 which are the majority of what is making this so ugly at the moment - would absolutely love any ideas/suggestions for alternatives if you have any!

jlblancoc · 2021-12-09T23:02:55Z

@gchenfc Ah, very good job then, IMO you are on the right track! 👍

but I'm hung up on all the "custom" additions for backwards compatibility, e.g. Key key1 const { return keys_[0]; }

I'm afraid gtsam is so used, it needs that backwards compatibility, so it should be kept despite the "ugliness" ;-)

PS: I can't think of any "elegant" way to circumvent it, neither...

gchenfc · 2021-12-09T23:34:26Z

@jlblancoc Cool, thanks for sharing + advice! much appreciated :)

varunagrawal

@gchenfc this looks great except for some nitpicks. Can you please make the modifications and we can land this?

gtsam/nonlinear/NonlinearFactor.h

gchenfc · 2022-11-16T16:07:00Z

Thanks @varunagrawal for the review! I'll go through and add DEPRECATED - I think you created that after I had originally made this PR so I'm glad you reminded me of this! Can you also confirm the other 2 items and, if you still want those changes, I'll add those changes too.

@gchenfc Will I be able to test this NoiseModelFactorN in Matlab? What is holding up the merge on this PR? I know me and my lab mate have both needed this in the past.

I don't think this can directly be used in Matlab since NoiseModelFactorN is templated and you need to supply the types of all the variables during compile-time. Actually, now that I think of it, I forget how e.g. NoiseModelFactor2 is even used in Matlab, since you can't define custom factors? Because NoiseModelFactor1, ..., NoiseModelFactorN are primarily used as base classes for defining custom factors. But it would be easier (less copy-paste) to define e.g. NoiseModelFactor8<T1, T2, ..., T8> if hypothetically you wanted it. If it's just for linear, we can do that in a different PR. If it's for nonlinear, I'm not sure how this would help in Matlab since custom factors need to be defined in C++ anyway (unless using Fan's CustomFactor class).
And I think the PR was previously just waiting for review, so thanks varun again :)

varunagrawal · 2022-11-16T17:37:12Z

GTSAM_DEPRECATED was already present even before I started working with GTSAM. @dellaert and I have been deprecating stuff as we revisit components for hybrid estimation (e.g. EliminateForMPE) which is how I know about it, so passing that knowledge on. :)

@mattking-smith we'll be able to write a C++ class with 8+ variables (which isn't doable in current develop) and then we can wrap that class. We can talk about it offline (especially since now that I better understand what you're doing).

gchenfc · 2022-11-16T19:59:05Z

Thanks! Will merge once CI completes, assuming that github lets me; we may still need an approval from @dellaert since he previously requested changes (that have since been fixed I think).

dellaert

Some comments. This PR is a year old now :-) Could we meet and put a pin in it ? I really like the idea and want to have it in 4.2.

gtsam/nonlinear/NonlinearFactor.h

dellaert · 2022-12-13T22:40:39Z

@gchenfc

This reverts commit d62033a.

This is a byproduct of the overload resolution problem when N=1, then it can be hard to differentiate between: NoiseModelFactorN(noise, key) NoiseModelFactorN(noise, {key})

dellaert · 2022-12-22T17:17:50Z

PS, re-reading some above, we definitely need to check that both wrappers (python and matlab) still work. @mattking-smith might be able to help with latter...

gchenfc · 2022-12-23T07:01:25Z

PS, re-reading some above, we definitely need to check that both wrappers (python and matlab) still work. @mattking-smith might be able to help with latter...

Tested both and all pass!

Python is tested by CI and I also ran on my machine, all pass.

Matlab I tested on my machine and all pass, after needing to change some instances of symbol -> Symbol due to my computer having case insensitive filenames.

Also, I put the "changing gtsam pre-made factors to inherit from NoiseModelN instead" into a new PR to keep things separate.

dellaert

This is an amazing piece of work, Gerry! Will merge so it’s ready to go into 4.2 :-)

NoiseModelFactorN - fixed-number of variables >6

81f1d93

gchenfc requested a review from dellaert December 1, 2021 21:15

c++11 doesn't support std::size so use obj.size() instead

e037fa1

dellaert reviewed Dec 2, 2021

View reviewed changes

gchenfc added 9 commits December 2, 2021 23:26

alternate make_index_sequence impl if no boost::mp11

d9c8ce2

optional jacobian overloads backwards compatibility

2aecaf3

backward compatibility unit tests for NoiseModelFactor4

8fe7e48

NoiseModelFactor4 implemented as derived class of NoiseModelFactorN

bee4eee

converted all NoiseModelFactorX to inherit from NoiseModelFactorN

ed07edb

documentation

ea7d769

avoid inheritance by conditionally defining backwards compatibility t…

ba3cc85

…ypes/funcs in NoiseModelFactorN

switch template bool specialization order

ddcca4c

can't get "using NoiseModelFactorX = NoiseModelFactorN" to work

280acde

gchenfc added 4 commits December 8, 2021 18:29

alternate option for typedef-ing X1, X2, ...

89b4340

revert typedef X1, X2, ... to old version, and clean up a little

5004c47

switch using NoiseModelFactorX = ... to #define ....

018213e

Reasoning is given as comments.

fix Windows CI issue: VALUE happens to have the same name in PriorFactor

84e873e

gchenfc requested a review from dellaert December 9, 2021 05:56

dellaert requested changes Dec 9, 2021

View reviewed changes

gchenfc added 2 commits December 9, 2021 02:26

review comments

40e585b

update doxygen (review comment)

11fd861

varunagrawal requested review from mattking-smith and removed request for dellaert November 10, 2022 19:38

varunagrawal approved these changes Nov 10, 2022

View reviewed changes

address review comments

322e555

dellaert reviewed Dec 9, 2022

View reviewed changes

gtsam/nonlinear/NonlinearFactor.h Outdated Show resolved Hide resolved

gtsam/nonlinear/NonlinearFactor.h Outdated Show resolved Hide resolved

gtsam/nonlinear/NonlinearFactor.h Outdated Show resolved Hide resolved

gtsam/nonlinear/NonlinearFactor.h Outdated Show resolved Hide resolved

gchenfc added 5 commits December 19, 2022 17:20

fix boost 1.65 patch bug

94865c4

Revert "fix namespace collision with symbol_shorthand::X in unit test"

63950b9

This reverts commit d62033a.

Change X<N> to ValueType<N> and VALUES -> ValueTypes

0ebc6e8

address review comments

b24511f

Check type of CONTAINER constructor tparam

e8ddbbe

This is a byproduct of the overload resolution problem when N=1, then it can be hard to differentiate between: NoiseModelFactorN(noise, key) NoiseModelFactorN(noise, {key})

gchenfc requested a review from dellaert December 22, 2022 15:02

gchenfc added 5 commits December 22, 2022 13:18

make SFINAE templates more readable

040eb63

better docstrings w/ usage examples

d16d263

Change backwards-compatibility defs to utilize new style

4b93970

update and fix unit tests

19215af

Merge branch 'develop' into feature/NoiseModelFactorN_replaceDeprecated

2a7efc7

gchenfc mentioned this pull request Dec 23, 2022

Replace NoiseModelFactor1 in with NoiseModelFactorN in pre-made factors #1344

Merged

gchenfc requested review from dellaert and removed request for dellaert December 23, 2022 07:03

dellaert approved these changes Dec 23, 2022

View reviewed changes

dellaert merged commit 8886ad1 into develop Dec 23, 2022

gchenfc deleted the feature/NoiseModelFactorN branch December 27, 2022 02:46

This was referenced Jan 5, 2023

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

Closed

Un-deprecate NoiseModelFactor1-6 and move X1 and key1-6 shortcuts to NoiseModelFactorN #1370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NoiseModelFactorN - fixed-number of variables >6 #947

NoiseModelFactorN - fixed-number of variables >6 #947

gchenfc commented Dec 1, 2021 •

edited

Loading

varunagrawal commented Dec 2, 2021

dellaert left a comment

gchenfc commented Dec 3, 2021

dellaert commented Dec 3, 2021 •

edited

Loading

gchenfc commented Dec 3, 2021 •

edited

Loading

dellaert commented Dec 7, 2021

dellaert Dec 9, 2021

jlblancoc commented Dec 9, 2021

gchenfc commented Dec 9, 2021

jlblancoc commented Dec 9, 2021 •

edited

Loading

gchenfc commented Dec 9, 2021 •

edited

Loading

varunagrawal left a comment

gchenfc commented Nov 16, 2022

varunagrawal commented Nov 16, 2022

gchenfc commented Nov 16, 2022

dellaert left a comment

dellaert commented Dec 13, 2022

dellaert commented Dec 22, 2022

gchenfc commented Dec 23, 2022 •

edited

Loading

dellaert left a comment

		@@ -272,503 +307,306 @@ class GTSAM_EXPORT NoiseModelFactor: public NonlinearFactor {


		/* ************************************************************************* */
		/* We need some helper structs to help us with NoiseModelFactorN - specifically

NoiseModelFactorN - fixed-number of variables >6 #947

NoiseModelFactorN - fixed-number of variables >6 #947

Conversation

gchenfc commented Dec 1, 2021 • edited Loading

varunagrawal commented Dec 2, 2021

dellaert left a comment

Choose a reason for hiding this comment

gchenfc commented Dec 3, 2021

dellaert commented Dec 3, 2021 • edited Loading

gchenfc commented Dec 3, 2021 • edited Loading

dellaert commented Dec 7, 2021

dellaert Dec 9, 2021

Choose a reason for hiding this comment

jlblancoc commented Dec 9, 2021

gchenfc commented Dec 9, 2021

jlblancoc commented Dec 9, 2021 • edited Loading

gchenfc commented Dec 9, 2021 • edited Loading

varunagrawal left a comment

Choose a reason for hiding this comment

gchenfc commented Nov 16, 2022

varunagrawal commented Nov 16, 2022

gchenfc commented Nov 16, 2022

dellaert left a comment

Choose a reason for hiding this comment

dellaert commented Dec 13, 2022

dellaert commented Dec 22, 2022

gchenfc commented Dec 23, 2022 • edited Loading

dellaert left a comment

Choose a reason for hiding this comment

gchenfc commented Dec 1, 2021 •

edited

Loading

dellaert commented Dec 3, 2021 •

edited

Loading

gchenfc commented Dec 3, 2021 •

edited

Loading

jlblancoc commented Dec 9, 2021 •

edited

Loading

gchenfc commented Dec 9, 2021 •

edited

Loading

gchenfc commented Dec 23, 2022 •

edited

Loading