Skip to content
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

Implement --minimum-confidence parameter for vCA reward command #156

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

2072
Copy link

@2072 2072 commented Aug 1, 2022

This PR implements a new command line parameter: --minimum-confidence which takes a value between 0.5 and 1

It represents the minimum confidence for a vCA ranking to be excluded from eligible rankings when in disagreement from simple majority.
Confidence is either #FO / #Rankings or (#Excellent + #Good) / #Rankings depending on the final ranking.

Simple majority is 50%. Qualified majority is 70%. Using 70% avoids punishing vCAs where this confidence is not clear.

70% is because when #vCA == 3 confidence is only 66% and thus in this case, where there is just 1 vote in disagreement, all 3 vCAs get rewarded.

Note: to get the previous behavior just set this parameter to 0.5: --minimum-confidence 0.5

@2072
Copy link
Author

2072 commented Aug 1, 2022

Here is a spreadsheet showing the difference of output: https://docs.google.com/spreadsheets/d/1Sam0fcEIRdx7flujAiKzS0zcM60UfdQupJBy8altbzw/edit#gid=0

Note that I used the f8-vca-merged.csv that was removed a few commits ago.

@2072
Copy link
Author

2072 commented Aug 2, 2022

I've updated the spreadsheet and also added the output of the original tool without this patch into a new tab to show that it is identical when using --minimum_consensus 0.5.
Note that this is the first time I deal with Rust so do not hesitate to tell me if there are better ways,,,

Value in range [0.5, 1]
The minimum consensus for a vCA ranking to be excluded from eligible rankings when in disagreement from simple majority.
Simple majority is 50%.
Qualified majority is 70%. Using 70% avoids punishing vCAs where the consensus is not clear.
70% is because when #vca == 3 consensus is only 66% and thus in this case, where there is just 1 vote in disagreement, all 3 vCAs get rewarded.
@2072 2072 force-pushed the vca_rewards_minimum_consensus branch from d2cce74 to c4e6dfc Compare August 2, 2022 02:52
@minikin minikin requested a review from zeegomo August 2, 2022 08:39
@zeegomo
Copy link
Contributor

zeegomo commented Aug 2, 2022

Hey @2072, thank you for your PR!
We do already filter out from rewards advisors in disagreement with the majority in

(exact threshold is tunable from cli).
Or is what you are proposing different from that?

Also, I'm lacking a bit of context, is there a broader consensus behind this change or is it just your (valid) suggestion?

@2072
Copy link
Author

2072 commented Aug 2, 2022

We do already filter out from rewards advisors in disagreement with the majority in

(exact threshold is tunable from cli).
Or is what you are proposing different from that?

No this is different, it's about not making a review not legitimate when the vcas are on a tie on whether it's good or FO. Which in turn prevents these vCAs from seeing their agreement rate falling below one of these tunable threshold you mentioned and being unduly punished for a crime they did not commit.

Also, I'm lacking a bit of context, is there a broader consensus behind this change or is it just your (valid) suggestion?

This change was discussed and accepted by the community but we were told it was too late to be implemented for this fund given the delay and that more data was needed. So I took the opportunity and did it myself so you get the change and the data :-)

Here is the google doc where this change was discussed: https://docs.google.com/document/d/1bN3L0-R7ntmKKY9KwQTeevQLuDExyJ1NkH7b7-uOXtE/edit#heading=h.dmlc2526vg0q

Copy link
Contributor

@zeegomo zeegomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! I left some comments

catalyst-toolbox/src/rewards/veterans.rs Outdated Show resolved Hide resolved
catalyst-toolbox/src/rewards/veterans.rs Outdated Show resolved Hide resolved
catalyst-toolbox/src/rewards/veterans.rs Outdated Show resolved Hide resolved
catalyst-toolbox/src/bin/cli/rewards/veterans.rs Outdated Show resolved Hide resolved
catalyst-toolbox/src/bin/cli/rewards/veterans.rs Outdated Show resolved Hide resolved
…sus_per_review() into calc_final_ranking_with_consensus_per_review()

This also fixes a small bug where the consensus was lowered on Excellent assessments because the Good ones count was not taken into account, This had a low impact since Excellent assessments are much rarer. (The final_ranking_is_correct test caught this.)
@2072 2072 requested a review from zeegomo August 2, 2022 18:04
@2072
Copy link
Author

2072 commented Aug 2, 2022

OK, all your requested changes are done, this allowed me to catch a small issue on the consensus of Excellent assessment where the number of Good where not taken into account (very low impact on rewards du to the rareness of Excellent and the rareness of positive disagreement on those).

catalyst-toolbox/src/rewards/veterans.rs Outdated Show resolved Hide resolved
catalyst-toolbox/src/rewards/veterans.rs Outdated Show resolved Hide resolved
catalyst-toolbox/src/rewards/veterans.rs Outdated Show resolved Hide resolved
@2072 2072 requested a review from zeegomo August 3, 2022 12:20
Copy link
Contributor

@zeegomo zeegomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last few things

catalyst-toolbox/src/rewards/veterans.rs Outdated Show resolved Hide resolved
catalyst-toolbox/src/rewards/veterans.rs Outdated Show resolved Hide resolved
The documentation added is enough to clear any doubts and "confidence" does not imply as much as does the term "consensus" which prevent false assumptions.
@2072 2072 changed the title Implement --minimum_consensus parameter for vca reward command Implement --minimum-confidence parameter for vCA reward command Aug 3, 2022
@2072 2072 requested a review from zeegomo August 3, 2022 14:39
Copy link
Contributor

@zeegomo zeegomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work

@2072
Copy link
Author

2072 commented Aug 4, 2022

Good work

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants