Skip to content

Conversation

@leorasc
Copy link

@leorasc leorasc commented Jan 16, 2023

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.

	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
	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
	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):

Copy link

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.

Copy link
Author

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):

Copy link

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.

Copy link
Author

Choose a reason for hiding this comment

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

Added. Thank you

Copy link
Owner

@the-maldridge the-maldridge left a 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.
Copy link
Owner

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
Copy link
Owner

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.
Copy link
Owner

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.
Copy link
Owner

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
Copy link
Owner

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),
Copy link
Owner

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',
Copy link
Owner

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
Copy link
Owner

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.

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.

3 participants