Add trivial groups Rn #109

artivis · 2019-11-14T00:12:56Z

Add the trivial groups Rn.

Close #108.

artivis · 2019-11-14T00:14:57Z

@joansola This PR is not fully finished yet but ready for a first round of review.
Tests are passing with R2d to R9d.

FYI I'm experiencing some test regression on my machine. Some SO3 test are failing due to precision, some other results (sometime) in segfault.
I'm not sure what has changed, possibly my compiler version, I'll have to check. While investigating I got concerned that we possibly have some memory alignment issues lying around. More investigation required...

artivis · 2019-11-14T00:15:29Z

@markusgft FYI.

codecov-io · 2019-11-14T00:35:52Z

Codecov Report

Merging #109 into devel will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            devel     #109      +/-   ##
==========================================
+ Coverage   98.08%   98.21%   +0.13%     
==========================================
  Files          32       36       +4     
  Lines        1046     1123      +77     
==========================================
+ Hits         1026     1103      +77     
  Misses         20       20

joansola

Voila my first round of review.

include/manif/impl/macro.h

include/manif/impl/rn/Rn.h

include/manif/impl/rn/RnTangent_base.h

include/manif/impl/rn/Rn_base.h

include/manif/impl/se2/SE2.h

joansola · 2019-11-14T08:06:15Z

test/common_tester.h

@@ -8,9 +8,7 @@
 #define MANIF_TEST(manifold)                                              \


Did you run all the standard tests on Rn?

Yes, the tests do pass locally and on Travis CI. See the tests here and Travis CI here.

Does your answer still apply after further commits?

Yes, only the documentation-related test is failing. I'll look it up.

joansola · 2019-11-14T15:11:29Z

OK good job. We have a warning TODO on the smallAdj, I just put it here for highlighting it.

joansola · 2019-11-14T15:13:05Z

A side, general question: Why Rn and not e.g. Tn? In the paper I wrote T(n) for these groups. T is for 'translation'.

This comment is no big deal. But it might be interesting to come up with a name that the community can feel right.

artivis · 2019-11-14T15:22:46Z

OK good job. We have a warning TODO on the smallAdj, I just put it here for highlighting it.

Yeah I could use the macro that throws a 'Not implemented yet' message for safety.
Also we should implement a generic test for smallAdj, I couldn't find any after a quick search.

artivis · 2019-11-14T15:25:36Z

A side, general question: Why Rn and not e.g. Tn? In the paper I wrote T(n) for these groups. T is for 'translation'.

I used Rn because you seemed to favor it in #108 discussion. I don't mind renaming it (once) so lets agree on what it should be.

joansola · 2019-11-14T15:36:13Z

OK good job. We have a warning TODO on the smallAdj, I just put it here for highlighting it.

Yeah I could use the macro that throws a 'Not implemented yet' message for safety.
Also we should implement a generic test for smallAdj, I couldn't find any after a quick search.

I know of some properties of smallAdj but I am afraid they only hold for multiplicative groups:

for x,y \in Tangent, then

adj_x * y = vee( xy - yx)

For additive groups this may collapse to a trivial thing, since x*y = x+y and then the whole thing goes to zero -- does this mean that smallAdj = 0?

This is the mistery and the danger by now.

joansola · 2019-11-14T15:39:15Z

A side, general question: Why Rn and not e.g. Tn? In the paper I wrote T(n) for these groups. T is for 'translation'.

I used Rn because you seemed to favor it in #108 discussion. I don't mind renaming it (once) so lets agree on what it should be.

I tend to favor T(n). But unlike SO(n) which are well established, and also S^n, the name T(n) I just invented it from thin air in the paper. I have seen T(n) to signify the torus of dimension n, which is another thing.

artivis · 2019-11-14T16:10:05Z

I tend to favor T(n). But unlike SO(n) which are well established, and also S^n, the name T(n) I just invented it from thin air in the paper. I have seen T(n) to signify the torus of dimension n, which is another thing.

Ok then we have to be careful, maybe Tn is not the best solution.

joansola · 2019-11-14T16:23:36Z

I tend to favor T(n). But unlike SO(n) which are well established, and also S^n, the name T(n) I just invented it from thin air in the paper. I have seen T(n) to signify the torus of dimension n, which is another thing.

Ok then we have to be careful, maybe Tn is not the best solution.

From Wikipedia https://en.wikipedia.org/wiki/Translation_(geometry)

Seems that T(n) is a good name.

From wikipedia https://en.wikipedia.org/wiki/Euclidean_group

The E(n) group is the Euclidean group, and it is not the same because Euclidean group includes rotation.

From StackExchange https://math.stackexchange.com/questions/666870/generators-of-translation-lie-algebra

the group is essentially R^n , and when enriched with the additive composition is a group isomorphic to the matrix group of translation matrices `M = [ I t ; 0 1 ]`.

I feel going for T(n) is safe. I do not like R^n because it is not a group, it is a set, or a space, and it lacks the group law of composition. You could say that T(n) = {R(n), +}.

artivis · 2019-11-14T16:36:09Z

for x,y \in Tangent, then

adj_x * y = vee( xy - yx)

Did you mean,

adj_x*y^ = x^*y^ − y^*x^ ?
That's the Lie bracket [x^,y^] right ?

artivis · 2019-11-14T16:38:15Z

I feel going for T(n) is safe. I do not like R^n because it is not a group, it is a set, or a space, and it lacks the group law of composition. You could say that T(n) = {R(n), +}.

Ok then I'll rename to Tn in a bit, in case you change your mind ;p

joansola · 2019-11-14T16:44:02Z

for x,y \in Tangent, then
adj_x * y = vee( xy - yx)

Did you mean,

adj_x*y^ = x^*y^ − y^*x^ ?
That's the Lie bracket [x^,y^] right ?

Yes and no. We are mixing lie algebra and vector representations of the tangent elements x and y. Also, the small adjoint that we have is in reality the small adjoint matrix. Here are the equivalences:

Given:

Vectors x, y in tangent space
Elements x^, y^ in Lie algebra
Small adjoint operator adj_x
Small adjoint matrix [adj_x]

In Lie algebra, we have

adj_x(y^) = x^*y^ - y^*x^       <-- yes, this is the Lie bracket

In vector space we have

[adj_x] * y = ( x^*y^ - y^*x^ ) v

where 'v' is the vee operator. The two forms above are equivalent because of the isomorphism relating the vector space and the Lie algebra.

But, the thing is, in additive groups like the one we are handling, we have x^=x and *=+, so the Lie bracket resolves to zero:

(x+y) - (y+x) = 0

and so I am a little confused as to what is the small adjoint in such case. Following this reasoning, it should be simply [adj_x] = 0 !!

artivis · 2019-11-14T16:50:43Z

Oh yeah I had forgot that distinction adj / [adj]...
Ok then the only issue with implementing [adj_x] * y = ( x^*y^ - y^*x^ ) v is that we don't have the v operator implemented.
I'll add it to the todo list...

joansola · 2019-11-14T16:51:15Z

BTW, one way to see this is the following:

Consider T(n) as the subgroup of SE(n) with null rotation (that is, R = I).

Then, the adjoint of T(n) and the adjoint of SE(n) should be equal if we set theta=0 in the adjoint of SE(n).

Of course the adjoint of SE(n) has a larger dimension. We must look at the translation part only.

But if it happens that in SE(n) we have adj = 0 when theta=0, then we can conclude that the adjoint of T(n) is also zero.

joansola · 2019-11-14T17:04:15Z

I feel going for T(n) is safe. I do not like R^n because it is not a group, it is a set, or a space, and it lacks the group law of composition. You could say that T(n) = {R(n), +}.

Ok then I'll rename to Tn in a bit, in case you change your mind ;p

According to wikipedia https://en.wikipedia.org/wiki/Table_of_Lie_groups

The group name is finally R^n !!!!

:-)

joansola · 2019-11-14T17:10:56Z

All my research leads to smallAdj = 0_{nxn}. But let us not claim victory yet.

artivis · 2019-11-14T17:11:46Z

According to wikipedia https://en.wikipedia.org/wiki/Table_of_Lie_groups
The group name is finally R^n !!!!

Great, less work for me :)

joansola · 2019-11-14T17:17:27Z

All my research leads to smallAdj = 0_{nxn}. But let us not claim victory yet.

Another way to see it is through the formula given by Eade and Fourmy etal

J_r(\bftau)  = \sum_i \frac{(-\ad{\bftau})^i}{(i+1)!}

which is an infinite sum of powers of adj, starting with adj^0=I. Since J_r = I, we have that adj must be zero!

joansola · 2019-11-14T20:53:42Z

OK Just for the fun of it. Another way to show that the small adjoint is zero.

Take the matrix form of the group. For translations t and s in Rn, their Lie algebras are tau = [ 0 t ; 0 0 ] and sigma = [ 0 s ; 0 0 ] in R^{(n+1)x(n+1)}.

Now the small adjoint operator is

adj_tau (sigma) = tau*sigma - sigma*tau = [0 0;0 0] - [0 0;0 0] = [0 0;0 0]

which is a perfect zero. The matrix version is consequently zero too:

[adj_t] * s = [0 0;0 0]v = 0 in R^n

since this is true for all t and all s, then necessarily,

[adj_t] = 0 in R^{n x n}

voila.

(again, 'v' is the vee operator and returns a vector from a matrix in the Lie algebra).

joansola

This is OK for me now.

artivis · 2019-11-14T22:59:21Z

CI is happy now!
I only have to update the doc and the readme then this is good to be merged.

joansola · 2019-11-15T10:37:29Z

@artivis FYI

[==========] 828 tests from 36 test suites ran. (719 ms total)
[  PASSED  ] 825 tests.
[  FAILED  ] 3 tests, listed below:
[  FAILED  ] TEST_R1f_JACOBIANS_TESTER.TEST_R1f_RMINUS_JACOBIANS
[  FAILED  ] TEST_R1f_JACOBIANS_TESTER.TEST_R1f_LMINUS_JACOBIANS
[  FAILED  ] TEST_R1f_JACOBIANS_TESTER.TEST_R1f_MINUS_JACOBIANS

 3 FAILED TESTS

full log of failed tests:

[ RUN      ] TEST_R1f_JACOBIANS_TESTER.TEST_R1f_RMINUS_JACOBIANS
/Users/jsola/dev/manif/test/rn/../common_tester.h:727: Failure
Value of: manif::isManifNear(state_pert, state_lin, "state_pert", "state_lin", tol_)
  Actual: false (state_pert != state_lin
state_pert:
0.011310875415802002
state_lin:
0.011310858651995659
diff:
1.6763806343078613e-08
)
Expected: true
/Users/jsola/dev/manif/test/rn/../common_tester.h:734: Failure
Value of: manif::isManifNear(state_pert, state_lin, "state_pert", "state_lin", tol_)
  Actual: false (state_pert != state_lin
state_pert:
0.011417090892791748
state_lin:
0.011417107656598091
diff:
-1.6763806343078613e-08
)
Expected: true
[  FAILED  ] TEST_R1f_JACOBIANS_TESTER.TEST_R1f_RMINUS_JACOBIANS (1 ms)
[ RUN      ] TEST_R1f_JACOBIANS_TESTER.TEST_R1f_LMINUS_JACOBIANS
/Users/jsola/dev/manif/test/rn/../common_tester.h:747: Failure
Value of: manif::isManifNear(state_pert, state_lin, "state_pert", "state_lin", tol_)
  Actual: false (state_pert != state_lin
state_pert:
0.011310875415802002
state_lin:
0.011310858651995659
diff:
1.6763806343078613e-08
)
Expected: true
/Users/jsola/dev/manif/test/rn/../common_tester.h:754: Failure
Value of: manif::isManifNear(state_pert, state_lin, "state_pert", "state_lin", tol_)
  Actual: false (state_pert != state_lin
state_pert:
0.011417090892791748
state_lin:
0.011417107656598091
diff:
-1.6763806343078613e-08
)
Expected: true
[  FAILED  ] TEST_R1f_JACOBIANS_TESTER.TEST_R1f_LMINUS_JACOBIANS (0 ms)
[ RUN      ] TEST_R1f_JACOBIANS_TESTER.TEST_R1f_MINUS_JACOBIANS
/Users/jsola/dev/manif/test/rn/../common_tester.h:767: Failure
Value of: manif::isManifNear(state_pert, state_lin, "state_pert", "state_lin", tol_)
  Actual: false (state_pert != state_lin
state_pert:
0.011310875415802002
state_lin:
0.011310858651995659
diff:
1.6763806343078613e-08
)
Expected: true
/Users/jsola/dev/manif/test/rn/../common_tester.h:774: Failure
Value of: manif::isManifNear(state_pert, state_lin, "state_pert", "state_lin", tol_)
  Actual: false (state_pert != state_lin
state_pert:
0.011417090892791748
state_lin:
0.011417107656598091
diff:
-1.6763806343078613e-08
)
Expected: true
[  FAILED  ] TEST_R1f_JACOBIANS_TESTER.TEST_R1f_MINUS_JACOBIANS (0 ms)

These errors are very unlikely--> We are testing that the linear approximation is good enough. But in Rn, the linear approximation should be exact!!!

f(X+d,Y) = f(X,Y) + J_d * d

with f(X,Y) = X+Y and J_d = I , this reduces to

f(X+d,Y) = (X+d) + Y = X+Y + d

and

f(X,Y) + J_d * d = X+Y + d

so the relation is exactly equal !!!!

joansola · 2019-11-15T17:23:27Z

BTW the failed tests above were on my Mac OSX. Maybe on Linux the error is slightly smaller and the tests pass. But see my comments above: the test should pass with error zero!!!

joansola · 2019-11-15T19:53:01Z

FYI I'm experiencing some test regression on my machine. Some SO3 test are failing due to precision, some other results (sometime) in segfault.
I'm not sure what has changed, possibly my compiler version, I'll have to check. While investigating I got concerned that we possibly have some memory alignment issues lying around. More investigation required...

No idea what can cause this test regression!

Maybe investigating the Rn tests (comments above) we can find a common cause for all these weakly passing tests?

artivis · 2019-11-18T23:41:03Z

Did you make this test generic? The test is as follows:
Tangent x, y;
x = random(); y = random();
x.smallAdj() * y = (x.hat() * y.hat() - y.hat() * x.hat() ).vee();
or, if we do not have vee(),
(x.smallAdj() * y).hat() = x.hat() * y.hat() - y.hat() * x.hat() ;

See #114.

joansola · 2019-11-27T10:47:43Z

What's the status of this PR?

I have one only concern: that hat() and vee() transform to the Lie algebra of the matrix translation group T(n), that is x.hat = [ 0 x ; 0 0].

I guess we can live with it, but strictly speaking in the translation vector group Rn we have x.vee() = x, which continues to be a vector of R^n...

artivis · 2019-12-02T22:57:46Z

What's the status of this PR?

I just replaced x.hat() so that x.hat()=x.
@joansola If you agree, I will merge it once the tests are passed.

markusgft · 2019-12-03T08:46:32Z

Thanks again @artivis and @joansola for taking action so quickly, I really appreciate it!

joansola · 2019-12-03T09:35:13Z

@artivis but then how about the test of smallAdj?

(x.smallAdj() * y).hat() = x.hat() * y.hat() - y.hat() * x.hat() ;

what is the * operator doing in this test? Is it group composition, that is, + for Rn?

Solving this consistently is not easy:

The x.hat() is not a member of the group, but of the Lie algebra, and therefore the group composition operator does not apply to them.
On the other hand, if * is matrix multiplication, then since x.hat() is not a matrix but a vector, the test expression is inconsistent.
Finally, if we make x.hat() a matrix to recover consistency of the test then we go back to where we started.

The only way out I suppose is to have the test redefined as so:

x.smallAdj() * y = x.bracket(y)

For matrix Lie groups (and maybe for many multiplicative groups), the bracket is defined as

x.bracket(y) = x*y - y*x

But this cannot be the case for additive groups such as Rn. The Lie bracket in Rn is zero,

x.bracket(y) = 0

This is why I said that we could 'live' with x.hat() = [x 0 ; 0 0], to save the form of the current test using the multiplicative form of the lie bracket.

All thhese changes could make the point for a new issue and a new PR. But for the time being, defining x.hat() = x just breaks down the test on smallAdj.

CONCLUSION: I cannot approve this PR as it is now.

Inconsistent definition of x.hat() with test on smallAdj in group Rn

markusgft · 2020-01-13T07:10:45Z

Is there roadmap for getting this onto master, @artivis @joansola ?

artivis · 2020-01-13T13:42:51Z

@markusgft, the plan is to wait for #30 to land before merging this. This should happen sooner than later.
The reason is that, if it all goes as planned, #30 will be accompanied by a first tagged release of manif and a DOI to ease referencing. #30 has essentially been reviewed in the current state of devel and the addition of this PR would probably require a new review, further delaying #30. The process is in its final step, you can follow it here.
Sorry for the inconvenience.

minrui-hust · 2020-01-20T08:21:25Z

In RnTangent.h line 180:

Ei(i, RnTangentBase<Derived>::DoF) = 1;

Ei is a column vector, the second index RnTangentBase::DoF out of bound.
Test fail and throw an exception, on ubuntu 16.04.

artivis · 2020-01-23T13:14:45Z

In RnTangent.h line 180:
Ei(i, RnTangentBase<Derived>::DoF) = 1;
Ei is a column vector, the second index RnTangentBase::DoF out of bound.
Test fail and throw an exception, on ubuntu 16.04.

Hi @minrui-hust,
Ei is a matrix (defined here) and tests are passing both on Travis and on my machine. Could you provide some more information concerning the exception you encountered??

artivis · 2020-02-16T22:04:46Z

@joansola with #30 being merged, we can now consider merging this PR. Could you give it a last review and eventually approve it?

joansola

Did you address the x.hat() definition as we spoke? This PR is old and I lost track of all issues, but I do remember that x.hat() needed attention. I have seen a commit about this being reverted today, fc52aa3, and I am a little confused of what has been going on regarding this.

artivis · 2020-02-17T16:54:41Z

Did you address the x.hat() definition as we spoke? This PR is old and I lost track of all issues, but I do remember that x.hat() needed attention. I have seen a commit about this being reverted today, fc52aa3, and I am a little confused of what has been going on regarding this.

I did revert the change to x.hat() so that now it returns the Lie algebra of the matrix translation group T(n), that is x.hat = [ 0 x ; 0 0]. See implementation here.
We agreed that "we can live with it" for now and revisit it if necessary.

joansola

OK for me with the temporary fix of x.hat() using that of the matrix translation group.

artivis · 2020-02-20T00:35:07Z

I cleaned up the commit history hence the force-push.
Merging!

artivis added the enhancement New feature or request label Nov 14, 2019

artivis requested a review from joansola November 14, 2019 00:12

artivis self-assigned this Nov 14, 2019

joansola reviewed Nov 14, 2019

View reviewed changes

joansola previously approved these changes Nov 14, 2019

View reviewed changes

artivis mentioned this pull request Nov 18, 2019

Fix memory alignment issues. #115

Merged

joansola mentioned this pull request Jan 20, 2020

Add test for smallAdj and fix so2 se3 #114

Merged

joansola reviewed Feb 17, 2020

View reviewed changes

artivis mentioned this pull request Feb 17, 2020

Add some Map tests #127

Merged

joansola approved these changes Feb 19, 2020

View reviewed changes

artivis added 8 commits February 19, 2020 17:18

GeneratorEvaluetor::run int -> unsigned int

b891431

MANIF_*_TYPEDEF closer to abstract API

0fcf2e3

remove normalize-related tests from common_tester

2d27c8e

relax unit tests

3fe7557

add MANIF_MAKE_ALIGNED_OPERATOR_NEW_COND* macros

29785c8

add Rn

6078aeb

add Rn tests

ed7bcdb

let travis know that codecov Rn may take more than 10min

5e0608a

artivis force-pushed the feature/rn branch from 2635ca4 to 5e0608a Compare February 20, 2020 00:13

artivis merged commit ac06ddb into devel Feb 20, 2020

artivis deleted the feature/rn branch February 20, 2020 00:35

joansola mentioned this pull request Jun 3, 2023

Paper / Cheat Sheet vs Code inconsistency #268

Open

Add trivial groups Rn #109

Add trivial groups Rn #109

Conversation

artivis commented Nov 14, 2019

artivis commented Nov 14, 2019 • edited Loading

artivis commented Nov 14, 2019

codecov-io commented Nov 14, 2019 • edited Loading

Codecov Report

joansola left a comment

Choose a reason for hiding this comment

joansola Nov 14, 2019

Choose a reason for hiding this comment

artivis Nov 14, 2019

Choose a reason for hiding this comment

joansola Nov 14, 2019

Choose a reason for hiding this comment

artivis Nov 14, 2019

Choose a reason for hiding this comment

joansola commented Nov 14, 2019

joansola commented Nov 14, 2019

artivis commented Nov 14, 2019

artivis commented Nov 14, 2019

joansola commented Nov 14, 2019

joansola commented Nov 14, 2019

artivis commented Nov 14, 2019

joansola commented Nov 14, 2019 • edited Loading

artivis commented Nov 14, 2019

artivis commented Nov 14, 2019

joansola commented Nov 14, 2019 • edited Loading

artivis commented Nov 14, 2019

joansola commented Nov 14, 2019

joansola commented Nov 14, 2019

joansola commented Nov 14, 2019

artivis commented Nov 14, 2019

joansola commented Nov 14, 2019

joansola commented Nov 14, 2019 • edited Loading

joansola left a comment

Choose a reason for hiding this comment

artivis commented Nov 14, 2019

joansola commented Nov 15, 2019 • edited Loading

joansola commented Nov 15, 2019

joansola commented Nov 15, 2019

artivis commented Nov 18, 2019

joansola commented Nov 27, 2019

artivis commented Dec 2, 2019

markusgft commented Dec 3, 2019

joansola commented Dec 3, 2019 • edited Loading

markusgft commented Jan 13, 2020

artivis commented Jan 13, 2020

minrui-hust commented Jan 20, 2020

artivis commented Jan 23, 2020

artivis commented Feb 16, 2020

joansola left a comment

Choose a reason for hiding this comment

artivis commented Feb 17, 2020

joansola left a comment

Choose a reason for hiding this comment

artivis commented Feb 20, 2020

artivis commented Nov 14, 2019 •

edited

Loading

codecov-io commented Nov 14, 2019 •

edited

Loading

joansola commented Nov 14, 2019 •

edited

Loading

joansola commented Nov 14, 2019 •

edited

Loading

joansola commented Nov 14, 2019 •

edited

Loading

joansola commented Nov 15, 2019 •

edited

Loading

joansola commented Dec 3, 2019 •

edited

Loading