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

Changes VanDerCorputSampleGenerator to include goal. #304

Merged
merged 3 commits into from
May 10, 2016
Merged

Conversation

psigen
Copy link
Member

@psigen psigen commented Apr 29, 2016

This refactors VanDerCorputSampleGenerator to include the start and end (goal) values as the first two samples to be checked, followed by the discretized intermediate checks within the interval range.

This is an alternate way to address #297 and #303, but avoids the non-termination problem introduced in #303 by simplifying the generator loop. The resulting output should be identical to the expected output of #303.

@mkoval @DavidB-CMU let's continue the discussion from #303 here.

This refactors VanDerCorputSampleGenerator to include the start
and end (goal) values as the first two samples to be checked,
followed by the discretized intermediate checks within the
interval range.

This is an alternate way to address #297 and #303, but avoids
the non-termination problem introduced in #303 by simplifying
the generator loop.  The resulting output is identical to the
expected output of #303.
Fixes non-termination issue in sampling loop.
@psigen
Copy link
Member Author

psigen commented Apr 29, 2016

@mkoval: Heads-up that the VDC unit test will fail because it slightly disrupts the VDC sequence used as the "correct" output by not doing all the "step" stuff.

It's no big deal, since it gets the same values in an alternate maximum dispersion sequence and I'll fix the unit test before merge, but I wanted to wait until after we converged on discussion.

prev: array([  0.,  11.,   6.,   3.,   8.,   1.,   7.,   4.,  10.,   2.,   5.,   9.])
this: array([  0.,  11.,   6.,   3.,   9.,   2.,   7.,   5.,  10.,   1.,   4.,   8.])

After examining the differences between the sequences, I think that
these outputs are consistent with the intent of the new generator.
@psigen
Copy link
Member Author

psigen commented May 10, 2016

Ok, since I didn't hear any feedback and the sequences seemed to be pretty correct, I changed the unit tests to match them.

@mkoval
Copy link
Member

mkoval commented May 10, 2016

I agree that we explicitly check the endpoint of trajectories inside motion planners. However, I also agree with @DavidB-CMU that this does not belong in our implementation of the Van de Corput sequence - which does not inherently have anything to do with collision checking or trajectories.

I would prefer to either:

  1. manually check the endpoint of trajectories in each planner
  2. add an optional check_endpoints argument to the Van de Corput sequence that defaults to False

I am slightly in favor of (2) because it matches the API that @ClintLiddick implemented in personalrobotics/aikido#37.

@psigen
Copy link
Member Author

psigen commented May 10, 2016

That is why I didn't modify the VanDerCorputSequence, I modified the VanDerCorputSampleGenerator. From its docstring:

This wraps VanDerCorputSequence() in a way that's useful for collision-checking.

This function is NOT the Van Der Corput sequence at all. It does the relatively arbitrary step-size discretization that we only use for collision checking. The VanDerCorputSequence already has an include_endpoints option, which we use in this very function.

@mkoval
Copy link
Member

mkoval commented May 10, 2016

Sorry - I forgot that these were split. This is the right place to add that this functionality. 👍

@mkoval mkoval merged commit ab6f31b into master May 10, 2016
@mkoval mkoval deleted the bugfix/vdc_goal branch May 10, 2016 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants