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

BugFix: Van der Corput - consider span for resolution check #552

Merged
merged 4 commits into from
Oct 8, 2019

Conversation

aditya-vk
Copy link
Contributor

@aditya-vk aditya-vk commented Sep 13, 2019

Expected behavior for VDC with resolution 0.5:

  1. If an edge is of length(span) 1, output is 0, 0.5, 1.
  2. If the edge is of length(span) 2, output is 0, 0.5, 1.0, 1.5, 2.0.

Current behavior generates sequence for [0,1] and multiplies with length thus giving

  1. 0, 0.5, 1.0
  2. 0, 1.0, 2.0

We have been using Van der Corput sequence only for interpolated trajectories with [0, 1] time so it was not an issue.

This PR introduces a test that fails with the current master and proposes a fix.


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

@aditya-vk aditya-vk added the bug label Sep 13, 2019
@aditya-vk aditya-vk added this to the Aikido 0.3.0 milestone Sep 13, 2019
Copy link
Member

@jslee02 jslee02 left a comment

Choose a reason for hiding this comment

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

Good catch! This fixes for the VDC value, but the resolution seems to need to be fixed as well.

Not critical for this PR, but I found a type while reading the header here: Construts --> Constructs

EXPECT_DOUBLE_EQ(2.0 / 8, vdc[3].first);
EXPECT_DOUBLE_EQ(10. / 8, vdc[4].first);
EXPECT_DOUBLE_EQ(6.0 / 8, vdc[5].first);
EXPECT_DOUBLE_EQ(14. / 8, vdc[6].first);
Copy link
Member

Choose a reason for hiding this comment

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

Why not keeping this test? Isn't this still a valid test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured the replaced test would suffice for both. I brought this back and added a separate test for this bugfix.

@aditya-vk
Copy link
Contributor Author

@jslee02 fixed both the value as well as the resolution.

jslee02
jslee02 previously approved these changes Oct 8, 2019
Copy link
Member

@jslee02 jslee02 left a comment

Choose a reason for hiding this comment

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

LGTM!

As a note, I still see the type here. Don't need to be fixed by this PR though.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Brian Hou <brianhou@users.noreply.github.com>
@brianhou brianhou merged commit d374e55 into master Oct 8, 2019
@brianhou brianhou deleted the avk/bugfix/vandercorput branch October 8, 2019 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants