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

Bug fix in ties #17

Merged
merged 5 commits into from
Aug 28, 2022
Merged

Bug fix in ties #17

merged 5 commits into from
Aug 28, 2022

Conversation

mikefranze
Copy link
Contributor

When resolving ties we're extending the runoff candidates list with the tie winner. extend expects a list, so when the candidate names are strings it would break up the string.

Instead of ['Allison', 'Bill'] the runoff candidates would be ['Allison', 'B', 'i', 'l', 'l'] which causes the error later on found in #16

@endolith
Copy link
Contributor

endolith commented Aug 7, 2022

This fixes the first example, but not the others. Should I file a second bug or just list them here?

        columns = ['Allison', 'Bill', 'Carmen', 'Doug']
        election = [[5, 4, 1, 4],
                    [5, 4, 1, 4],
                    [2, 4, 1, 2],
                    [4, 3, 2, 1],
                    [0, 5, 4, 4],
                    [3, 2, 4, 2],
                    [3, 1, 5, 3],
                    [3, 1, 5, 3],
                    [1, 3, 2, 2],
                    [4, 3, 5, 5]]
        ballots = pd.DataFrame(columns=columns, data=election)
        results = STAR(ballots)

        columns = ['Allison', 'Bill', 'Carmen', 'Doug']
        election = [[5, 4, 1, 4],
                    [5, 4, 1, 4],
                    [2, 4, 1, 2],
                    [4, 3, 2, 1],
                    [0, 5, 4, 4],
                    [3, 2, 4, 2],
                    [3, 1, 5, 3],
                    [3, 1, 5, 3],
                    [1, 3, 2, 2],
                    [4, 3, 5, 5]]
        ballots = pd.DataFrame(columns=columns, data=election)
        results = STAR(ballots)

both produce:

STAR.py", line 216, in STAR
    results['elected'].extend(round_results['winners'])

TypeError: 'NoneType' object is not iterable

@mikefranze
Copy link
Contributor Author

Other bug fixed in latest commits.

@endolith
Copy link
Contributor

endolith commented Aug 8, 2022

Would it make more sense to return sets instead of lists? For things like 'runoff_candidates', since the order doesn't matter?

When writing tests, for instance, 'runoff_candidates': [0, 1] doesn't match [1, 0]

@mikefranze
Copy link
Contributor Author

Correction on last commit: some test assertions were commented out and I also found a weird bug.

If I run STAR_Test.py, all tests pass. But if I run pytest, 2 tests fail. It looks like the two ways of calling pytest result in different outputs.
using
pytest
{'elected': [0], 'round_results': [{'winners': [0], 'runner_up': 1, 'logs': [{'top_score': [1]}, {'top_score': [0]}, {'runoff_candidates': [1, 0]}]}]}

using
STAR_Test.py
{'elected': [1], 'round_results': [{'winners': [0], 'runner_up': 1, 'logs': [{'top_score': [1]}, {'top_score': [0]}, {'runoff_candidates': [1, 0]}]}]}

@endolith
Copy link
Contributor

endolith commented Aug 9, 2022

If you start a new kernel and run STAR_Test.py again does it pass?

pytest-dev/pytest#3143 (comment)

Oh wait no you said pytest fails but running the file doesn't. Not sure why that would happen, will look later

@endolith
Copy link
Contributor

When I checkout 666aea1 I don't see any failures, just 7 test passing, whether running STAR_Test.py directly or running pytest from command line.

mikefranze and others added 2 commits August 9, 2022 20:27
@mikefranze
Copy link
Contributor Author

So I tracked it down to the scores.argmax() line. Running the test the two different ways resulted in different outputs. I updated to not use it and make code more readable.

@endolith
Copy link
Contributor

That's strange, why would it behave differently when run different ways?

@mikefranze mikefranze merged commit 0e211db into Equal-Vote:main Aug 28, 2022
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.

2 participants