Create a Sequential Sampleable #393

aditya-vk · 2018-05-05T00:01:05Z

This PR adds a Sequential Sampleable. It samples in the order given, moving to the next one upon exhaustion of the current sampleable. This is useful, for instance, in solving IK, in sampling the current configuration first before other random/informed sampling.

Before creating a pull request

Document new methods and classes
Format code with make format

Before merging a pull request

Set version target by selecting a milestone on the right side
Summarize this change in CHANGELOG.md
Add unit test(s) for this change

codecov · 2018-05-05T08:35:27Z

Codecov Report

Merging #393 into master will decrease coverage by 0.01%.
The diff coverage is 78.26%.

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
- Coverage   80.96%   80.94%   -0.02%     
==========================================
  Files         201      203       +2     
  Lines        5705     5774      +69     
==========================================
+ Hits         4619     4674      +55     
- Misses       1086     1100      +14

Impacted Files	Coverage Δ
include/aikido/constraint/SequentialSampleable.hpp	`100% <100%> (ø)`
src/constraint/CartesianProductSampleable.cpp	`78.33% <40%> (-3.49%)`	⬇️
src/constraint/SequentialSampleable.cpp	`80.95% <80.95%> (ø)`
src/constraint/FiniteSampleable.cpp	`86.88% <0%> (+1.63%)`	⬆️

jslee02 · 2018-05-05T00:05:15Z

include/aikido/constraint/SequentialSampleable.hpp

+#ifndef AIKIDO_CONSTRAINT_SEQUENTIALSAMPLEABLE_HPP_
+#define AIKIDO_CONSTRAINT_SEQUENTIALSAMPLEABLE_HPP_
+
+#include "Sampleable.hpp"


Nit: Use full path (e.g., "aikido/constraint/Sampleable.hpp")

jslee02 · 2018-05-05T00:13:34Z

include/aikido/constraint/SequentialSampleable.hpp

+
+private:
+  statespace::StateSpacePtr mStateSpace;
+  std::vector<SampleablePtr> mSampleables;


Please add docstring

jslee02 · 2018-05-05T00:13:54Z

src/constraint/SequentialSampleable.cpp

@@ -0,0 +1,117 @@
+#include <aikido/constraint/SequentialSampleable.hpp>


Nit: #include "aikido/constraint/SequentialSampleable.hpp"

jslee02 · 2018-05-05T00:14:56Z

src/constraint/SequentialSampleable.cpp

+namespace constraint {
+
+// TODO (avk): Should I declare the class first and then define?
+// What is "good" C++ convention? Don't tell me it's a preference JS! -.-


I don't think that's necessary when defining in a source file.

In that case, shall we add docstring during definitions, which we generally do not do right?

shall we add docstring during definitions

I think so. Although they are not exposed to the users, it would be very helpful for us (i.e., developers).

jslee02 · 2018-05-05T00:16:30Z

include/aikido/constraint/SequentialSampleable.hpp

+  /// \param[in] sampleables Set of sampleables.
+  SequentialSampleable(
+      statespace::StateSpacePtr stateSpace,
+      std::vector<SampleablePtr> sampleables);


Pass sampleables by const reference to avoid the unnecessary copy. Looking at the implementation, you used std::move(...). (a) Passing by const reference and (b) passing by value + std::move() are technically the same, but we only use (b) for smart pointers to indicate the ownership is being shared or transferred.

jslee02 · 2018-05-06T01:28:29Z

src/constraint/SequentialSampleable.cpp

+  {
+    // Do nothing
+    // TODO (avk): Should we again check equivalence of statespaces between
+    // that of each generator and mStateSpace? I don't think it's necessary.


It seems this class is vulnerable in the case of empty generators.

getNumSamples() returns 0 after checking for empty vector.

jslee02 · 2018-05-06T01:34:37Z

src/constraint/SequentialSampleable.cpp

+
+  int getNumSamples() const override
+  {
+    return mGenerators[mIndex]->getNumSamples();


I'm not sure if this logic is following the description of getNumSamples():

Gets an upper bound on the number of samples remaining or NO_LIMIT.

jslee02 · 2018-05-06T01:38:16Z

src/constraint/SequentialSampleable.cpp

+    if (mIndex >= mGenerators.size())
+      return false;
+
+    return true;


I think this function should return true when:

the number of remaining generators is not zero and

one of the remaining generators is able to sample.

It seems the current implementation is missing 2.

The current implementation with the iterating through generators until sampling is possible, (2) is implicitly taken care of

It sounds like canSample() works correctly only when it's used in sample(), which I don't think that's a good idea. Also, this doesn't agree with the docstrion of canSample():

/// Returns whether getNumSamples() > 0.

I propose changing this function to return getNumSamples() > 0; and changing canSample() not to call this function and to check the size of mGenerators directly in canSample().

Would it be possible for one of the remaining generators to not be sampleable? Then even though you have generators remaining (this implementation would return true), it still seems like you wouldn't be sample (logically, maybe it should return false). What if we just do something like

for (std::size_t i = mIndex; i < mGenerators.size(); ++i) if (mGenerators[i].canSample()) return true; return false;

@brianhou 's version would be more efficient because returns getNumSamples() > 0 can be more expensive as it computes the exact number of samples while @brianhou 's version can early terminate.

jslee02 · 2018-05-06T01:38:45Z

src/constraint/SequentialSampleable.cpp

+private:
+  statespace::StateSpacePtr mStateSpace;
+  std::vector<std::unique_ptr<SampleGenerator>> mGenerators;
+  std::size_t mIndex;


Please add docstrings.

jslee02 · 2018-05-06T01:42:00Z

src/constraint/SequentialSampleable.cpp

+  bool sample(statespace::StateSpace::State* state)
+  {
+    if (!mGenerators[mIndex]->canSample())
+      mIndex++;


I think this function should keep iterating until it finds a sampleable generator instead of just check only one generator.

jslee02 · 2018-05-06T17:07:37Z

include/aikido/constraint/SequentialSampleable.hpp

+  /// \param[in] sampleables Set of sampleables.
+  SequentialSampleable(
+      statespace::StateSpacePtr stateSpace,
+      std::vector<SampleablePtr>& sampleables);


I meant passing by const reference rather than just reference. 😄

jslee02 · 2018-05-06T17:08:11Z

include/aikido/constraint/SequentialSampleable.hpp

+private:
+  /// StateSpace in which the constraint operates.
+  statespace::StateSpacePtr mStateSpace;
+  /// Set of sampleables.


Nit: Add empty line between members.

jslee02 · 2018-05-06T17:10:02Z

src/constraint/SequentialSampleable.cpp

+public:
+  SequentialSampleGenerator(
+      statespace::StateSpacePtr stateSpace,
+      std::vector<std::unique_ptr<SampleGenerator>>& generators)


I think we should pass by value for generators (and move) because it's transferring the onwerships.

jslee02 · 2018-05-06T17:12:53Z

src/constraint/SequentialSampleable.cpp

+    , mIndex(0)
+  {
+    for (std::size_t i = 0; i < mGenerators.size(); ++i)
+      assert(mGenerators[i]->getStateSpace() == mStateSpace);


Please note that assert is ignored in release mode. This code will not be built in release mode since the for-statement is empty.

We could either:

#ifndef NDEBUG // debug mode for (const auto& generator : mGenerators) assert(generator->getStateSpace() == mStateSpace); #endif

or

for (const auto& generator : mGenerators) { assert(generator->getStateSpace() == mStateSpace); }

I slightly prefer former version. 😄

jslee02 · 2018-05-06T17:21:21Z

src/constraint/SequentialSampleable.cpp

+    if (mIndex >= mGenerators.size())
+      return false;
+
+    return true;


It sounds like canSample() works correctly only when it's used in sample(), which I don't think that's a good idea. Also, this doesn't agree with the docstrion of canSample():

/// Returns whether getNumSamples() > 0.

I propose changing this function to return getNumSamples() > 0; and changing canSample() not to call this function and to check the size of mGenerators directly in canSample().

jslee02 · 2018-05-06T17:34:55Z

src/constraint/SequentialSampleable.cpp

+
+      mIndex++;
+    }
+    return false;


This logic (i.e., "try the current generator to sample and return the result") is possible to return false even when one of the remaining generators can sample, which I don't think that's the expected behavior of sample.

I propose changing this logic to something like:

index2 = index for generator in generators[index2:]: result = generator.sample(state); index += 1 if result: return True return False

so that it tries to sample until it can sample and returns false when all the remaining generators are not able to draw a sample.

Ah. Got it.
Your proposal will account for sample() may transiently fail, i.e. return false, at any point before then

brianhou · 2018-05-06T17:38:40Z

src/constraint/SequentialSampleable.cpp

+    if (mIndex >= mGenerators.size())
+      return false;
+
+    return true;


Would it be possible for one of the remaining generators to not be sampleable? Then even though you have generators remaining (this implementation would return true), it still seems like you wouldn't be sample (logically, maybe it should return false). What if we just do something like

for (std::size_t i = mIndex; i < mGenerators.size(); ++i) if (mGenerators[i].canSample()) return true; return false;

brianhou · 2018-05-06T17:39:54Z

src/constraint/SequentialSampleable.cpp

+  // Documentation inherited
+  int getNumSamples() const override
+  {
+    if (mGenerators.empty())


I think this might be unnecessary. If this happens, the for loop wouldn't execute and numSamples would be 0 anyway.

brianhou · 2018-05-06T18:20:11Z

src/constraint/SequentialSampleable.cpp

+    , mGenerators(std::move(generators))
+    , mIndex(0)
+  {
+    #ifndef NDEBUG


@jslee02 what's the rule with indentation and preprocessor commands? Should this be indented or not? Should the for loop be indented more?

We haven't explicitly decided yet, but most people don't indent due to preprocessors. So my preference is like:

{ #ifndef NDEBUG // debug mode for (...) assert(...); #endif }

or

{ #ifndef NDEBUG // debug mode for (...) assert(...); #endif }

Looks like our current settings for formatting prefer the former.

jslee02 · 2018-05-06T18:42:15Z

src/constraint/SequentialSampleable.cpp

+          return true;
+      }
+      mIndex++;
+    }


This logic is still inefficient because of calling canSample(); canSample() iterates over the generators remaining while canSample() is called repeatidly in this logic.

It seems that calling both of canSample() is not necessary. What would be the problem if we remove calling canSample() and just calling sample(...) iterating the generators?

Yeah that was my concern as well. I was wondering if changing mIndex within canSample() would make it any better but I guess that wouldn't be as clean.

I don't think that's a desirable behavior.

Could you point me out what's your concern with the following proposal?:

for (auto i = mIndex; i < mGenerators.size(); ++i) { if (mGenerators[mIndex]->sample(state)) return true; mIndex++; } return false;

The current [updated] implementation has the same behavior as your proposal.

Oh wait, actually I think it is beneficial to get rid of canSample(), for instance if a SequentialSampler is composed of more SequentialSamplers, the current implementation would be still inefficient.

jslee02 · 2018-05-06T18:54:30Z

src/constraint/SequentialSampleable.cpp

+  {
+#ifndef NDEBUG
+    for (const auto& generator : mGenerators)
+      assert(generator->getStateSpace() == mStateSpace);


Nit: Could we add an assertion for checking nullity of the generators?

@jslee02 Should it be an assertion? 🤔

jslee02

It seems this PR is almost there. :) Two minor nitpicks though.

jslee02 · 2018-05-06T19:10:21Z

src/constraint/SequentialSampleable.cpp

+  }
+
+  // Documentation inherited
+  bool sample(statespace::StateSpace::State* state)


Nit: Missing override specifier.

jslee02 · 2018-05-06T19:13:51Z

src/constraint/SequentialSampleable.cpp

@@ -0,0 +1,135 @@
+#include "aikido/constraint/SequentialSampleable.hpp"
+
+#include <dart/common/StlHelpers.hpp>


Nit: I guess you meant dart/common/Memory.hpp for [dart::common::make_unique].

brianhou · 2018-05-07T23:19:11Z

include/aikido/constraint/SequentialSampleable.hpp

+  /// StateSpace in which the constraint operates.
+  statespace::StateSpacePtr mStateSpace;
+
+  /// Set of sampleables.


Nit: set -> sequence

brianhou · 2018-05-07T23:23:04Z

include/aikido/constraint/SequentialSampleable.hpp

+namespace aikido {
+namespace constraint {
+
+/// Sampleable supporting a composition of multiple sampleables which


I think this is confusing. Here's my stab at it:

Sampleable that wraps a sequence of Sampleables. Exhausts the current Sampleable before sampling from the next Sampleable in the sequence.

brianhou · 2018-05-07T23:26:19Z

src/constraint/SequentialSampleable.cpp

+#endif
+  }
+
+  // Documentation inherited


Nit: //==============================================================================, not // Documentation inherited.

In that case, I prefer the current formatting where the declaration happens before definition.

brianhou · 2018-05-07T23:26:24Z

src/constraint/SequentialSampleable.cpp

+    return mStateSpace;
+  }
+
+  // Documentation inherited


Nit: //==============================================================================, not // Documentation inherited.

brianhou · 2018-05-07T23:26:28Z

src/constraint/SequentialSampleable.cpp

+    return false;
+  }
+
+  // Documentation inherited


Nit: //==============================================================================, not // Documentation inherited.

brianhou · 2018-05-07T23:26:40Z

src/constraint/SequentialSampleable.cpp

+    return numSamples;
+  }
+
+  // Documentation inherited


Nit: //==============================================================================, not // Documentation inherited.

brianhou · 2018-05-07T23:28:29Z

src/constraint/SequentialSampleable.cpp

+      throw std::invalid_argument(msg.str());
+    }
+  }
+  // TODO (avk): Somewhere we need to give warning if initial samplers are


This is a good idea. I think something like "the i'th sampleable is infinite, will ignore N subsequent sampleables" would be helpful (if N > 0).

jslee02 · 2018-05-08T12:53:25Z

src/constraint/SequentialSampleable.cpp

+    if (mGenerators[i]->getNumSamples() == NO_LIMIT)
+      dtwarn << "Sampleable " << i << " is infinite. "
+             << "Remaining " << mGenerators.size() - i - 1
+             << " sampleable(s) will potentially be ignored.";


This warning seems very informative! It would be slightly better:

to break once print a warning

not to warn if i is equal to mGenerators.size() - 1; otherwise, mGenerators.size() - i - 1 would be the greatest number of std::size_t.

Agree with the first point, but I think the second point should never happen since the for loop condition is i < mGenerators.size() - 1?

Oh, I missed that. Please ignore the second point.

brianhou · 2018-05-08T17:18:05Z

src/constraint/SequentialSampleable.cpp

+  /// sequence of sampleables
+  SequentialSampleGenerator(
+      statespace::StateSpacePtr stateSpace,
+      std::vector<std::unique_ptr<SampleGenerator>> generators);


Should this be passed by reference? (https://app.codacy.com/app/personalrobotics/aikido/pullRequest?prid=1615713)

Passing by const reference is not allowed because std::unique_ptr is not copiable (see this).

The codacy reports regarding smart pointers are somewhat inaccurate. 😞

brianhou · 2018-05-08T17:18:29Z

src/constraint/SequentialSampleable.cpp

+//==============================================================================
+SequentialSampleGenerator::SequentialSampleGenerator(
+    statespace::StateSpacePtr stateSpace,
+    std::vector<std::unique_ptr<SampleGenerator>> generators)


Should this be passed by reference? (https://app.codacy.com/app/personalrobotics/aikido/pullRequest?prid=1615713)

* sequential sampleable init * implement the functions * make format * add docstring * address JS comments * clean up getNumSamples() * make format * codacy issue resolved * format + fix sample() logic * const correctness * make format * iron out sample() * simplify the logic for sample() * remove unnecessary variables * add nullity check * address JS comments * rename test file for cartesianProductSampleable * init test files * add some initial tests * add more tests * make format * update change log * make format * warning in the gen. constructor * make format * nit warning update * break point

aditya-vk added 4 commits May 4, 2018 16:26

sequential sampleable init

ccfe70f

implement the functions

0095ed9

make format

e635bc2

add docstring

b13a1e8

aditya-vk requested review from brianhou, jslee02 and gilwoolee May 5, 2018 00:01

aditya-vk mentioned this pull request May 5, 2018

Sampling IKs close to the current configuration #384

Closed

jslee02 reviewed May 6, 2018

View reviewed changes

aditya-vk added the pr: work-in-progress Calls for preliminary review of the PR. Remove when PR is ready for complete review. label May 6, 2018

address JS comments

04b2608

aditya-vk removed the pr: work-in-progress Calls for preliminary review of the PR. Remove when PR is ready for complete review. label May 6, 2018

aditya-vk added 4 commits May 5, 2018 23:20

clean up getNumSamples()

69dbd3e

make format

302d17f

codacy issue resolved

e37c1f5

Merge branch 'master'

f5eadd8

jslee02 suggested changes May 6, 2018

View reviewed changes

brianhou suggested changes May 6, 2018

View reviewed changes

format + fix sample() logic

132c02e

brianhou reviewed May 6, 2018

View reviewed changes

aditya-vk and others added 3 commits May 6, 2018 11:32

const correctness

3101535

make format

d422c99

Merge branch 'master' into enhancement/avk/SequentialSampleable

f71d932

jslee02 reviewed May 6, 2018

View reviewed changes

aditya-vk added 3 commits May 6, 2018 11:56

iron out sample()

85ad5d9

merge master

b333d23

simplify the logic for sample()

199d144

jslee02 reviewed May 6, 2018

View reviewed changes

aditya-vk added 9 commits May 6, 2018 12:15

remove unnecessary variables

3884a3c

add nullity check

2501340

address JS comments

0764246

rename test file for cartesianProductSampleable

020ca0e

init test files

23c7c0d

add some initial tests

2af423b

add more tests

1562ff6

make format

279e769

update change log

121c34c

brianhou suggested changes May 7, 2018

View reviewed changes

brianhou and others added 6 commits May 7, 2018 16:29

Merge branch 'master' into enhancement/avk/SequentialSampleable

a50ec6c

make format

302a530

merge remote into local

f31d649

warning in the gen. constructor

db36c88

make format

d1a19f9

nit warning update

8a409c2

jslee02 reviewed May 8, 2018

View reviewed changes

break point

1da6473

brianhou reviewed May 8, 2018

View reviewed changes

jslee02 approved these changes May 8, 2018

View reviewed changes

brianhou approved these changes May 8, 2018

View reviewed changes

brianhou merged commit fe07a1e into master May 8, 2018

brianhou deleted the enhancement/avk/SequentialSampleable branch May 8, 2018 20:49

jslee02 added this to the Aikido 0.3.0 milestone May 9, 2018

		@@ -0,0 +1,117 @@
		#include <aikido/constraint/SequentialSampleable.hpp>

		@@ -0,0 +1,135 @@
		#include "aikido/constraint/SequentialSampleable.hpp"

		#include <dart/common/StlHelpers.hpp>

Create a Sequential Sampleable #393

Create a Sequential Sampleable #393

Uh oh!

Conversation

aditya-vk commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jslee02 May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditya-vk May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

aditya-vk commented May 5, 2018 •

edited

Loading

codecov bot commented May 5, 2018 •

edited

Loading

jslee02 May 6, 2018 •

edited

Loading

aditya-vk May 6, 2018 •

edited

Loading