-
Notifications
You must be signed in to change notification settings - Fork 84
randomBiasState #70
base: master
Are you sure you want to change the base?
randomBiasState #70
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the small comment above, I'm going to let @joshhting do the full review, but it looks good to me overall! Nice job commenting everything! 😄
I also think this is a nice opportunity to make some pretty graphs about the random distribution, (which we could publish in our TDP next year maybe?) Up to you though!
src/rrt/2dplane/PlaneStateSpace.hpp
Outdated
*/ | ||
POINT_CLASS randomBiasState(const POINT_CLASS& goalState, float goalBias) const { | ||
// Generates random value based on goalBias | ||
float logitX = logit(goalBias, drand48() * .8 + .1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to use the c++ rand
function here
see this: http://en.cppreference.com/w/cpp/numeric/random/rand
You can get a double from it by dividing it with RAND_MAX
. I'm not sure if this random value has special significance though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any advantage in doing that over drand48 though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use the c++ libraries when possible (because they fit in better both in style and in function to the rest of c++).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I said the distribution was fine before, but now that I think about it I think the effect is too pronounced. For a nonzero goal bias, it pretty much defines a small rectangle within the search space with practically visible borders that it searches within, and the nodes are never able to extend to anywhere outside that region. I think the effect should look more gradual than it does right now, so that for small goal biases, the tree is still spread out throughout the whole field. The problem with the current distribution is that when you place an obstacle very near the goal state, it is never able to escape for most goal biases because 100% of the extend() calls extend into the obstacle and fail. I think the best way of solving this is relaxing our bounds from [.1, .9) to something like [.05, .95). Play around with these values when you've got time and see if you can observe a less concentrated distribution.
Also, feel free to push this to a RoboJackets modify-random-pool branch so that people who pull your code don't have to add a new remote for your own fork |
Random bias state
7b90062
to
f3e0df8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is randomBiasState
called anywhere, or is it waiting for an implmementation somewhere? Did you remove it in a later commit (was it being called somewhere before)?
POINT_CLASS randomBiasState(const POINT_CLASS& goalState, float goalBias) const { | ||
// Generates random value based on goalBias | ||
float logitX = logit(goalBias / 3 + .67, (float) rand() / RAND_MAX); | ||
float logitY = logit(goalBias / 3 + .67, (float) rand() / RAND_MAX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this 0.67 value guess and check? Where did it come from?
Is this still going to happen? Regardless of whether we implement this as the default feature or not, it'd be nice to have it as an option that we could set to compare performance. Otherwise this pull request should be closed. |
I would definetly like to merge this (as a seperate option), so that we can use it when tuning things later (which we need to do for performance....) |
Fixes #64
Created a function randomBiasState that returns a point based on a goal state and goal bias using logit function