feat: truncate ranked ballots at sentinel candidates - #383
Conversation
Add profile cleaning for CVR markers such as overvote and undervote. Preserve the existing cleaning metadata and cover truncation, retention options, and profile validation.\n\nFixes mggg#380\n\nAssisted-by: Codex:gpt-5
fetachino
left a comment
There was a problem hiding this comment.
I checked the first-match truncation logic, including a marker after an empty rank and the case where the marker shares a tied position. Extending with trailing placeholder frozensets keeps the dataframe width stable, and delegating profile construction to clean_rank_profile preserves the existing empty/zero-weight and candidate-list semantics.
I ran the new truncation tests with the neighboring clean-profile tests: 5 passed. Ruff and ty check also pass for the touched cleaning code and test. I found no blocking issue in the current patch. Since this PR is still a draft, I would re-check this review if the implementation changes materially before it is marked ready.
|
Thanks for the thorough review of the truncation logic and the 5-test validation. I'll keep the patch stable and re-request review when it is marked ready. |
|
Hey @aryansk! Thank you so much for contributing to VoteKit! It's wonderful to see our work being used by people outside of our community. If you happen to have a moment, I would love to hear more about how you found the package and what you are using it for so that we can better fit the needs of our users. You may have noticed that we made our latest release of VoteKit around two weeks ago. Since then, I have been preoccupied with some other projects and missed the latest set of PRs that came in. This looks like a great start on this issue, and one of the other maintainers will be adding a review with some additional feedback shortly. I also saw you mention that you were assisted by Codex when working on this issue. Thank you for including that information. As an academic lab, we value transparency of process, and knowing this sort of thing is genuinely helpful. I think that, until this point, we have not been clear about our preferences for how contributors use AI tools when submitting work for review on this project. I hope the following makes this clear, but I want to state it explicitly anyway: the recent updates to this policy are not an indictment of any of the work that you have done here. As a maintainer of a small OSS package, it's always heartening to see more people trying to become a part of the community, and I am truly grateful for the work that you have put in. Additional notes (this section is a form that I am including on several recent PRs) Since there have been so many new contributors in the last couple of weeks (a wonderful problem to have), I thought it important to update the "Use of AI Tools" section of the Contributing.md on our main branch to clarify what we currently consider to be best-practices for our lab. To be clear, this this is not an indictment of the use of any AI tools or a statement about the work presented here (indeed, it would be a bit illogical to find anyone at issue with this policy before it was explicitly stated!). I include it here only because this is 1. a recent update in a document people infrequently visit and 2. an effort to clearly communicate our expectations around contributing to this project. I'll include the exact snippet here so you don't have to go through the trouble of finding it yourself:
Thank you again for your contribution, and I hope that we'll see more from you soon! -Peter |
There was a problem hiding this comment.
Nice job! Looks like the function properly truncates at the specified candidate(s). Added some comments around documentation, handling retain_original_candidate_list, testing, and some code refactoring of the if/else block and naming of functions/variables.
| for cand_set in ranking_tup: | ||
| if cand_set.isdisjoint(removed_set): | ||
| out.append(cand_set) | ||
| continue | ||
|
|
||
| out.extend([frozenset("~")] * (len(ranking_tup) - len(out))) | ||
| break |
There was a problem hiding this comment.
[nit] Suggest to make an explicit if/else. The if and trailing break reads as two statements to trace when it's really one decision: add the cand_set or truncate at removed_set. Or, have only one if that checks whether the cand_set contains a removed_set candidate, truncate and break if so and add the cand_set to the cleaned ranking as the default.
| assert cleaned_profile.no_wt_altr_idxs == set() | ||
|
|
||
|
|
||
| def test_truncate_rank_profile_can_retain_empty_and_zero_weight_ballots(): |
There was a problem hiding this comment.
This test does not cover retaining empty ballots. Only zero weight ballots.
There was a problem hiding this comment.
The undervote ballot would become an empty ballot after cleaning. Could truncate at that candidate to test empty ballots are retained.
| out: list[frozenset] = [] | ||
|
|
||
| for cand_set in ranking_tup: | ||
| if cand_set.isdisjoint(removed_set): |
There was a problem hiding this comment.
Currently, this will truncate ballots where a candidate within the removed_set exists. So, it will truncate where that candidate may be tied with other non-removed candidates. This is covered in your documentation but could be nice to cover in a test.
| ranking_tup (tuple): Ranking to truncate. | ||
|
|
||
| Returns: | ||
| tuple: Ranking truncated at the first matching position. |
There was a problem hiding this comment.
Update docstring to tuple[frozenset, ...] to match the function signature. It's better to be specific because a user could read the docstring and assume they can pass any tuple as an argument. I see this is done throughout the file so I will update the doc strings elsewhere!
|
|
||
|
|
||
| def truncate_rank_profile( | ||
| removed: Candidate | CandidateList, |
There was a problem hiding this comment.
Would move away from removed because this function is not only removing this candidate but removing all rankings/candidates below that candidate.
|
|
||
| This is useful for cleaning CVR data where values such as ``"overvote"`` or ``"undervote"`` | ||
| terminate the meaningful portion of a ballot. The matching position and all lower-ranked | ||
| positions are removed. Ballots without a matching value are retained unchanged. |
There was a problem hiding this comment.
You're right that this function is especially useful for cleaning CVR data with ballot error markers or end-of-ballot markers but it doesn't need to be captured in the docstring. And then add "Wrapper for clean_rank_profile that does some extra processing to ensure the candidate list is handled correctly."
| remove_zero_weight_ballots, | ||
| retain_original_candidate_list, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Add candidate list handling here for retain_original_candidate_list. The candidate list is important metadata to maintain for an election/profile. User expects the "truncate_at" candidate to be removed but may want to retain all other candidates even if after truncation, some are no longer casted. Follows the implementation of the remove candidate cleaning functions.
| ) | ||
|
|
||
|
|
||
| def truncate_ranking_row( |
There was a problem hiding this comment.
Would update name to be something like truncate_at_cand_ranking_row or truncate_ranking_row_at_cand to be more specific about the type of truncation. Same comment for truncate_rank_profile.
Summary
truncate_rank_profilefor CVR markers such asovervoteandundervoteCleanedRankProfilemetadataFixes #380
Validation
uv run pytest -o addopts="" tests -q— 1,607 passed, 22 skippeduv run ty check src testsuv run ruff check ...git diff --checkAssisted-by: Codex:gpt-5