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

Separate Pybind modules for inner namespaces #49

Open
5 tasks
varunagrawal opened this issue Jul 28, 2020 · 6 comments
Open
5 tasks

Separate Pybind modules for inner namespaces #49

varunagrawal opened this issue Jul 28, 2020 · 6 comments
Assignees

Comments

@varunagrawal
Copy link
Collaborator

Feature

Currently, all namespaces under the top level gtsam (e.g. symbol_shorthand) are added as pybind submodules. This requires the python import to look like

from gtsam.gtsam.symbol_shorthand import X

aka a gtsam module inside a gtsam module.

We can get rid of the inner gtsam module by configuring the pybind wrapper to create separate modules for all the inner namespaces, allowing us to do

from gtsam.symbol_shorthand import X

which is more intuitive and neater, especially since import gtsam.symbol_shorthand works.

Motivation

Outlined above.

Pitch

Update the Pybind wrapper to have separate modules for each inner namespace.
Currently there are 5:

  • noiseModel
  • mEstimator
  • symbol_shorthand
  • imuBias
  • utilities

Alternatives

N/A

Additional context

@ProfFan should be able to provide details on whether this is doable without breaking other stuff since he's the lead on the Pybind wrapper. :-)

@ProfFan
Copy link
Collaborator

ProfFan commented Jul 28, 2020

It's just time consuming 🤷 I feel it's more like a cosmetic issue (and I feel that it may be possible to solve with a trick)

[spoiler]

add a symbol_shorthands subfolder, put a __init__.py there, and add delegates.

@dellaert
Copy link
Member

I do agree with @varunagrawal there is an issue here. @ProfFan cosmetics matters for our users.
Also, "adding delegates" may be something that adds maintenance issues?
Typing is not the bottleneck here so I'd like to avoid tricks if possible.

@ProfFan
Copy link
Collaborator

ProfFan commented Jul 28, 2020

@dellaert It's because we are currently having this one massive header and it's not trivial to escape this design pattern, as the wrap script is designed for this. Maybe in the future we can split the headers so this won't be an issue, but for now, I don't see an easy way to get this working.

@varunagrawal
Copy link
Collaborator Author

So I actually did quite a bit of research on this today. I'd be glad to put in a PR for this once I'm done with my summer commitments.

@dellaert
Copy link
Member

Could you write up the solution in an issue? Maybe with links?

@dellaert
Copy link
Member

Or, this issue, of course :-)

@varunagrawal varunagrawal transferred this issue from borglab/gtsam Mar 22, 2021
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

No branches or pull requests

3 participants