Template Rn for dimension to exploit performance of fixed-size Eigen objects #159

jslee02 · 2017-04-06T01:57:50Z

TODOs:

Explicitly instantiates for frequently used dimensions
Use extern template to force the compiler not to instantiate for the explicitly instantiated types
Decide name: template <int N> class Rn vs template <int N> class R (I prefer R<N>)
Support dynamic size

Resolves #152

…object

coveralls · 2017-04-06T03:26:58Z

Changes Unknown when pulling f06e949 on feature/template_rn_state_space into ** on master**.

coveralls · 2017-04-06T03:34:58Z

Changes Unknown when pulling f06e949 on feature/template_rn_state_space into ** on master**.

…_state_space

coveralls · 2017-04-07T13:02:54Z

Coverage decreased (-0.8%) to 84.437% when pulling 4be2bb1 on feature/template_rn_state_space into a9b50ae on master.

psigen · 2017-04-07T14:00:07Z

I prefer the R<N> syntax. I think it makes semantic sense and then instantiations can look like R2 or R3.

…_state_space Conflicts: src/constraint/uniform/RnBoxConstraint.cpp src/constraint/uniform/RnConstantSampler.cpp src/statespace/Rn.cpp tests/constraint/PolynomialConstraint.cpp tests/trajectory/test_Interpolated.cpp

coveralls · 2017-04-11T03:04:28Z

Coverage decreased (-0.8%) to 84.403% when pulling 277698b on feature/template_rn_state_space into a59975f on master.

coveralls · 2017-04-11T03:29:14Z

Coverage decreased (-0.8%) to 84.37% when pulling f1ffd63 on feature/template_rn_state_space into a59975f on master.

coveralls · 2017-04-11T19:53:26Z

Coverage decreased (-0.8%) to 84.403% when pulling 82a9c6e on feature/template_rn_state_space into a59975f on master.

coveralls · 2017-04-12T01:36:27Z

Coverage decreased (-0.7%) to 84.487% when pulling ef80605 on feature/template_rn_state_space into a59975f on master.

coveralls · 2017-04-12T01:55:19Z

Coverage decreased (-0.8%) to 84.403% when pulling 4810b2a on feature/template_rn_state_space into a59975f on master.

coveralls · 2017-04-12T02:08:13Z

Coverage decreased (-0.8%) to 84.437% when pulling 4810b2a on feature/template_rn_state_space into a59975f on master.

mkoval

A few minor comments, but generally looks good.

mkoval · 2017-04-12T02:02:11Z

include/aikido/constraint/detail/JointStateSpaceHelpers-impl.hpp

+  statespace::dart::R1Joint,
+  statespace::dart::R2Joint,
+  statespace::dart::R3Joint,
+  statespace::dart::R6Joint,


Could we add R<Eigen::Dynamic> here now? 😁

All the RnX classes support Eigen::Dynamic, but not for RnJoint. This is because all the dimensions of joints are determinant in compile time. More importantly, dynamic size dimension is not supported by none of DART's joint classes.

mkoval · 2017-04-12T02:03:35Z

include/aikido/constraint/uniform/RnBoxConstraint.hpp

+using R2BoxConstraint = RnBoxConstraint<2>;
+using R3BoxConstraint = RnBoxConstraint<3>;
+using R6BoxConstraint = RnBoxConstraint<6>;
+using RxBoxConstraint = RnBoxConstraint<Eigen::Dynamic>;


Nit: Rename to RnBoxConstraint?

mkoval · 2017-04-12T02:03:58Z

include/aikido/constraint/uniform/RnConstantSampler.hpp

+using R2ConstantSampler = RnConstantSampler<2>;
+using R3ConstantSampler = RnConstantSampler<3>;
+using R6ConstantSampler = RnConstantSampler<6>;
+using RxConstantSampler = RnConstantSampler<Eigen::Dynamic>;


Nit: Rename to RnConstantSampler?

Which means, we also rename the template class RnConstantSampler to RConstantSampler, right?

Yes, I think that makes sense if we renamed Rn to R.

mkoval · 2017-04-12T02:05:09Z

include/aikido/constraint/uniform/detail/RnBoxConstraint-impl.hpp

+bool RnBoxConstraintSampleGenerator<N>::sample(
+  statespace::StateSpace::State* _state)
+{
+  Eigen::VectorXd value(mDistributions.size());


I think this can be Eigen::Vector<double, N, 1>.

mkoval · 2017-04-12T02:06:14Z

include/aikido/distance/RnEuclidean.hpp

+using R2Euclidean = RnEuclidean<2>;
+using R3Euclidean = RnEuclidean<3>;
+using R6Euclidean = RnEuclidean<6>;
+using RxEuclidean = RnEuclidean<Eigen::Dynamic>;


Nit: Rename to RnEuclidean?

mkoval · 2017-04-12T02:07:12Z

include/aikido/statespace/Rn.hpp

+  /// state space becomes a dynamic size real vector space; if N is a
+  /// non-negative, then this state space becomes a \c N dimensional fixed size
+  /// real vector space.
+  R();


This should throw an exception if N is Eigen::Dynamic, because there is no way to resize it.

mkoval · 2017-04-12T02:08:38Z

include/aikido/statespace/detail/Rn-impl.hpp

+template <int N>
+R<N>::R()
+{
+  static_assert(N > -2,


N >= 0 || N == Eigen::Dynamic would be cleaner, since it doesn't assume Eigen::Dynamic = -1.

Can we add a static_assert(N != Eigen::Dynamic) to avoid the edge case below?

N >= 0 || N == Eigen::Dynamic would be cleaner, since it doesn't assume Eigen::Dynamic = -1.

👍

Can we add a static_assert(N != Eigen::Dynamic) to avoid the edge case below?

I instead changed to throw exceptions for that case because that wouldn't be compiled whenever N == Eigen::Dynamic.

coveralls · 2017-04-12T02:31:12Z

Coverage decreased (-0.8%) to 84.403% when pulling 4e8eac0 on feature/template_rn_state_space into a59975f on master.

psigen · 2017-04-12T03:28:56Z

Wait a second, doesn't it make sense to leave the template as Rn? I read it as `Rn<where n=1>`

jslee02 · 2017-04-12T03:35:55Z

Rn is reserved for R<Eigen::Dynamic> so that it would be the same as before.

So we now have:

Rn = R<Eigen::Dynamic>
R0 = R<0>
R1 = R<1>
...

I believe above is better looking than below:

Rnn = Rn<Eigen::Dynamic> // what?!
Rn0 = Rn<0>
Rn1 = Rn<1>
...

psigen · 2017-04-12T04:04:18Z

I agree with `R<n>` but it looks much weirder with `RConstantSampler<2>` where it is unclear what word the template parameter is associated with.

coveralls · 2017-04-12T04:15:23Z

Coverage decreased (-0.8%) to 84.421% when pulling 0ff6170 on feature/template_rn_state_space into 3c9c8cc on master.

coveralls · 2017-04-12T04:17:50Z

Coverage decreased (-0.8%) to 84.421% when pulling 0ff6170 on feature/template_rn_state_space into 3c9c8cc on master.

jslee02 · 2017-04-12T13:58:16Z

I wish C++ supports this syntax R<n>ConstantSampler (jk, that would be disaster 😨).

I believe mostly we might be using type aliasing for the templates like R2ConstantSampler so it would be fine with either template names. However, my concern with putting n to template names is using Rnn = Rn<Eigen::Dynamic>.

Anyways, I don't have a strong preference here as long as it makes sense. 😄

mkoval · 2017-04-13T01:20:41Z

I slightly prefer R for the reason @jslee02 described - the aliases Rnn, RXd and Rxd are all very awkward for the Eigen::Dynamic alias. I could be convinced otherwise if @psigen is sufficiently stubborn.

psigen · 2017-04-13T01:39:07Z

Guys, I am not opposed to R<> for the StateSpace, I am concerned about it for types like RBoxConstraint where it seems like:

using R2BoxConstraint = RBoxConstraint<2>

is not clearer than:

using R2BoxConstraint = RnBoxConstraint<2>

because the template parameter is nowhere near the thing it is attached to, and it is less obvious what the R means on its own.

One option could be to always just put the state space at the end like BoxConstraintR<2>.

Another option is to use a different placeholder, like:

using R2BoxConstraint = RxBoxConstraint<2>
using RnBoxConstraint = RxBoxConstraint<Eigen::Dynamic>

jslee02 · 2017-04-13T01:50:56Z

I thought we all implicitly assume that we want to keep the consistency of R[n] and R[n]X classes.

The alternatives could be options, but RnBoxConstraint<N> is not consistent with R<N> and BoxConstraintR<N> is not consistent with the current naming convention (at least until now) of [SpaceName]OtherName.

I think we should choose which one we weight more between consistency and readability. 😓

mkoval · 2017-04-13T03:37:17Z

I thought we all implicitly assume that we want to keep the consistency of R[n] and R[n]X classes.

I strongly agree. All of the other constraint classes are named like <StateSpace type><optional descriptor><constraint type>. Breaking this convention just for R<n> will cause me (and I suspect many others) endless frustration.

If it comes down to R<n> vs. consistency, I'd rather go with Rn<n> with consistent names.

psigen · 2017-04-13T12:56:44Z

I guess I'll just leave it up to you guys, but note that for strong consistency, the naming conventions end up being:

RJoint<N>  -> R2Joint
SOJoint<N> -> SO2Joint
SEJoint<N> -> SE3Joint
TJoint<N>  -> T2Joint

And if you are in a situation where the constraint takes some template parameter

RSomeConstraint<N, M> -> R3SomeMConstraint

If you think those are acceptable, then by all means go for it. Personally I think this leads to making it hard to tell how the template parameter is going to affect the substituted name, given that it inserts a number in the middle of the original name in this convention. But I also don't really have a problem with inconsistent naming between the state spaces and templates that use them, e.g.

R<N> -> R2
RnBoxConstraint<N> -> R2BoxConstraint

Anyway, it's just a style opinion, so you guys should do whatever you conclude is best.

jslee02 · 2017-04-13T13:47:46Z

I think RSomeConstraint<N, M> -> RNSomeMConstraint is acceptable.

Personally I think this leads to making it hard to tell how the template parameter is going to affect the substituted name, given that it inserts a number in the middle of the original name in this convention.

I agree with you, but I also don't know what would be the better alternative under the current convention. I think using different placeholder could be an option as you commented above unless we will support resizable statespace (in this case, we might prefer X for the aliasing name rather than the placeholder name), which I believe so, as:

using R2 = Rx<2>
using Rn = Rx<Eigen::Dynamic>

using R2BoxConstraint = RxBoxConstraint<2>
using RnBoxConstraint = RxBoxConstraint<Eigen::Dynamic>

using R2BoxSome3Constraint = RxBoxSomeMConstraint<2, 3>
using RnBoxSome3Constraint = RxBoxSomeMConstraint<Eigen::Dynamic, 3>

However, we might have additional style issue if we start to put placeholder to template class name: upper letter vs. lower letter.

Anyway, it's just a style opinion, so you guys should do whatever you conclude is best.

I agree. I would like to vote to not using placeholder in template class name. 😓

jslee02 · 2017-04-13T18:14:18Z

Alright, let's merge this as it is since at least we could use it as before (Rn[X] classes are the same), and revisit if necessary.

Template Rn for dimension to exploit performance of fixed-size Eigen …

5c96dcb

…object

jslee02 requested a review from mkoval April 6, 2017 01:57

jslee02 changed the title ~~Template Rn for dimension to exploit performance of fixed-size Eigen objects~~ [WIP] Template Rn for dimension to exploit performance of fixed-size Eigen objects Apr 6, 2017

jslee02 and others added 2 commits April 5, 2017 23:08

Fix build errors due to incomplete previous refactor

19ea91e

Merge branch 'master' into feature/template_rn_state_space

f06e949

jslee02 added 2 commits April 7, 2017 08:41

Merge remote-tracking branch 'origin/master' into feature/template_rn…

e65841d

…_state_space

Print log after CI failed

4be2bb1

Fix path to build logs

f1ffd63

Remove unreachable code from RnConstantSampler

82a9c6e

Explicitly instantiates for frequently used dimensions

3d19d31

jslee02 changed the title ~~[WIP] Template Rn for dimension to exploit performance of fixed-size Eigen objects~~ Template Rn for dimension to exploit performance of fixed-size Eigen objects Apr 11, 2017

jslee02 added 3 commits April 11, 2017 20:58

Add dynamic size support, Rx (i.e., R<Eigen::Dynamic>)

1b63cc7

Remove debug code

6cc3c68

Add dynamic size support for other Rn related classes

ef80605

Add RxBoxConstraint tests

4810b2a

Update comments for Rn constructors

4e8eac0

mkoval approved these changes Apr 12, 2017

View reviewed changes

Address Mike's comments

82dd8d8

Merge branch 'master' into feature/template_rn_state_space

b2f25ec

jslee02 added 3 commits April 11, 2017 23:39

Minor comment update

ac12cca

Fix Conversions.cpp

f78a108

Update minor comment

0ff6170

jslee02 added this to the Aikido 0.1.0 milestone Apr 12, 2017

jslee02 merged commit 2b21ddd into master Apr 13, 2017

jslee02 deleted the feature/template_rn_state_space branch April 15, 2017 00:33

Template Rn for dimension to exploit performance of fixed-size Eigen objects #159

Template Rn for dimension to exploit performance of fixed-size Eigen objects #159

Conversation

jslee02 commented Apr 6, 2017 • edited Loading

coveralls commented Apr 6, 2017

coveralls commented Apr 6, 2017

coveralls commented Apr 7, 2017

psigen commented Apr 7, 2017

coveralls commented Apr 11, 2017

coveralls commented Apr 11, 2017

coveralls commented Apr 11, 2017

coveralls commented Apr 12, 2017

coveralls commented Apr 12, 2017

coveralls commented Apr 12, 2017

mkoval left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 12, 2017

psigen commented Apr 12, 2017 via email

jslee02 commented Apr 12, 2017 • edited Loading

psigen commented Apr 12, 2017 via email

coveralls commented Apr 12, 2017

coveralls commented Apr 12, 2017

jslee02 commented Apr 12, 2017 • edited Loading

mkoval commented Apr 13, 2017

psigen commented Apr 13, 2017 • edited Loading

jslee02 commented Apr 13, 2017 • edited Loading

mkoval commented Apr 13, 2017

psigen commented Apr 13, 2017 • edited Loading

jslee02 commented Apr 13, 2017 • edited Loading

jslee02 commented Apr 13, 2017

jslee02 commented Apr 6, 2017 •

edited

Loading

mkoval left a comment •

edited

Loading

jslee02 commented Apr 12, 2017 •

edited

Loading

jslee02 commented Apr 12, 2017 •

edited

Loading

psigen commented Apr 13, 2017 •

edited

Loading

jslee02 commented Apr 13, 2017 •

edited

Loading

psigen commented Apr 13, 2017 •

edited

Loading

jslee02 commented Apr 13, 2017 •

edited

Loading