Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix step sequence iterator #305

Merged
merged 6 commits into from
Jan 20, 2018
Merged

Conversation

dqyi11
Copy link
Contributor

@dqyi11 dqyi11 commented Jan 19, 2018

In current aikido, unexpected exception is thrown in some cases.
StepSequence seq(0.2, false); for (double v : seq) { DART_UNUSED(v); count++; }
An exception will be throw

unknown file: Failure
C++ exception with description "Indexed maximum intenger." thrown in the test body.
Expected sequence shall be (0.0, 0.2, 0.4, 0.6, 0.8); but getMaxSteps() returns 6.
Thus an exception is triggered in operator [].

In this PR,
StepSequence seq(0.2, true) returns (0.0, 0.2, 0.4, 0.6, 0.8, 1.0) and getMaxSteps() returns 6.
StepSequence seq(0.2, false) returns (0.0, 0.2, 0.4, 0.6, 0.8) and getMaxSteps() returns 5.
Unit tests are updated accordingly.

  • Document new methods and classes (N/A)

  • Format code with make format

  • Set version target by selecting a milestone on the right side

  • Summarize this change in CHANGELOG.md (N/A)

  • Add unit test(s) for this change

@dqyi11 dqyi11 requested a review from jslee02 January 19, 2018 02:08
@dqyi11 dqyi11 mentioned this pull request Jan 19, 2018
5 tasks
@codecov
Copy link

codecov bot commented Jan 19, 2018

Codecov Report

Merging #305 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
- Coverage   81.13%   81.12%   -0.01%     
==========================================
  Files         204      204              
  Lines        5893     5891       -2     
==========================================
- Hits         4781     4779       -2     
  Misses       1112     1112
Impacted Files Coverage Δ
src/common/StepSequence.cpp 100% <100%> (ø) ⬆️

Copy link
Contributor

@brianhou brianhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the tests! I left a minor suggestion about the logic in this method.

@@ -61,8 +61,11 @@ int StepSequence::getMaxSteps() const
int numSteps = (mEndPoint - mStartPoint) / mStepSize;
if (!mIncludeEndpoints)
{
// Return numSteps + 1 for the start
return numSteps + 1;
if (fabs(mStartPoint + mStepSize * numSteps - mEndPoint) < 1e-7)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the current logic somewhat difficult to follow. Can we do something like:

int numSteps = (mEndPoint - mStartPoint) / mStepSize;
if (fabs(mStartPoint + mStepSize * numSteps - mEndPoint) > 1e-7)
  numSteps++; // include start
return numSteps + mIncludeEndpoints;

@jslee02 jslee02 added this to the Aikido 0.3.0 milestone Jan 19, 2018
@brianhou brianhou merged commit 3a85944 into master Jan 20, 2018
@brianhou brianhou deleted the bugfix/dqyi/fixStepSequenceIterator branch January 20, 2018 02:09
gilwoolee pushed a commit that referenced this pull request Jan 21, 2019
* fix step sequence iterator

* fabs --> std::abs

* address Brian's comments

* Update CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants