Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: implement random sampling without replacement and staking power #83
feat: implement random sampling without replacement and staking power #83
Changes from 11 commits
fbc390c
9b8d758
d68124d
3749b65
521cdc1
5a4cf87
85a08e1
c99b7bd
41fe4c2
0f2d198
37a2431
52de880
b2247ba
8e74da4
8eda4c1
b85eedb
fcd158b
af8ea82
aac3c8b
9e8dc55
d5cf9b8
cbe2847
45e573e
cb7f045
a3835cb
b1f68f5
d930617
083f637
fdb7a02
e33896f
0a18969
f08edeb
08b7835
deb89eb
d30feec
06e84e8
4387cb6
94b2975
3efc1d7
1c759e3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't think
MaxFloat64Significant
masking has much effect, since the division is done afterward. If we should eliminate the uncertainty introduced by floating-point arithmetic rounding, it's best to usebig.Int
, as shown below.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.
Okay, I'll apply 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.
Could you make sure that the idempotency with staking is not broken?
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.
Good point. Okay.
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.
Putting the winners after the
candidates
array seems to be a bit of a tricky code, but what's the purpose? I don't think the effect of saving small buffer space is significant, and the risk of allowingcandidates
to overrun references seems to be greater.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.
Even if we create a new array for winners and take winners out of the candidates and put them in the winner list, the overrun references problem is the same unless we reduce the capacity of the candidates(we should re-allocate array to reduce the capacity). I don't know why I have to make a new array for winners.
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.
Well, this is a trivial point, so I think it's also a good idea to reuse the tail in the area of the winner. I'm also slightly concerned that when it
moveWinnerToLast
, the winner's area also moves.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 looks like that the boundary condition for the cumulative summation is designed not to select a zero-priority candidate. If an assertion is required, I think it's sufficient to make sure that
winner.Priority() != 0
when a winner was selected.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.
As you said, the restriction that a candidate's priority cannot be zero seems to be bad. However, it seems that the loop escape condition should be added from the sampling algorithm to assume zero.
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.
And I think we should remove validator having 0 staking power from voter set in
ToVoterAll()
function.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 added some test cases having 0 staking power.
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.
The first comment means that a Candidate with Priority() of 0 would fundamentally not be selected in the above code (in other words, the presence or absence of a candidate with Priority=0 doesn't affect the results). So this is a comment that I think it seems to be enough to verify Priority!=0 only to the winner for
assert
purposes.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.
The
sum
isn't necessary because it's declared in the return value. In golang fashion, I think it should be specified explicitly by return only when returning a value other than the one declared in the return value.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.
Okay