Add satisfiable tolerance parameter to TSR constructor #180

dqyi11 · 2017-04-18T21:57:11Z

No description provided.

jslee02 · 2017-04-19T17:08:44Z

I think we could simply pass the tolerance to TSR::isSatisfied(...). How do you think @mkoval ?

dqyi11 · 2017-04-19T17:24:20Z

@jslee02 isSatisfied() is inherited from a base class Testable.
We will still have to add similar arguments in the constructors of adapter classes unless we change isSatisfiable() of Testable.

jslee02 · 2017-04-19T17:32:54Z

We will still have to add similar arguments in the constructors of adapter classes unless we change isSatisfiable() of Testable.

👍 I thought other Testable classes also need some tolerance but it seems not.

jslee02

Could you resolve the build errors?

Also, it would be nice to have a pair of setter and getter for the tolerance.

jslee02 · 2017-04-19T17:33:31Z

include/aikido/constraint/TSR.hpp

@@ -44,7 +44,8 @@ class TSR : public Sampleable,
      const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),
      const Eigen::Matrix<double, 6, 2>& _Bw =
          Eigen::Matrix<double, 6, 2>::Zero(),
-      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity());
+      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity(),
+      const double& _satisfiableTolerance = 1e-6);


Nit: Passing const reference for primitive type is pointless. Please change to double.

jslee02 · 2017-04-19T17:33:47Z

include/aikido/constraint/TSR.hpp

@@ -57,7 +58,8 @@ class TSR : public Sampleable,
  TSR(const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),
      const Eigen::Matrix<double, 6, 2>& _Bw =
          Eigen::Matrix<double, 6, 2>::Zero(),
-      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity());
+      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity(),
+      const double& _satisfiableTolerance = 1e-6);


jslee02 · 2017-04-19T17:34:09Z

include/aikido/constraint/TSR.hpp

 private:
  std::unique_ptr<util::RNG> mRng;
  std::shared_ptr<statespace::SE3> mStateSpace;
+


Nit: Could we remove this empty line?

jslee02 · 2017-04-19T17:38:02Z

src/constraint/TSR.cpp

    : mT0_w(_T0_w)
    , mBw(_Bw)
    , mTw_e(_Tw_e)
    , mRng(std::move(_rng))
    , mStateSpace(std::make_shared<SE3>())
+    , mSatisfiableTolerance(_satisfiableTolerance)


Initialize mSatisfiableTolerance before mRng and mStateSpace to resolve the warning.

jslee02 · 2017-04-19T17:38:20Z

src/constraint/TSR.cpp

    : mT0_w(_T0_w)
    , mBw(_Bw)
    , mTw_e(_Tw_e)
    , mRng(std::unique_ptr<util::RNG>(
          new util::RNGWrapper<std::default_random_engine>(0)))
    , mStateSpace(std::make_shared<SE3>())
+    , mSatisfiableTolerance(_satisfiableTolerance)


jslee02 · 2017-04-19T17:38:32Z

src/constraint/TSR.cpp

@@ -109,6 +112,7 @@ TSR::TSR(const TSR& other)
    , mTw_e(other.mTw_e)
    , mRng(std::move(other.mRng->clone()))
    , mStateSpace(std::make_shared<SE3>())
+    , mSatisfiableTolerance(other.mSatisfiableTolerance)


jslee02 · 2017-04-19T17:38:38Z

src/constraint/TSR.cpp

@@ -120,6 +124,7 @@ TSR::TSR(TSR&& other)
    , mTw_e(other.mTw_e)
    , mRng(std::move(other.mRng))
    , mStateSpace(std::make_shared<SE3>())
+    , mSatisfiableTolerance(other.mSatisfiableTolerance)


dqyi11 · 2017-04-19T18:09:02Z

@jslee02 I addressed all your comments. Why does the order matter?
Also I put it as a public variable, because I see other member variables are public.
Shall I move it to private/protected and write get/set?

coveralls · 2017-04-19T18:34:44Z

Coverage decreased (-0.06%) to 80.311% when pulling 03082ad on feature/add_eps_for_TSR into 2af5c55 on master.

coveralls · 2017-04-19T21:23:11Z

Coverage decreased (-0.2%) to 80.213% when pulling a77b734 on feature/add_eps_for_TSR into 2af5c55 on master.

jslee02

Looks good to me! A few minor style updates and docstring requested.

jslee02 · 2017-04-20T02:02:38Z

include/aikido/constraint/TSR.hpp

@@ -44,7 +44,8 @@ class TSR : public Sampleable,
      const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),
      const Eigen::Matrix<double, 6, 2>& _Bw =
          Eigen::Matrix<double, 6, 2>::Zero(),
-      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity());
+      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity(),
+      const double _satisfiableTolerance = 1e-6);


Nit: const is meaningless for primitive types please change this to double _satisfiableTolerance = 1e-6

Need docstring for the new variable.

jslee02 · 2017-04-20T02:02:46Z

include/aikido/constraint/TSR.hpp

@@ -57,7 +58,8 @@ class TSR : public Sampleable,
  TSR(const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),
      const Eigen::Matrix<double, 6, 2>& _Bw =
          Eigen::Matrix<double, 6, 2>::Zero(),
-      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity());
+      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity(),
+      const double _satisfiableTolerance = 1e-6);


jslee02 · 2017-04-20T02:03:02Z

include/aikido/constraint/TSR.hpp

@@ -112,6 +114,10 @@ class TSR : public Sampleable,
  bool project(const statespace::StateSpace::State* _s,
      statespace::StateSpace::State* _out) const override;

+  double getSatisfiableTolerance();
+
+  void setSatisfiableTolerance(const double satisfiableTolerance);


Nit: missing docstring

jslee02 · 2017-04-20T02:03:05Z

include/aikido/constraint/TSR.hpp

@@ -112,6 +114,10 @@ class TSR : public Sampleable,
  bool project(const statespace::StateSpace::State* _s,
      statespace::StateSpace::State* _out) const override;

+  double getSatisfiableTolerance();


Nit: missing docstring

jslee02 · 2017-04-20T02:03:22Z

src/constraint/TSR.cpp

@@ -79,10 +79,12 @@ class TSRSampleGenerator : public SampleGenerator

 //=============================================================================
 TSR::TSR(std::unique_ptr<util::RNG> _rng, const Eigen::Isometry3d& _T0_w,
-         const Eigen::Matrix<double, 6, 2>& _Bw, const Eigen::Isometry3d& _Tw_e)
+         const Eigen::Matrix<double, 6, 2>& _Bw, const Eigen::Isometry3d& _Tw_e,
+         const double _satisfiableTolerance)


jslee02 · 2017-04-20T02:03:31Z

src/constraint/TSR.cpp

@@ -91,10 +93,11 @@ TSR::TSR(std::unique_ptr<util::RNG> _rng, const Eigen::Isometry3d& _T0_w,

 //=============================================================================
 TSR::TSR(const Eigen::Isometry3d& _T0_w, const Eigen::Matrix<double, 6, 2>& _Bw,
-         const Eigen::Isometry3d& _Tw_e)
+         const Eigen::Isometry3d& _Tw_e, const double _satisfiableTolerance)


coveralls · 2017-04-20T16:38:20Z

Coverage decreased (-0.1%) to 80.238% when pulling baa8310 on feature/add_eps_for_TSR into 2af5c55 on master.

gilwoolee

The name mSatisfiableTolerance should be changed to mTestableTolerance, as this tolerance is being used for methods inherited from the Testable class.

While this PR is good in that it makes the arbitrary eps in the existing isSatisfied method into a member variable, I'm not sure that this is the right metric. I believe the previous use of eps was just there as a placeholder. In Vector6d representation of TSR, the first three are translation, whole the last three are for rotation. If we're to do a fix, shouldn't mTestableTolerance be a Vector6d with some default value? @mkoval @jslee02 any thoughts?

Also, I am not sure whether set and get for the tolerance is necessary. Is there a strong need to change this once a TSR is created?

gilwoolee · 2017-04-20T18:49:13Z

include/aikido/constraint/TSR.hpp

@@ -44,7 +44,8 @@ class TSR : public Sampleable,
      const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),
      const Eigen::Matrix<double, 6, 2>& _Bw =
          Eigen::Matrix<double, 6, 2>::Zero(),
-      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity());
+      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity(),
+      const double _satisfiableTolerance = 1e-6);


Need docstring for the new variable.

gilwoolee · 2017-04-20T18:50:34Z

include/aikido/constraint/TSR.hpp

@@ -112,6 +114,14 @@ class TSR : public Sampleable,
  bool project(const statespace::StateSpace::State* _s,
      statespace::StateSpace::State* _out) const override;

+  /// Get the staisfiable tolerance for isSatisfiable.
+  /// \param[out] _out Satisfialble tolerance, double.
+  double getSatisfiableTolerance();


Change this to getTestableTolerance(). Although, I wonder why these set and get methods are necessary. Why do we want to change the tolerance once a TSR is created?

gilwoolee · 2017-04-20T18:50:53Z

include/aikido/constraint/TSR.hpp

+  double getSatisfiableTolerance();
+
+  /// Set the staisfiable tolerance for isSatisfiable.
+  /// \param _satisfiableTolerance Satisfiable tolerance to set.


Typo. Though not sure why we need this method.

gilwoolee · 2017-04-20T18:51:08Z

include/aikido/constraint/TSR.hpp

 private:
+  /// Tolerance for checking satisfiability
+  double mSatisfiableTolerance;


Consider changing this to Eigen::Vector6d

Yes, I also think Eigen::Vector6d makes more sense.

dqyi11 · 2017-04-20T21:58:33Z

I have addressed @gilwoolee 's comments.
I have not changed it to Vector6d, because the current implementation takes the norm of the distance vector to compare with a double threshold. I will make the change if comparing two vectors is expected.
Set() and Get() are still kept. Will keep or remove after hearing back from @mkoval and @jslee02 .

coveralls · 2017-04-20T22:22:17Z

Coverage decreased (-0.2%) to 80.213% when pulling aa443dd on feature/add_eps_for_TSR into 2af5c55 on master.

mkoval

I am conflicted about whether the tolerance parameter should be a double or a Vector6d. On one hand, @gilwoolee raises a valid point that the elements have different units that may have dissimilar scales. On the other hand, this tolerance parameter should only be used to deal with numerical precision issues that most often occur with point TSRs: the actual tolerance is encoded in Bw.

I slightly prefer the Vector6d option, but I don't feel strongly either way. Maybe @siddhss5 can break the tie?

mkoval · 2017-04-21T00:58:35Z

include/aikido/constraint/TSR.hpp

@@ -44,7 +44,8 @@ class TSR : public Sampleable,
      const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),


Add a docstring for _testableTolerance .

mkoval · 2017-04-21T00:58:55Z

include/aikido/constraint/TSR.hpp

@@ -44,7 +44,8 @@ class TSR : public Sampleable,
      const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),
      const Eigen::Matrix<double, 6, 2>& _Bw =
          Eigen::Matrix<double, 6, 2>::Zero(),
-      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity());
+      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity(),
+      const double _testableTolerance = 1e-6);


Don't mark arguments that are passed by value as const in function declarations.

mkoval · 2017-04-21T00:59:16Z

include/aikido/constraint/TSR.hpp

@@ -57,7 +58,8 @@ class TSR : public Sampleable,
  TSR(const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),
      const Eigen::Matrix<double, 6, 2>& _Bw =
          Eigen::Matrix<double, 6, 2>::Zero(),
-      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity());
+      const Eigen::Isometry3d& _Tw_e = Eigen::Isometry3d::Identity(),
+      const double _testableTolerance = 1e-6);


Don't mark arguments that are passed by value as const in function declarations.

mkoval · 2017-04-21T00:59:26Z

include/aikido/constraint/TSR.hpp

@@ -57,7 +58,8 @@ class TSR : public Sampleable,
  TSR(const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),


Add a docstring for _testableTolerance .

mkoval · 2017-04-21T00:59:39Z

include/aikido/constraint/TSR.hpp

+
+  /// Set the testable tolerance used in isSatisfiable.
+  /// \param _testableTolerance Testable tolerance to set.
+  void setTestableTolerance(const double _testableTolerance);


Don't mark arguments that are passed by value as const in function declarations.

jslee02 · 2017-04-21T15:06:14Z

As a third option, we could provide either way by providing class TSR::ErrorMethod that has a virtual function that measures the error and concrete classes that implement the error measurements. Then use a reasonable one as the default.

coveralls · 2017-04-21T21:01:18Z

Coverage decreased (-0.1%) to 80.238% when pulling ed566bf on feature/add_eps_for_TSR into 2af5c55 on master.

coveralls · 2017-04-22T19:29:01Z

Coverage decreased (-0.2%) to 79.006% when pulling 0e41653 on feature/add_eps_for_TSR into 5d19317 on master.

gilwoolee · 2017-04-22T20:45:17Z

Based on what @mkoval said, double tolerance seems to be fine since it's only about numerical precision. @jslee02 's suggestion seems reasonable, but it may be an overcomplication if this is only for numerical precision.

I'm still not convinced that we need setTolerance and getTolerance. Why do we want to change tolerance after initializing it? @mkoval @jslee02 thoughts?

coveralls · 2017-04-22T21:50:50Z

Coverage decreased (-0.1%) to 79.044% when pulling d118d2d on feature/add_eps_for_TSR into 78d38af on master.

jslee02 · 2017-04-22T22:14:41Z

Why do we want to change tolerance after initializing it? @mkoval @jslee02 thoughts?

I don't have a compelling reason to have the getters/setters but also not to have. One use case I can think of now is doing planning changing the error tolerance when it's failed to see if the failure is because of it. I'm fine either leaving them or not.

add satisfiable tolerance parameter to TSR constructor

5aada2a

dqyi11 requested a review from mkoval April 18, 2017 21:57

jslee02 suggested changes Apr 19, 2017

View reviewed changes

address JS's comments

03082ad

address JS's comment

a77b734

jslee02 approved these changes Apr 20, 2017

View reviewed changes

address JS's comment

baa8310

gilwoolee suggested changes Apr 20, 2017

View reviewed changes

address Gilwoo's comments

aa443dd

mkoval requested changes Apr 21, 2017

View reviewed changes

address Mike's comments

ed566bf

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

Merge branch 'master' into feature/add_eps_for_TSR

0e41653

Merge branch 'master' into feature/add_eps_for_TSR

d118d2d

mkoval approved these changes Apr 22, 2017

View reviewed changes

gilwoolee merged commit 4af95ae into master Apr 22, 2017

gilwoolee deleted the feature/add_eps_for_TSR branch April 22, 2017 22:25

		@@ -44,7 +44,8 @@ class TSR : public Sampleable,
		const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),

		@@ -57,7 +58,8 @@ class TSR : public Sampleable,
		TSR(const Eigen::Isometry3d& _T0_w = Eigen::Isometry3d::Identity(),

Add satisfiable tolerance parameter to TSR constructor #180

Add satisfiable tolerance parameter to TSR constructor #180

Conversation

dqyi11 commented Apr 18, 2017

jslee02 commented Apr 19, 2017

dqyi11 commented Apr 19, 2017

jslee02 commented Apr 19, 2017

jslee02 left a 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

dqyi11 commented Apr 19, 2017

coveralls commented Apr 19, 2017

coveralls commented Apr 19, 2017

jslee02 left a 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 20, 2017

gilwoolee left a 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

dqyi11 commented Apr 20, 2017

coveralls commented Apr 20, 2017

mkoval left a 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

jslee02 commented Apr 21, 2017

coveralls commented Apr 21, 2017

coveralls commented Apr 22, 2017

gilwoolee commented Apr 22, 2017

coveralls commented Apr 22, 2017

jslee02 commented Apr 22, 2017