Initial commits for geomstats module #1
Conversation
rigid_transformations.py
Outdated
| Compute the group inverse in SE(3). | ||
|
|
||
| Formula: | ||
| TODO(nina). |
There was a problem hiding this comment.
I added the formula :)
rigid_transformations.py
Outdated
| from the identity to transfo in the Lie group SE(3). | ||
|
|
||
| Formula: | ||
| TODO(nina). |
There was a problem hiding this comment.
syntax: TODO(nina): add formula
| return prod_transfo | ||
|
|
||
|
|
||
| def jacobian_translation(transfo, left_or_right='left'): |
There was a problem hiding this comment.
what about direction instead of left_or_right?
There was a problem hiding this comment.
Left or Right is the actual mathematical term: it comes from the left- or right- translations associated to the group composition resp. by the left- or the right-.
There was a problem hiding this comment.
cool thanks for the clarification
rigid_transformations.py
Outdated
| jacobian[3:, 3:] = np.eye(3) | ||
|
|
||
| else: | ||
| raise ValueError('\'left_or_right\' needs \'left\' or \'right\'') |
There was a problem hiding this comment.
you should probably do the argument checking at the beginning:
assert left_or_right in ('left', 'right')
rigid_transformations.py
Outdated
| return tangent_vector | ||
|
|
||
|
|
||
| # --- Riemannian |
There was a problem hiding this comment.
comment is kinda useless. if you really want to make it clear that these are specific, maybe put this in a riemannian.py file? more of a nit though
There was a problem hiding this comment.
Yes, actually when I told you that I wanted to factorize code this is because the code on riemannian is general across rotations and rigid transformations. So I will indeed create a separate file riemannian.py and I want to use OO code for lie groups to use the riemannian functions.
I'll only delete the comments for now.
There was a problem hiding this comment.
you can add a todo if you want
visualization.py
Outdated
| import rotations | ||
|
|
||
|
|
||
| class Arrow3D(): |
visualization.py
Outdated
| from matplotlib import pyplot as plt | ||
| from mpl_toolkits.mplot3d import Axes3D | ||
|
|
||
| # import rigid_transformations |
visualization.py
Outdated
| @@ -0,0 +1,78 @@ | |||
| """Visualization for Geometric Statistics.""" | |||
|
|
|||
| # import pylab | |||
visualization.py
Outdated
| fig = plt.figure() | ||
| ax = fig.add_subplot(111, projection='3d') | ||
| trihedron.draw(ax, **kwargs) | ||
| plt.show() |
There was a problem hiding this comment.
you may also want to return a figure instead of showing here. And then ppl can use the figure how they see fit (like save it etc.)
visualization.py
Outdated
| return trihedron | ||
|
|
||
|
|
||
| def plot_trihedron(trihedron, **kwargs): |
There was a problem hiding this comment.
kinda hate kwargs... At the very least say which function you're passing the kwards too. Something like
plot_trihedron(trihedron, **draw_kwargs)
There was a problem hiding this comment.
You advice not to use them in general? Isn't it tedious to copy-paste all plotting parameters from fn to fn?
| @@ -0,0 +1,78 @@ | |||
| """Unit tests for rigid_transformations module.""" | |||
There was a problem hiding this comment.
so the canonical syntax for tests is:
in the same directory, create a file that starts with the module that you want to test and suffix _test.py
this will work with self discovery for the unit test library
There was a problem hiding this comment.
Actually, it does not work with self discovery but test_[module name].py works. I.e. prefix instead of suffix.
unittests/test_rotations.py
Outdated
|
|
||
| rot_vec_3 = 11 * np.pi * np.array([1., 2., 3.]) | ||
| rot_vec_3 = rotations.regularize_rotation_vector(rot_vec_3) | ||
| fact = 0.84176874548664671 * np.pi / np.sqrt(14) |
There was a problem hiding this comment.
where does this constant come from? can you be a bit more clear into the kind of things you're testing here?? Even though I usually hate comments, in unit tests it's fine.
| all_transfos = [transfo_1, transfo_2, transfo_3, transfo_4] | ||
| i_debug = 1 | ||
| for transfo in all_transfos: | ||
| print('at i=%d' % i_debug) |
There was a problem hiding this comment.
so one tip for this test: here you rely on 2 things for the test to pass: your regularization function and your transformation function. I would recommend not relying on the regularization for testing your transformation - or this is not a unit test and it makes debugging harder.
|
I wrote more and (hopefully) cleaner unit tests. I kept the unit tests that use several functions, as I want to test not only the code, but also the maths. |
Update to version of jan 14 2020
Tristancabel custom gradient
backend: Added "endpoint" support to linspace #1
No description provided.