Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 4 commits into from
Jan 5, 2023

Conversation

gchenfc
Copy link
Member

@gchenfc gchenfc commented Jan 5, 2023

Following the discussion from #1350, this PR replaces #1350 and undoes some changes from #1344:

  1. Moves X1-X6 aliases into NoiseModelFactorN (not deprecated)
  2. Un-deprecates key1()-key6() shortcuts in NoiseModelFactorN
  3. #define NoiseModelFactor1 NoiseModelFactorN. Now that NoiseModelFactorN's API is a superset of NoiseModelFactor1-6, we can simply define it so.

As for 3, template <T> using NoiseModelFactor1 = NoiseModelFactorN<T> doesn't work because some detail regarding templates and class name injection. See #947 (comment)

Primary change is in NonlinearFactor.h. The rest of the files are just undo-ing some changes from #1344 and un-disabling deprecation warnings in testNonlinearFactor.h.


Copy-pasting pros and cons from previous discussion for reference:

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 slightly difficult to read (now inherits from detail::NoiseModleFactorAliases<ValueTypes>).
  • since we would do #define NoiseModelFactor1 NoiseModelFactorN, some compilers may not support deprecating the old classes, and generally #define is kind of bad practice (?)

@gchenfc gchenfc requested a review from dellaert January 5, 2023 04:13
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wow, this is much better !!!!

@dellaert
Copy link
Member

dellaert commented Jan 5, 2023

Feel free to merge when CI passes

@gchenfc gchenfc merged commit 7bd4556 into develop Jan 5, 2023
@gchenfc gchenfc deleted the feature/NoiseModelFactorN_undeprecate branch January 5, 2023 06:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants