boost::optional<const Ordering &> #156

gchenfc · 2019-10-20T07:25:11Z

Get rid of boost::optional, this PR is for Ordering and Scatter classes (should fix #148 ).
Replace all usages of boost::optional<*Ordering*> with function overloads.

compiles and passes tests, but some potentially code-breaking changes in:

Marginals.h - order of arguments had to change since factorization has a default value
EliminatableFactorGraph.h - marginalMultifrontalBayesNet and marginalMultifrontalBayesTree no longer accept boost::none as a placeholder to specify later arguments
NonlinearFactorGraph.h - same deal

Notes:

EliminateableFactorGraph.h - orderingType is no longer needed in function overloads that specify ordering, but I left it for the time being to avoid potential code breaking
NonlinearOptimizerParams.h and LevenbergMarquardParams.h still have a parameter that's a boost::optional<Ordering> but I left it in because it's not readily handle-able by a single function overload (not sure what other code depends on it being optional)

This change is

…s &>)` to overloaded `func(...)` and `func(..., const Class &)` not all done Also, although it currently passes all tests, I think some more tests may be needed...

compiles and passes tests, but some potentially code-breaking changes in: Marginals.h - order of arguments had to change since `factorization` has a default value EliminatableFactorGraph.h - marginalMultifrontalBayesNet and marginalMultifrontalBayesTree no longer accept `boost::none` as a placeholder to specify later arguments Notes: EliminateableFactorGraph.h - `orderingType` is no longer needed in function overloads that specify ordering, but I left it for the time being to avoid potential code breaking

gchenfc

some notes

gchenfc · 2019-10-20T07:31:03Z

gtsam/linear/JacobianFactor.cpp


  // Cast or convert to Jacobians
  FastVector<JacobianFactor::shared_ptr> jacobians = _convertOrCastToJacobians(
      graph);

-  gttic(Order_slots);


4 new constructors (generated from combinations of 2 optional parameters) will use this same code so i moved it into a separate function, not sure why diff util didn't recognize it better

gchenfc · 2019-10-20T07:35:26Z

gtsam/nonlinear/Marginals.h

-  Marginals(const NonlinearFactorGraph& graph, const Values& solution, Factorization factorization = CHOLESKY,
-            EliminateableFactorGraph<GaussianFactorGraph>::OptionalOrdering ordering = boost::none);
+  Marginals(const NonlinearFactorGraph& graph, const Values& solution, Factorization factorization = CHOLESKY);
+
+  /** Construct a marginals class from a nonlinear factor graph.
+   * @param graph The factor graph defining the full joint density on all variables.
+   * @param solution The linearization point about which to compute Gaussian marginals (usually the MLE as obtained from a NonlinearOptimizer).
+   * @param factorization The linear decomposition mode - either Marginals::CHOLESKY (faster and suitable for most problems) or Marginals::QR (slower but more numerically stable for poorly-conditioned problems).
+   * @param ordering The ordering for elimination.
+   */
+  Marginals(const NonlinearFactorGraph& graph, const Values& solution, const Ordering& ordering, // argument order switch due to default value of factorization, potentially code breaking
+              Factorization factorization = CHOLESKY);


note ordering now comes before factorization because factorization has a default value

gchenfc · 2019-10-20T07:37:31Z

gtsam/nonlinear/NonlinearFactorGraph.cpp

+  KeyVector keys = values.keys();
+  Ordering defaultOrdering(keys);
+  return linearizeToHessianFactor(values, defaultOrdering, dampen);


is this tactic of creating a "default" ordering inefficient compared to creating a helper function? (as I did with the rest of the PR)

gchenfc · 2019-10-20T07:42:46Z

gtsam/inference/EliminateableFactorGraph.h

    boost::shared_ptr<BayesTreeType> marginalMultifrontalBayesTree(
      boost::variant<const Ordering&, const KeyVector&> variables,
-      OptionalOrdering marginalizedVariableOrdering = boost::none,
+      const Ordering& marginalizedVariableOrdering, // this no longer takes boost::none - potentially code breaking
      const Eliminate& function = EliminationTraitsType::DefaultEliminate,
      OptionalVariableIndex variableIndex = boost::none) const;


potentially breaks code,
~~marginalMultifrontalBayesTree(variables, boost::none, EliminationQR, variableIndex);~~ now becomes
marginalMultifrontalBayesTree(variables, EliminationQR, variableIndex);

gchenfc · 2019-10-20T07:44:39Z

gtsam/nonlinear/NonlinearFactorGraph.cpp

-    const Values& values, boost::optional<Ordering&> ordering, const Dampen& dampen) const {
+    const Values& values, const Dampen& dampen) const {


potentially breaks code,
~~linearizeToHessianFactor(values, boost::none, dampen)~~ changes to
linearizeToHessianFactor(values, dampen)

added deprecated function to .h file

dellaert · 2019-10-20T09:29:23Z

Unfortunately, API breaking/ changing is not allowed here. So we have to find another solution

ProfFan · 2019-10-20T20:05:40Z

Can we have a flag that enables another overload for boost::optionals?

dellaert

I added a bunch of comments. Did not get to Marginals yet, but I assume some of earlier comments I made establish a pattern...

gtsam/base/TestableAssertions.h

gtsam/discrete/DiscreteConditional.cpp

gtsam/inference/EliminateableFactorGraph.h

gtsam/linear/GaussianBayesNet.cpp

dellaert · 2019-10-20T20:03:21Z

gtsam/linear/HessianFactor.cpp

@@ -250,10 +250,10 @@ HessianFactor::HessianFactor(const GaussianFactor& gf) :

 /* ************************************************************************* */
 HessianFactor::HessianFactor(const GaussianFactorGraph& factors,
-    boost::optional<const Scatter&> scatter) {
+    const Scatter& scatter) {


Are we doing scatter in this PR?

I included Scatter to address issue #148

dellaert · 2019-10-20T20:04:58Z

gtsam/linear/JacobianFactor.cpp

+// Order variable slots - we maintain the vector of ordered slots, as well as keep a list
+// 'unorderedSlots' of any variables discovered that are not in the ordering.  Those will then
+// be added after all of the ordered variables.
+FastVector<VariableSlots::const_iterator> orderedSlotsHelper(


Did not read in detail but is this a delegated constructor pattern?

I instead put it in a member function rather than delegated constructor for a couple reasons:

stuff needs to get done before the shared code, i.e.
JacobianFactor() {
// specific stuff creates some_variables
// common stuff relies on some_variables
}

I didn't want to mess up the gttic() timings

In fact actually there are 2 helper functions (orderedSlotsHelper and JacobianFactorHelper), see constructor in line 409 for the quintessential example.

I also converted boost::optional<const VariableSlots &> while I was here because it was convenient, but I can revert that if you prefer to keep this PR tidy.

dellaert · 2019-10-20T20:07:23Z

Can we have a flag that enables another overload for boost::optionals?

Let’s keep this PR focused...

also removed some edits that were tangential to the PR.

gchenfc

addressed comments for API breaking changes, unrelated changes, and others.

gtsam/base/TestableAssertions.h

gtsam/discrete/DiscreteConditional.cpp

gchenfc · 2019-10-28T13:51:39Z

gtsam/linear/HessianFactor.cpp

@@ -250,10 +250,10 @@ HessianFactor::HessianFactor(const GaussianFactor& gf) :

 /* ************************************************************************* */
 HessianFactor::HessianFactor(const GaussianFactorGraph& factors,
-    boost::optional<const Scatter&> scatter) {
+    const Scatter& scatter) {


I included Scatter to address issue #148

gchenfc · 2019-10-28T14:57:27Z

gtsam/linear/JacobianFactor.cpp

+// Order variable slots - we maintain the vector of ordered slots, as well as keep a list
+// 'unorderedSlots' of any variables discovered that are not in the ordering.  Those will then
+// be added after all of the ordered variables.
+FastVector<VariableSlots::const_iterator> orderedSlotsHelper(


I instead put it in a member function rather than delegated constructor for a couple reasons:

stuff needs to get done before the shared code, i.e.
JacobianFactor() {
// specific stuff creates some_variables
// common stuff relies on some_variables
}

I didn't want to mess up the gttic() timings

In fact actually there are 2 helper functions (orderedSlotsHelper and JacobianFactorHelper), see constructor in line 409 for the quintessential example.

I also converted boost::optional<const VariableSlots &> while I was here because it was convenient, but I can revert that if you prefer to keep this PR tidy.

gchenfc · 2019-10-28T16:16:23Z

gtsam/linear/Scatter.cpp

-  }
-
-  // Now, find dimensions of variables and/or extend
+void Scatter::setDimensions(const GaussianFactorGraph& gfg, size_t sortStart) {


changed to delegated constructor

gtsam/inference/EliminateableFactorGraph.h

gchenfc · 2019-10-28T19:56:25Z

gtsam/inference/EliminateableFactorGraph.h

+     *  \endcode
+     *  */
+    boost::shared_ptr<BayesTreeType> eliminateMultifrontal(
+      const Ordering& ordering,


gtsam/inference/EliminateableFactorGraph.h

gchenfc · 2019-10-28T20:12:50Z

gtsam/nonlinear/NonlinearFactorGraph.cpp

-    const Values& values, boost::optional<Ordering&> ordering, const Dampen& dampen) const {
+    const Values& values, const Dampen& dampen) const {


added deprecated function to .h file

dellaert

I have no time to do a second, detailed review. I propose @ProfFan goes through fine comb, and you wait to merge until he signs off. However, please resist expanding the scope of this PR, and please make absolutely sure it is backwards compatible. All API-breaking needs to be in deprecated blocks.

ProfFan · 2019-11-01T18:23:13Z

@gchenfc We need to have API stability tests probably :(
Also, the previous boost::Optional&-based I/F should probably be retained with deprecated macros https://stackoverflow.com/questions/295120/c-mark-as-deprecated

dellaert · 2019-11-01T18:38:07Z

No, I vote not to retain them. Let's merge this once API calls are verified (by a detailed review, not by additional tests). Again, don't expand scope...

ProfFan · 2019-11-01T18:49:58Z

@dellaert I am all good with that 👍 The current testset should have already validated the API calls, but I will double check.

gtsam/inference/EliminateableFactorGraph.h

dellaert · 2019-11-13T15:22:23Z

Status?

ProfFan · 2019-11-13T19:31:14Z

Good to go.

dellaert · 2019-11-13T19:58:27Z

And, @ProfFan , by "good to go" you mean : you verified it's 100% backward compatible, yes? :-)

ProfFan · 2019-11-13T20:56:02Z

It is 99% backward compatible due to the fact that if someone used the pattern xxx(args, boost::none) instead of xxx(args)(using the default value) then there will be an error that boost::none cannot be converted to TypeOfArgument. Otherwise it would be just fine.

ProfFan · 2019-11-13T20:57:53Z

BTW, this can be 100% if we retain the old boost::Optional<T&> interface.

gchenfc · 2019-11-18T20:41:40Z

Apologies for late reply, but I had already added deprecated overloads with boost::none_t to preserve this 1% of non-backwards compatibility.
For example, xxx(args, boost::optional<>) -> xxx(args, boost::none_t) For example, line 328 of EliminateableFactorGraph.h. I didn't think to do this until later though so I may have missed a couple. I can go back and check again if I missed any?

ProfFan · 2019-11-19T02:13:31Z

@gchenfc Feel free to check and add :) Currently I have not found any issue in the current revision.

ba3dcab16 Merge pull request #156 from borglab/fix/matlab-properties feb4ee1b9 update docs e57fec56b add unit test for templated class property git-subtree-dir: wrap git-subtree-split: ba3dcab16a8316634b56e3c4c6061531c91eb36c

Gerry Chen added 3 commits October 13, 2019 23:59

Some low hanging fruit changing `func(..., boost::optional<const Clas…

5a35848

…s &>)` to overloaded `func(...)` and `func(..., const Class &)` not all done Also, although it currently passes all tests, I think some more tests may be needed...

Caught some stragglers using boost::optional<Ordering>

4877bdb

gchenfc requested review from dellaert and ProfFan October 20, 2019 07:25

gchenfc commented Oct 20, 2019

View reviewed changes

dellaert reviewed Oct 20, 2019

View reviewed changes

deprecated functions for backwards compatibility

8e62a14

also removed some edits that were tangential to the PR.

gchenfc commented Oct 28, 2019

View reviewed changes

Update test cases for new preferred syntax

e40a500

dellaert approved these changes Nov 1, 2019

View reviewed changes

ProfFan requested changes Nov 1, 2019

View reviewed changes

gtsam/inference/EliminateableFactorGraph.h Show resolved Hide resolved

ProfFan approved these changes Nov 13, 2019

View reviewed changes

ProfFan merged commit ed7f949 into develop Nov 13, 2019

ProfFan mentioned this pull request Dec 20, 2019

Boost warning #11

Closed

dellaert deleted the fix/boost-optional branch January 2, 2020 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boost::optional<const Ordering &> #156

boost::optional<const Ordering &> #156

gchenfc commented Oct 20, 2019 •

edited

Loading

gchenfc left a comment

gchenfc Oct 20, 2019

gchenfc Oct 20, 2019

gchenfc Oct 20, 2019

gchenfc Oct 20, 2019

gchenfc Oct 20, 2019

gchenfc Oct 28, 2019

dellaert commented Oct 20, 2019

ProfFan commented Oct 20, 2019

dellaert left a comment

dellaert Oct 20, 2019

gchenfc Oct 28, 2019

dellaert Oct 20, 2019

gchenfc Oct 28, 2019

dellaert commented Oct 20, 2019

gchenfc left a comment

gchenfc Oct 28, 2019

gchenfc Oct 28, 2019

gchenfc Oct 28, 2019

gchenfc Oct 28, 2019

gchenfc Oct 28, 2019

dellaert left a comment

ProfFan commented Nov 1, 2019

dellaert commented Nov 1, 2019

ProfFan commented Nov 1, 2019

dellaert commented Nov 13, 2019

ProfFan commented Nov 13, 2019

dellaert commented Nov 13, 2019

ProfFan commented Nov 13, 2019

ProfFan commented Nov 13, 2019

gchenfc commented Nov 18, 2019 •

edited

Loading

ProfFan commented Nov 19, 2019

		const Values& values, boost::optional<Ordering&> ordering, const Dampen& dampen) const {
		const Values& values, const Dampen& dampen) const {

boost::optional<const Ordering &> #156

boost::optional<const Ordering &> #156

Conversation

gchenfc commented Oct 20, 2019 • edited Loading

gchenfc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dellaert commented Oct 20, 2019

ProfFan commented Oct 20, 2019

dellaert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dellaert commented Oct 20, 2019

gchenfc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dellaert left a comment

Choose a reason for hiding this comment

ProfFan commented Nov 1, 2019

dellaert commented Nov 1, 2019

ProfFan commented Nov 1, 2019

dellaert commented Nov 13, 2019

ProfFan commented Nov 13, 2019

dellaert commented Nov 13, 2019

ProfFan commented Nov 13, 2019

ProfFan commented Nov 13, 2019

gchenfc commented Nov 18, 2019 • edited Loading

ProfFan commented Nov 19, 2019

gchenfc commented Oct 20, 2019 •

edited

Loading

gchenfc commented Nov 18, 2019 •

edited

Loading