-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
Sobol sampler implemented #413
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
Sobol sampler implemented #413
Conversation
This pull request introduces 1 alert when merging 37c56d2 into 2a5bbdc - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging eba410c into 2a5bbdc - view on LGTM.com new alerts:
|
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.
Hi. Thank you for interesting PR. I think the current rrt.py should not be changed, you should add a new python script like "rrt_with_sobol_sampler" or something like that.
This pull request introduces 1 alert when merging 4400786 into 840d6af - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 9a8bb39 into 840d6af - view on LGTM.com new alerts:
|
I forgot to write the test!. I'll write it! |
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.
Thank you for very interesting PR. I have some comments. And please add a test code for it. PTAL.
PathPlanning/RRT/sobol/sobol.py
Outdated
import sys | ||
import numpy as np | ||
|
||
# from numpy import np.bitwise_xor |
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.
Please remove this line if it is not needed.
PathPlanning/RRT/sobol/sobol.py
Outdated
global seed_save | ||
global v | ||
|
||
if 'initialized' is None: |
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.
Please fix code scanning warning.
PathPlanning/RRT/sobol/sobol.py
Outdated
Input, real A(M,N), the matrix. | ||
|
||
""" | ||
output = open(filename, 'w') |
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.
Please fix code scanning warning.
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.
Can you use with
statement? like
with open(filename, 'w') as f:
for i in range(0, m):
This pull request introduces 1 alert when merging 4b13c91 into 40df009 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging fbebc7d into 40df009 - view on LGTM.com new alerts:
|
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.
Thank you for patient. This is the last review. After fix these points, I will merge this PR.
seed = 0 | ||
|
||
if (seed == 0): | ||
l_var = 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.
lgtm.com reports this line as unnecessary code, because l_var
is already 1 in line 417.
PathPlanning/RRT/sobol/sobol.py
Outdated
elif (seed <= seed_save): | ||
|
||
seed_save = 0 | ||
l_var = 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.
same as upper comment.
PathPlanning/RRT/sobol/sobol.py
Outdated
Input, real A(M,N), the matrix. | ||
|
||
""" | ||
output = open(filename, 'w') |
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.
Can you use with
statement? like
with open(filename, 'w') as f:
for i in range(0, m):
This pull request introduces 1 alert when merging 6b219a7 into e30696a - view on LGTM.com new alerts:
|
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.
LGTM. Thank you for your patience and your great contribution!!. I will merge it.
What does this implement?
This pull request implements a low discrepancy sequence for the generation of sample points in the RRT method.
Low discrepancy sampler are preferred over uniform random distribution because they provide a more evenly distributed sequence of points.
The Sobol sequence was implemented using the code with MIT licence available here.
Additional information
Sorry if I did not create an Issue before, but the pull request was very straight forward. If it is necessary, I can create the issue.