-
Notifications
You must be signed in to change notification settings - Fork 22
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
Fix #38: improve dfbuyer random algorithm #39
Fix #38: improve dfbuyer random algorithm #39
Conversation
pdr_backend/dfbuyer/main.py
Outdated
return [k] | ||
num = random.randint(1, k) | ||
return [num] + numbers_with_sum(n - 1, k - num) | ||
numbers = [random.randint(1, k - n + 1) for _ in range(n - 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.
You should use random sampling without replacement.
And to do this, use random.sample()
.
Examples of usage: https://pynative.com/python-random-sample/
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.
Randint is not the way either. You shouldn't have to write a loop. Just use random.sample()
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.
Just to clarify, in this line I'd like to generate n-1
numbers that are between 1
and k - n + 1
. I don't understand how random.sample
is going to work in this case. Could you elaborate on that please?
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.
Something like
random.sample(range(1, k - n + 1), n - 1)
It might be that the first argument needs to be 1 larger or smaller. And same for second argument. I defer to you to test.
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 updated it to use random.sample, could you take a look?
Thanks @trentmc for the review! I've added docstring, inline comments, tests and type declarations. Also reverted the tx arguments changes. |
pdr_backend/dfbuyer/main.py
Outdated
return [k] | ||
num = random.randint(1, k) | ||
return [num] + numbers_with_sum(n - 1, k - num) | ||
numbers = [random.randint(1, k - n + 1) for _ in range(n - 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.
Randint is not the way either. You shouldn't have to write a loop. Just use random.sample()
…er-random-algorithm
Fixes #38
Changes proposed in this PR: