-
Notifications
You must be signed in to change notification settings - Fork 10
Borda voting rule added #5
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
base: master
Are you sure you want to change the base?
Conversation
not completley implemented yet.
not completley implemented yet.
modified: py3votecore/borda_at_large.py
modified: test_functionality/test_borda_at_large.py
modified: py3votecore/borda_manipulation_heurestics_methods.py modified: test_functionality/test_borda_manipulation_heurestics_methods.py
modified: py3votecore/borda_at_large.py modified: py3votecore/borda_manipulation_heurestics_methods.py modified: test_functionality/test_borda_manipulation_heurestics_methods.py
…exist modified: py3votecore/borda_manipulation_heurestics_methods.py modified: test_functionality/test_borda_manipulation_heurestics_methods.py
modified: py3votecore/borda_manipulation_heurestics_methods.py
modified: py3votecore/borda_manipulation_heurestics_methods.py
Working branch
modified: py3votecore/borda_manipulation_heurestics_methods.py modified: test_functionality/test_borda_manipulation_heurestics_methods.py
modified: test_functionality/test_borda_manipulation_heurestics_methods.py
modified: test_functionality/test_borda_manipulation_heurestics_methods.py
Working branch
new file: requirement.txt modified: setup.py modified: test_functionality/test_schulze_method.py
Added requirements and fixed a test.
modified: README.rst modified: py3votecore/borda.py modified: py3votecore/borda_at_large.py modified: py3votecore/borda_manipulation_heurestics_methods.py modified: setup.py modified: test_functionality/test_borda.py modified: test_functionality/test_borda_at_large.py modified: test_functionality/test_borda_manipulation_heurestics_methods.py
Added description of the added Borda votting rule and version update
| import copy | ||
|
|
||
| class BordaAtLarge(MultipleWinnerVotingSystem): | ||
|
|
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.
Add class documentation and doctest.
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.
Added. Thank you
| from .borda_at_large import BordaAtLarge | ||
|
|
||
| class Borda(AbstractSingleWinnerVotingSystem): | ||
|
|
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.
Add class documentation and doctest.
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.
Added. Thank you
modified: py3votecore/borda.py modified: py3votecore/borda_at_large.py
the-maldridge
left a comment
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.
Many small nits. This is a first pass review that does not check actual conformance against the algorithm in the linked whitepaper.
Each review item is commented on the first time I saw it, but multiple occurrences may exist within your patch. You will need to rebase your commits into a single logical change and push back to this PR prior to merge, but as I have not yet verified the correctness of the code, this is not required yet.
I found this PR difficult to review due to the lack of whitespace in most files. I strongly encourage you to refactor your code such that comments appear above the line they explain and to generally use whitespace to break up large blocks of code based on what each subsection does.
|
|
||
| def __init__(self, ballots, tie_breaker=None): | ||
| """ | ||
| The constructer accepts ballots of voters and a tie breaker if given. |
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.
Indeed it does expect ballots and a tie breaker, but the format would be useful to know for anyone consuming this class. You should amend the comment to include a brief description of the expected format of the ballots.
| @@ -0,0 +1,36 @@ | |||
| # Copyright (C) 2023, Leora Schmerler | |||
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.
name should be added to CONTRIBUTORS at the root of the repo.
|
|
||
| def calculate_results(self): | ||
| """ | ||
| calculate_results accepts an instance of Borda, and is called from the constructor. |
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.
Typically you would not state that it accepts anything if the only argument is the class instance itself. This should be reworded to describe what the function actually does.
|
|
||
| def as_dict(self): | ||
| """ | ||
| as_dict accepts an instance of Borda, and returns a dict of the BordaAtLarge. |
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 above, reword docstring.
| candidate will be elected by the Borda voting rule, tie-breaks in faver of the preferred_candidate. The algorithm outputs true if it succeeds | ||
| to find such manipulation, and false otherwise. | ||
| Programmer: Leora Schmerler |
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.
Move to CONTRIBUTORS in root.
|
|
||
| def AverageFit(ballots: list, preferred_candidate: str, k: int)->list or bool: | ||
| """ | ||
| "Complexity of and Algorithms for Borda Manipulation", by Jessica Davies, George Katsirelos, Nina Narodytska and Toby Walsh(2011), |
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.
If this reference is here to explain something you're going to need more words to explain that.
|
|
||
| setup(name='python3-vote-core', | ||
| version='20170329.00', | ||
| version='20230116.00', |
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.
This should be removed, as your PR is not what will trigger a release build.
| output_tuple = SchulzeMethod(input_tuple, ballot_notation=SchulzeMethod.BALLOT_NOTATION_GROUPING).as_dict() | ||
| output_list = SchulzeMethod(input_list, ballot_notation=SchulzeMethod.BALLOT_NOTATION_GROUPING).as_dict() | ||
|
|
||
| # Run tests |
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.
This appears to be unrelated to the rest of your PR, and should be part of a separate patch.
I added the Borda voting rule for one winner and for more winners, similar to the implementation of the Plurality voting rule. In addition, I added an implementation of the coalitional manipulation heuristics: Average Fit and Largest Fit.
Another minor fix in test_schulze_method: the test test_tuple_ballots somtimes fails, and somtimes does not, for the random tie-breaking order.