Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Wrap GNC #843

Merged
merged 13 commits into from
Aug 13, 2021
Merged

Wrap GNC #843

merged 13 commits into from
Aug 13, 2021

Conversation

johnwlambert
Copy link
Contributor

@johnwlambert johnwlambert commented Aug 11, 2021

Add GNC to the wrapper.

@johnwlambert
Copy link
Contributor Author

johnwlambert commented Aug 11, 2021

@ProfFan hi Fan, any thoughts on why the Python CI fails here? resolved via Varun's comments

@@ -522,6 +522,14 @@ virtual class DoglegParams : gtsam::NonlinearOptimizerParams {
void setVerbosityDL(string verbosityDL) const;
};

#include <gtsam/nonlinear/GncParams.h>
template<class BaseOptimizerParameters>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You shouldn't be specifying templates as template<class T>, it should be just template<T>.
Same deal for GncOptimizer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thank you Varun

Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

I have a feeling this will compile but won't import since the templates don't have instantiations.

@@ -522,6 +522,14 @@ virtual class DoglegParams : gtsam::NonlinearOptimizerParams {
void setVerbosityDL(string verbosityDL) const;
};

#include <gtsam/nonlinear/GncParams.h>
template<BaseOptimizerParameters>
Copy link
Collaborator

@varunagrawal varunagrawal Aug 12, 2021

Choose a reason for hiding this comment

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

Nitpick: Use a smaller uppercase name like PARAMS and provide either a default via the template list PARAMS = {SomeValue} or use the typedef to declare one (I prefer the former).

Without a template list of typedef, the python module import should error out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, i checked and it compiled but import didn't work.

I updated the templates, let me know if this is what you had in mind @varunagrawal. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

weird, even with the updated templates we still get

======================================================================
ERROR: test_NonlinearOptimizer (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_NonlinearOptimizer
Traceback (most recent call last):
  File "/usr/lib/python3.6/unittest/loader.py", line 428, in _find_test_path
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name
    __import__(name)
  File "/home/runner/work/gtsam/gtsam/python/gtsam/tests/test_NonlinearOptimizer.py", line 18, in <module>
    from gtsam import (DoglegOptimizer, DoglegParams,
ImportError: cannot import name 'GncParams'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing this is because we were missing GTSAM_EXPORT calls, and because the class wasn't marked as virtual in the .i files. Do we have those steps documented somewhere in gtwrap, along with why they are required? I'm a bit confused about that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The wrapper appends the template name to disambiguate between instantiations, e.g. PinholeCameraCal3_S2 vs PinholeCameraCal3Bundler. You'll have to import the full name from gtsam import GncOptimizerGncParamsGaussNewtonParams.

Better design would be to use

#include <gtsam/nonlinear/GncParams.h>
template<PARAMS>
virtual class GncParams {
  GncParams(const PARAMS& baseOptimizerParams);
  GncParams();
  void print(const string& str) const;
};

typedef GncParams<gtsam::GaussNewtonParams> GncGaussNewtonParams;
typedef GncParams<gtsam::LevenbergMarquardtParams> GncLMParams;


#include <gtsam/nonlinear/GncOptimizer.h>
template<PARAMS>
virtual class GncOptimizer {
  GncOptimizer(const gtsam::NonlinearFactorGraph& graph,
               const gtsam::Values& initialValues,
               const PARAMS& params);
  gtsam::Values optimize();
};

typedef gtsam::GncOptimizer<gtsam::GncParams<gtsam::GaussNewtonParams>> GncGaussNewtonOptimizer;
typedef gtsam::GncOptimizer<gtsam::GncParams<gtsam::LevenbergMarquardtParams>> GncLMOptimizer;

You should then be able to do

from gtsam import GncLMParams, GncLMOptimizer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @varunagrawal, looks like all the tests pass now.

gtsam/nonlinear/nonlinear.i Outdated Show resolved Hide resolved
Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

Sweet!

@johnwlambert johnwlambert merged commit 4e1e762 into develop Aug 13, 2021
@johnwlambert johnwlambert deleted the wrap-gnc branch August 13, 2021 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants