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

Python print Values and NonlinearFactorGraph #136

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

gchenfc
Copy link
Member

@gchenfc gchenfc commented Apr 7, 2021

This fixes #131 by:

  1. creating a static gtdynamics::str(obj, s) function that calls obj.print(s, GTDKeyFormatter)
  2. creating pure-python "wrapper" classes around gtsam.Values and gtsam.NonlinearFactorGraph that override repr. Based on this SO answer. Now use gtd.Values instead of gtsam.Values - it should behave identically in every way except it will print using GTDKeyFormatter instead.
fg = gtd.NonlinearFactorGraph()
fg.push_back(gtd.MinTorqueFactor(gtd.internal.TorqueKey(0, 0).key(),
                                 gtsam.noiseModel.Unit.Create(1)))
print(fg)

will output:

size: 1

Factor 0: min torque factor
  keys = { T(0)0 }
  noise model: unit (1) 

I think I have also come up with a way make the GTDKeyFormatter function available in python (i.e. kf = gtd.GetKeyFormatter; fg.print_('fg', kf) and also supports functionals, but it will take some changes to wrap. i'll put that (unit tests + one-liner) in a different PR after wrap PR.

@varunagrawal
Copy link
Collaborator

This is ingenious!! But very hacky :(

I would prefer to

  1. Have NonlinearFactorGraph and Values overload operator<< so we can directly do print(fg).
  2. Update the print methods in GTSAM to use the key formatters assigned to each factor. So unless the user passes in a KeyFormatter, it should use the factor's default KeyFormatter which would be gtsam::DefaultKeyFormatter or GTDKeyformatter depending on the factor type.
  3. Add functional support to wrap so we can pass in functions and lambdas. This is easy for me to do but I am swamped by other stuff.

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.

There might be more substantial changes to gtsam that could make this better, I agree with varun on that, but I think it’s good for now. One thing I ask is that you add a README file in the python folder that explains this and (later) other peculiarities of the wrapper

@gchenfc gchenfc merged commit 4aa7bcd into master Apr 8, 2021
@varunagrawal varunagrawal deleted the feature/keyformatting branch April 19, 2021 17:21
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.

GTDKeyformatter is not working
3 participants