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.

Sign up for GitHub

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

Jump to bottom

Update "print" function arguments #738

Merged
merged 8 commits into from
Apr 29, 2021
Merged

Update "print" function arguments #738

merged 8 commits into from
Apr 29, 2021

Conversation

gchenfc
Copy link
Member

@gchenfc gchenfc commented Apr 8, 2021

After borglab/wrap#86 gets merged, it will allow optional arguments for the gtsam wrapper interface file. This updates the gtsam.i definitions to take advantage of these for the print statements.

This actually basically accomplishes the same thing as overloading... except that it also supports kwargs which can be handy. e.g.

factor.print_('factor: ', lambda key : return "the key is {}".format(key))
factor.print_(keyFormatter = lambda key : return "the key is {}".format(key))

Admittedly, the value add is not super large but imho it can still simplify the wrapping a little bit.

gchenfc added 5 commits April 8, 2021 04:34
@gchenfc
Squashed 'wrap/' changes from 5ddaff8ba..b43f7c6d7
78210f3 
b43f7c6d7 Merge pull request #80 from borglab/feature/multiple_interface_files
7b9d080f5 Revert "unit test for printing with keyformatter"
45e203195 Revert "funcitonal print passes unit test"
8b5d66f03 Revert "include functional and iostream"
34bfbec21 Revert "delete debug statement"
dbe661c93 Revert "add includes to the example tpl file."
f47e23f1a Revert "Revert "function to concatenate multiple header files together""
e667e2095 Revert "function to concatenate multiple header files together"
600c77bf4 add includes to the example tpl file.
d5caca20e delete debug statement
4a554edb8 include functional and iostream
e8ad2c26f funcitonal print passes unit test
077d5c4e7 unit test for printing with keyformatter
d5a1e6dff function to concatenate multiple header files together

git-subtree-dir: wrap
git-subtree-split: b43f7c6d7d6cb50ebe585d7e38390e2bfeb51dde
@gchenfc
Merging 'master' into 'wrap'
4222c88
@gchenfc
update gtsam.i print function declarations
36ab168
@gchenfc
update default args to match with c++
7065885
@gchenfc
include pybind11/function
8c6efb5
@gchenfc gchenfc added the wip This is still a work in progress label Apr 8, 2021
@gchenfc gchenfc added python Related to python wrapper and removed wip This is still a work in progress labels Apr 21, 2021
@gchenfc
Copy link
Member Author

gchenfc commented Apr 21, 2021

Note: this does the same as d0d8f48 from #750 but keeps the same default arguments as the c++ defaults, and also adds the keyFormatter optional arguments to the classes which accept it.

@dellaert
Copy link
Member

Ping me for a re-review when you and @varunagrawal agree on a solution

varunagrawal
varunagrawal reviewed Apr 21, 2021
View reviewed changes
gtsam/gtsam.i Outdated
@@ -1236,7 +1239,8 @@ class SymbolicBayesNet {
SymbolicBayesNet();
SymbolicBayesNet(const gtsam::SymbolicBayesNet& other);
// Testable
void print(string s="") const;
void print(string s = "BayesNet",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be SymbolicBayesNet?

varunagrawal
varunagrawal requested changes Apr 21, 2021
View reviewed changes
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.

Looks fine to me, but we we should make the default s values consistent with the C++ defaults or update them to the subclass value.

gtsam/gtsam.i Outdated
@@ -1582,7 +1590,8 @@ class VectorValues {
#include <gtsam/linear/GaussianFactor.h>
virtual class GaussianFactor {
gtsam::KeyVector keys() const;
void print(string s="") const;
void print(string s = "Factor",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep it empty like in the header file?

gtsam/gtsam.i Outdated
@@ -1610,7 +1619,8 @@ virtual class JacobianFactor : gtsam::GaussianFactor {
JacobianFactor(const gtsam::GaussianFactorGraph& graph);

//Testable
void print(string s="") const;
void print(string s = "Factor",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep it empty like in the header file?

gtsam/gtsam.i Outdated
@@ -1659,7 +1669,8 @@ virtual class HessianFactor : gtsam::GaussianFactor {

//Testable
size_t size() const;
void print(string s="") const;
void print(string s = "Factor",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep it empty like in the header file?

gtsam/gtsam.i Outdated
@@ -1684,7 +1695,8 @@ class GaussianFactorGraph {
GaussianFactorGraph(const gtsam::GaussianBayesTree& bayesTree);

// From FactorGraph
void print(string s="") const;
void print(string s = "FactorGraph",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leave this empty or change it to GaussianFactorGraph?

Copy link
Member

Choose a reason for hiding this comment

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

Should be same default as c++ which should not change @varunagrawal so change before merge if you did change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the default for the base class, so it will also print in the subclass. Should I override the print method?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify, the print in FactorGraph has "FactorGraph" as the default and since GaussianFactorGraph inherits from FactorGraph, it also prints "FactorGraph" as the default (which to me isn't quite right).

Copy link
Member

Choose a reason for hiding this comment

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

Not this PR to fix it

@varunagrawal
Copy link
Collaborator

I went ahead and updated this PR since I want to land it at the earliest.

@varunagrawal
Copy link
Collaborator

Okay I am merging this and will create a new PR for overriding the prints.

@varunagrawal varunagrawal merged commit 1431407 into develop Apr 29, 2021
@varunagrawal varunagrawal deleted the feature/update_print_wrap branch April 29, 2021 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dellaert dellaert dellaert approved these changes

@varunagrawal varunagrawal varunagrawal approved these changes

Assignees
No one assigned
Labels
python Related to python wrapper
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gchenfc @dellaert @varunagrawal