Bugfix: max candidates ranked > max_ranking_length#334
Merged
peterrrock2 merged 2 commits intomggg:3.4.0from Feb 28, 2026
Merged
Bugfix: max candidates ranked > max_ranking_length#334peterrrock2 merged 2 commits intomggg:3.4.0from
peterrrock2 merged 2 commits intomggg:3.4.0from
Conversation
…ed more than max_ranking_length candidates (via ties)
peterrrock2
reviewed
Feb 27, 2026
Collaborator
peterrrock2
left a comment
There was a problem hiding this comment.
This is looking great!
Small nit: I am trying to unify the doc string style accoss the repository. Can you modify the doc strings to look more like:
def foo( arg1: str | None, arg2: int = 3) -> str:
"""
Brief description (max 100 chars including indents)
More details (max 100 chars per line, but as many lines as you would like)
Example:
<Stuff goes here. This section is optional>
Args:
arg1 (str | None): description
arg2 (int, optional): description. Defaults to 3.
Returns:
str: The string "baz"
"""
...This is a mild modification of the Google-style doc string and adheres to the PEP 257 recommendation but with the line width modified to 100 chars rather than 80 (better for 16:9 screens IMO)
Contributor
Author
Done! |
peterrrock2
approved these changes
Feb 28, 2026
peterrrock2
approved these changes
Feb 28, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If a RankProfile contains a ballot that ranks more than max_ranking_length candidates, some of the scoring functions can either crash or silently return incorrect results.
For example, consider a profile with a single ballot that ranks candidates A and B as tied for first. first_place_votes(profile, scoring_tie_convention='low') should return 0 for each candidate, but it instead returns 1. The score functions create a score_vector whose length is max_ranking_length, so, in this case, the score_vector is just [1], and when we have two candidates tied for first, taking min([1]) gives both 1. The correct behavior should be: the score_vector is [1, 0], so min yields 0.
To fix this, we should consider such profiles to be malformed, and insist that the user explicitly pass max_ranking_length.
The implementation: RankProfile now has a cached_property max_candidates_ranked, which, at initialization, is computed and compared against max_ranking_length. If max_candidates_ranked > max_ranking_length, RankProfile throws an error and suggests that the user either clean the profile to remove ties or explicitly pass max_ranking_length equal to whatever it computed max_candidates_ranked to be.
I updated the following tests: