personalrobotics / prpy

Python utilities used by the Personal Robotics Laboratory.
BSD 3-Clause "New" or "Revised" License
62 stars 19 forks source link

Changes VanDerCorputSampleGenerator to include goal. #304

Closed psigen closed 8 years ago

psigen commented 8 years ago

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.

psigen commented 8 years ago

@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.])
psigen commented 8 years ago

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 commented 8 years ago

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 https://github.com/personalrobotics/aikido/pull/37.

psigen commented 8 years ago

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 commented 8 years ago

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