artivis / manif

A small C++11 header-only library for Lie theory.
https://artivis.github.io/manif
MIT License
1.47k stars 242 forks source link

[manifpy] General improvements in SO3 and SE3 objects #211

Closed GiulioRomualdi closed 3 years ago

GiulioRomualdi commented 3 years ago

This PR fixes #210 and #209

In details: ~~#209 is fixed by https://github.com/artivis/manif/commit/fdd136b182c7554c7f9344c98168c65ffffc1ce2 where the bindings are installed in a folder in <CMAKE_INSTALL_PREFIX>/<PYTHON_INSTALL_DIR>/manifpy where PYTHON_INSTALL_DIR is given by~~

from distutils import sysconfig 
print(sysconfig.get_python_lib(1,0,prefix=''))

in ubuntu 20.04 the script returns lib/python3/dist-packages

210 is fixed by https://github.com/artivis/manif/commit/759debb15ec0cf58aab01b5650104fe4e6b550e8 Here I also added an assert to check that the quaternion is unitary. Finally https://github.com/artivis/manif/commit/2a3342a38607afcb83077ea32dda4bd60b67046d and https://github.com/artivis/manif/commit/64567d6b1a98aa4b119c9b1a1e5ab9a6cfd17fc4 add some constructors in python that simplifies the creation of SE3 and SO3 objects

This is an example of usage. First of all, I updated the bash with the following lines

 export PYTHONPATH=${PYTHONPATH}:${MANIF_INSTALL_PREFIX}/lib/python3/dist-packages

Now it's possible to run the following python script everywhere in the system

import manifpy as manif                                                                      

so3 = manif.SO3(quaternion=[1, 0, 0, 0])
print(so3)

# so3 = manif.SO3(quaternion=[2, 0, 0, 0]) <---- This generates a python exeption                            

se3 = manif.SE3(position=[0,2,0], quaternion=[1, 0, 0, 0])
print(se3.translation)
print(se3.quat)

# se3.quat = [2, 0, 0, 0] <---- This generates a python exception
codecov[bot] commented 3 years ago

Codecov Report

Merging #211 (af1828e) into devel (5846bdd) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##            devel     #211   +/-   ##
=======================================
  Coverage   98.01%   98.02%           
=======================================
  Files          49       49           
  Lines        1462     1466    +4     
=======================================
+ Hits         1433     1437    +4     
  Misses         29       29           
artivis commented 3 years ago

Hi, thanks for the contribution. You may want to split this in two distinct PRs (see my comment on #209). Note that manifpy is available system-wide when installed through pip.

GiulioRomualdi commented 3 years ago

Hi @artivis. I've just installed manifpy with pip and everything went fine. So if you agree I can just remove https://github.com/artivis/manif/pull/211/commits/9737c8e84017f8edf20e8c48e9fa2402ac2f1beb from the PR

artivis commented 3 years ago

Ci is currently failing because of cppcheck. Related to #212.

GiulioRomualdi commented 3 years ago

Hi @artivis, I applied your suggestions. Let me know if you are fine with those