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

feat: improve handling redistribution of votes #4

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RonaldTreur
Copy link
Contributor

@RonaldTreur RonaldTreur commented Oct 1, 2024

The approach as to how excess votes are redistributed after a winner was selected, or a candidate was eliminated seems too simplistic and honestly just wrong to me.

I added the following two tests:

it('should correctly handle a scenario where a winner has no backup candidates to assign excess votes to', () => {
    // Quota values over time: 90, 30, 16 2/3
    const voteRecords: VoteRecord[] = [
      { voteCount: 170, voteOrder: ['Alice'] },
      { voteCount: 10, voteOrder: ['Alice'] },
      { voteCount: 40, voteOrder: ['Bob'] },
      { voteCount: 25, voteOrder: ['Charlie'] },
      { voteCount: 10, voteOrder: ['Dave', 'Charlie'] },
      { voteCount: 10, voteOrder: ['Eve', 'Charlie'] },
      { voteCount: 5, voteOrder: ['Frank', 'Charlie'] },
    ];

    const { winners, tieCount } = calculateStvWinners(voteRecords, 2);
    expect(winners).toEqual(['Alice', 'Bob']); // Alice wins first, followed by Bob
    expect(tieCount).toBe(0);
  });

  it('should correctly handle a scenario where a winner has a backup candidate defined for only a small portion of excess votes', () => {
    // Quota values over time: 90, 33 1/3, 20
    const voteRecords: VoteRecord[] = [
      { voteCount: 170, voteOrder: ['Alice'] },
      { voteCount: 10, voteOrder: ['Alice', 'Eve'] },
      { voteCount: 40, voteOrder: ['Bob'] },
      { voteCount: 25, voteOrder: ['Charlie'] },
      { voteCount: 10, voteOrder: ['Dave', 'Charlie'] },
      { voteCount: 10, voteOrder: ['Eve', 'Charlie'] },
      { voteCount: 5, voteOrder: ['Frank', 'Charlie'] },
    ];

    const { winners, tieCount } = calculateStvWinners(voteRecords, 2);
    expect(winners).toEqual(['Alice', 'Bob']); // Alice wins first, followed by Bob
    expect(tieCount).toBe(0);
  });

The only difference between the two is that the second person to vote for Alice has selected a backup candidate (Eve).
Unexpectedly (with the current codebase), this resulted in the second test failing, as Eve was selected as the 2nd winner.

The reason for this is that the current code will assign all excess votes (90!) to Eve, as she was the only backup candidate provided. This is an extreme example, but the code responsible did influence the actual outcome of the first round of (the first) Council voting.

My suggestion would be to: Ignore PVP that does not have a backup-candidate provided. Instead, lower the quota accordingly.

To accomplish this, two changes are required:

  • calculate the voteMultiplier by dividing the number of excess total votes by the original total votes. At present, the excess total votes are divided by the total number of backup votes, which means all excess votes are assigned as long as one person provides a backup option (and they are all assigned there).
  • always lower the quota to account for lost votes (no backup option provided) instead of only doing this when no one provided a backup option.

The first change would result in Charlie winning in the second test/example above. Incorporating the second change will pass the win to Bob instead.

A case could be made to subtract only the unassigned excess votes from the quota calculation, in which case Charlie would win in the above test (due to both remaining below the updated quota (60) and Charlie ending up being the last man standing). This is likely a change for another PR.

* calculate the voteMultiplier by dividing the number of excess total votes by the original total votes. Dividing the excess total votes by the total number of backup votes (current) means all excess votes are assigned as long as one person provided a backup option (and they are all assigned there).

* always lower the quota to account for votes that were lost (no backup option provided), instead of only doing this when no one provided a backup option.

A case could be made to subtract the unassigned excess votes only, but that is for another PR.
@@ -41,7 +41,7 @@ describe('calculateStvWinners', () => {
];

const { winners, tieCount } = calculateStvWinners(voteRecords, 3);
expect(winners).toEqual(expect.arrayContaining(['Eve', 'Alice', 'Dave'])); // Eve, Alice, Dave should win
expect(winners).toEqual(expect.arrayContaining(['Eve', 'Alice', 'Bob'])); // Eve, Alice, Bob should win
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated test as the output changed.

Comment on lines -170 to +176
{ voteCount: 2, voteOrder: ['Alice', 'Bob'] },
{ voteCount: 1.5, voteOrder: ['Alice', 'Bob'] },
{ voteCount: 1, voteOrder: ['Charlie', 'Bob'] },
{ voteCount: 1, voteOrder: ['Bob', 'Charlie'] },
];

const { winners, tieCount } = calculateStvWinners(voteRecords, 1);
expect(winners).toEqual(['Alice']); // Alice should win after Charlie is eliminated
expect(winners).toEqual(['Bob']); // Bob should win after Charlie is eliminated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the test so it would actually trigger an elimination (I assume it did originally, but not in the updated code).

Comment on lines -215 to +224
{ voteCount: 2, voteOrder: ['Alice', 'Charlie'] },
{ voteCount: 1, voteOrder: ['Bob', 'Alice'] },
{ voteCount: 1.2, voteOrder: ['Alice', 'Charlie'] },
{ voteCount: 1.1, voteOrder: ['Bob', 'Charlie'] },
{ voteCount: 1, voteOrder: ['Charlie', 'Bob'] },
{ voteCount: 0.9, voteOrder: ['Dave', 'Bob', 'Alice'] },
];

const { winners, tieCount } = calculateStvWinners(voteRecords, 2);
expect(winners).toEqual(['Bob', 'Alice']); // Bob wins first, followed by Alice
expect(tieCount).toBe(0);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the test so it would actually trigger an elimination (I assume it did originally, but not in the updated code).

Comment on lines -752 to +787
redistributeToCandidates(organizedVotes, candidateSet, 1, 10, 30);
redistributeToCandidates(organizedVotes, candidateSet, 1 / 3);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to update the next few tests as less input is passed to this method. This was also part of #3, but I had to do it here as there was just no need for the excess parameters anymore.

Comment on lines -300 to +296
const votesToRedistributeForCandidate =
votesToRedistribute * (vote.totalVotes / totalVotesForProportion);
const votesToRedistributeForCandidate = vote.totalVotes * voteMultiplier;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should have been the case from the get-go, see also #3

It is necessary here though, as we want to ensure we use the updated voteMultiplier.

@Buzzec
Copy link
Member

Buzzec commented Oct 1, 2024

This will be on hold pending a PIP decision.

@Buzzec Buzzec marked this pull request as draft October 1, 2024 19:19
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