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

[WIP] Add Python bindings to manif #179

Closed
wants to merge 67 commits into from
Closed

[WIP] Add Python bindings to manif #179

wants to merge 67 commits into from

Conversation

artivis
Copy link
Owner

@artivis artivis commented Dec 7, 2020

manifpy

Add Python bindings to manif.

Install,

sudo apt install -y libeigen3-dev cmake build-essential python3-pip
python3 -m pip install --upgrade pip
python3 -m pip install pytest "pybind11[global]"
cd manif/
python3 -m pip install -r requirements.txt
python3 -m pip install .

Test,

cd manif/
pytest

Use,

from manifpy import SE3

state = SE3.Identity()
...

Note

rjacinv/ljacinv

The functions rjacinv/ljacinv are not exposed at the moment. They are too complicated to wrap for little benefit.
This will be fixed in the future after the internal API redesign (see e.g. #150).

Eigen/Geometry-related API

The C++ API related to Eigen/Geometry is not exposed. The rational is that this API only exists as a convenience to interface with existing classes in Eigen (Quaternion/Transform etc).
The Eigen counterpart in Python is numpy which does not have such classes. We could wrap the Eigen objects to expose them to Python but that does not have much interest since they wont play nice with numpy unless we put some extra effort.
At the moment this seems to be too much effort for little outcome, especially since the different examples (_localization/_sam) were ported to Python without these classes and related API.

For this reason this will be left to future work or contribution.

Todo list

Bindings:

  • manif group-base API
  • manif tangent-base API
    • missing rjacinv/ljacinv
  • group specific API
    • missing Eigen/Geometry-related
  • port common test suite to python
  • port group specific tests to python

Misc:

  • python packaging
  • documentation
    • Python doc generation
    • unified manif website
    • automatic doc update in CI
  • examples
    • se2_localization
    • se3_localization
    • se2_sam
    • se3_sam
    • se3_sam_selfcalib
    • se2_3_localization

Blocked by #190
Closes #187

@artivis artivis added the enhancement New feature or request label Dec 7, 2020
@artivis artivis self-assigned this Dec 7, 2020
@artivis
Copy link
Owner Author

artivis commented Dec 7, 2020

@joansola FYI

@joansola
Copy link
Collaborator

joansola commented Dec 7, 2020

Cooooool!!! :-D

@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #179 (a76ae35) into devel (48ba49d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            devel     #179   +/-   ##
=======================================
  Coverage   98.37%   98.37%           
=======================================
  Files          49       49           
  Lines        1475     1475           
=======================================
  Hits         1451     1451           
  Misses         24       24           

@Edwinem
Copy link

Edwinem commented Dec 12, 2020

Damn I was actually making my own version of this, and was going to finish it and add it over the christmas break. I guess you beat me to it.

One idea I was playing around with is to add a cmake flag to enable building the library with EIGEN_DEFAULT_TO_ROW_MAJOR. That way if someone is using the library purely for Python they don't have to deal with the numpy<-->Eigen compatibility issues. I don't know though if this breaks the rest of the library though.

@artivis
Copy link
Owner Author

artivis commented Dec 14, 2020

Hi @Edwinem,

There is still much to do with the binding. So far only the API common to all groups is exposed, not even completely - there are a few road-blockers I couldn't figure out yet... Remain group-specific API, more testing, proper Python packaging, a couple examples, documentation etc... So, feel free to contribute if you feel like it ;)
Regarding the numpy<-->Eigen issue, so far I've addressed it on the numpy side (so to speak) specifying the storage order (see e.g. here). Your suggestion using EIGEN_DEFAULT_TO_ROW_MAJOR is interesting and I'll give it a go 👍.

@artivis
Copy link
Owner Author

artivis commented Dec 17, 2020

@joansola The python bindings has reached a usable state. The complete group/tangent base API is accessible through Python and the common test suite is ported and passing. There are still a few things that I'd like to discuss,

  • there exists a couple functions that are named the same in manif and I'd like to fix that. For instance tan.w() return the 'inner' weight matrix but se3_tan.w() return the angular block while so3.w() return the quaternion's 'w'. I'd propose to rename the first 2 occurrences and keep w() solely for the quaternion's 'w'. Any naming suggestions?
  • the Python package name is manifpy but I think pymanif sounds better. I'm not sure there is even a standard for that beside the usual Python naming conventions. I have seen both, py as a prefix and suffix...
  • unlike cpp, python offer a neat opportunity to name input arguments, for instance, all of the following expressions are valid:
    state_out = state.compose(state_other, J_sout_s)
    state_out = state.compose(state_other, J_sout_s, J_sout_so)
    state_out = state.compose(state_other, J_mc_ma = J_sout_s, J_mc_mb = J_sout_so)
    state_out = state.compose(state_other, J_mc_mb = J_sout_so, J_mc_ma = J_sout_s)
    state_out = state.compose(state_other, J_mc_ma = J_sout_s)
    state_out = state.compose(state_other, J_mc_mb = J_sout_so)
    Note that the argument names above are J_mc_ma and J_mc_mb.
    I'd like to uniformise the arguments naming with something simple for all functions. Maybe simply J_out_a, J_out_b following the pattern out = f(a, b, ...). What do you think?

@joansola
Copy link
Collaborator

@joansola The python bindings has reached a usable state. The complete group/tangent base API is accessible through Python and the common test suite is ported and passing. There are still a few things that I'd like to discuss,

  • there exists a couple functions that are named the same in manif and I'd like to fix that. For instance tan.w() return the 'inner' weight matrix but se3_tan.w() return the angular block while so3.w() return the quaternion's 'w'. I'd propose to rename the first 2 occurrences and keep w() solely for the quaternion's 'w'. Any naming suggestions?

yes:

  • innerWeightMatrix() -- I guess this is seldom used, so long name
  • omega()
  • w()
  • the Python package name is manifpy but I think pymanif sounds better. I'm not sure there is even a standard for that beside the usual Python naming conventions. I have seen both, py as a prefix and suffix...

I know of numpy only... or yamlcpp for the same matter. I'd go for manifpy. But I dont care much about this one.

  • unlike cpp, python offer a neat opportunity to name input arguments, for instance, all of the following expressions are valid:
    state_out = state.compose(state_other, J_sout_s)
    state_out = state.compose(state_other, J_sout_s, J_sout_so)
    state_out = state.compose(state_other, J_mc_ma = J_sout_s, J_mc_mb = J_sout_so)
    state_out = state.compose(state_other, J_mc_mb = J_sout_so, J_mc_ma = J_sout_s)
    state_out = state.compose(state_other, J_mc_ma = J_sout_s)
    state_out = state.compose(state_other, J_mc_mb = J_sout_so)

What's the = doing here?

Note that the argument names above are J_mc_ma and J_mc_mb.
I'd like to uniformise the arguments naming with something simple for all functions. Maybe simply J_out_a, J_out_b following the pattern out = f(a, b, ...). What do you think?

Yeah this sounds great as a generic naming convention for Jacobians. I do always like this and insist a lot that people do too.

@artivis
Copy link
Owner Author

artivis commented Dec 19, 2020

yes:

  • omega()

What about the linear block?

I know of numpy only... or yamlcpp for the same matter. I'd go for manifpy. But I dont care much about this one.

Ok then manifpy it is.

What's the = doing here?

Sorry I should have detailed the Python function signature, it roughly is:

def compose(other, J_mc_ma, J_mc_mb):
    ...

So here other, J_mc_ma and J_mc_ma are the argument names. In Python you can pass arguments either by position

state_out = state.compose(state_other, J_sout_s, J_sout_so)

or by name,

state_out = state.compose(other = state_other, J_mc_ma = J_sout_s, J_mc_mb = J_sout_so)

which let you change their position if you wish

state_out = state.compose(J_mc_mb = J_sout_so, J_mc_ma = J_sout_s, other = state_other)

That's why I'm paying attention to the naming here.
After some more thinking we could use out, other, J_out_self, J_out_other following the patterns out = state.inverse( J_out_self ) and out = state.compose( other, J_out_self, J_out_other). This naming is a little more programmatic but also I think clearer.

@joansola
Copy link
Collaborator

  • omega()
    What about the linear block?

uhm, yes, maybe then angVel(), linVel() is a clearer way to go. What do you think?

What's the = doing here?
or by name,

state_out = state.compose(other = state_other, J_mc_ma = J_sout_s, J_mc_mb = J_sout_so)

cool!!

After some more thinking we could use out, other, J_out_self, J_out_other following the patterns out = state.inverse( J_out_self ) and out = state.compose( other, J_out_self, J_out_other). This naming is a little more programmatic but also I think clearer.

+1

@artivis
Copy link
Owner Author

artivis commented Dec 19, 2020

uhm, yes, maybe then angVel(), linVel() is a clearer way to go. What do you think?

For SE_2_3 that would lead to linVel(), angVel() and linAcc(). It ain't no pretty but I don't have a better idea atm.

Maybe @prashanthr05 has a suggestion?

@prashanthr05
Copy link
Contributor

uhm, yes, maybe then angVel(), linVel() is a clearer way to go. What do you think?

For SE_2_3 that would lead to linVel(), angVel() and linAcc(). It ain't no pretty but I don't have a better idea atm.

Maybe @prashanthr05 has a suggestion?

I also agree with this naming. It is quite clear as to what the underlying motion vector means.

@artivis
Copy link
Owner Author

artivis commented Jan 11, 2021

Added Python packaging (see updated description).

@Edwinem FYI. Do you plan on adding some example or should I port them myself?

@artivis
Copy link
Owner Author

artivis commented Jan 13, 2021

@joansola, I added the localization examples in Python (2D/3D). See e.g. se3_localization.py.

@artivis
Copy link
Owner Author

artivis commented Jan 24, 2021

This PR is mostly ready at this point. I'll finish up the documentation generation and open a ticket to complete the Python API doc.
Then, I'll clean up the PR itself and likely break it down so that it only contains the Python bindings bits.

@artivis
Copy link
Owner Author

artivis commented Jan 30, 2021

New documentation website live at https://artivis.github.io/manif/

@joansola
Copy link
Collaborator

Website colors are rather unfortunate. Math formulas are not visible due to the dark gray background.

@artivis artivis mentioned this pull request Feb 4, 2021
6 tasks
@artivis
Copy link
Owner Author

artivis commented Feb 4, 2021

Website colors are rather unfortunate. Math formulas are not visible due to the dark gray background.

Right I'll see that fixed 👍

@artivis artivis closed this Feb 13, 2021
@artivis artivis deleted the feature/python branch February 13, 2021 20:31
@artivis artivis mentioned this pull request Feb 13, 2021
17 tasks
@artivis
Copy link
Owner Author

artivis commented Feb 13, 2021

Replaced by #201

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python Bindings discussion
4 participants