-
Notifications
You must be signed in to change notification settings - Fork 20
Animations for STV Elections #249
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
base: main
Are you sure you want to change the base?
Conversation
…with empty STVAnimation class.
…imation branch. Add animations module with empty STVAnimation class.
- Remove double underscores - Remove wildcard imports - Change votekit imports to local imports - Minor changes
cdonnay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @prismika. Switching computers here so just submitting a partial review for now, will pick it back up in an hour or so.
| dict: A dictionary whose keys are candidate names and whose values are themselves dictionaries with details about each candidate. | ||
| """ | ||
| candidates = { | ||
| name: {"support": support} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you anticipate having to include other information about the candidates later on? If yes, I can see why the nested dict structure is used. if not, this could just be a dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, candidate nicknames will go here as well.
src/votekit/animations.py
Outdated
| self.rounds = self._make_event_list(election) | ||
| self.title = title | ||
|
|
||
| def _make_candidate_dict(self, election: STV) -> dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you be more specific about type setting here? dict[str, dict[str, float]] for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now dict[str, dict[str, object]]
src/votekit/animations.py
Outdated
| Returns: | ||
| List[dict]: A list of dictionaries corresponding to the rounds of the election. Each dictionary records salient attributes of the corresponding round. | ||
| """ | ||
| events = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know ahead of time how long this list will be? If so it might be more efficient to pre-populate it with dummy entries.
cdonnay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright review complete!
- This looks great already! I think you should feel confident moving onto your other todos.
- I'd encourage a little more modularization. You can see the code for
PreferenceProfileas an example of super tiny private methods that build into the bigger methods. Peter is a big fan of keeping functions short.
src/votekit/animations.py
Outdated
| elected_candidates = [] | ||
| for fset in election_round.elected: | ||
| if len(fset) > 0: | ||
| (name,) = fset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does assume only one candidate is elected at a time? I.e. that the fset has exactly one element?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah it assumes the fset has one candidate, but there can be multiple fsets. Can you double check that the STV election class never elects two cands in one fset? Or add a check here for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced this code with a list comprehension like the ones that exist throughout the codebase. Of the form:
[c for s in election_round.elected for c in s]
src/votekit/animations.py
Outdated
| event_type = "win" | ||
| elected_candidates_str = elected_candidates[0] | ||
| for candidate_name in elected_candidates[1:]: | ||
| elected_candidates_str += ", " + candidate_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly does this do? Formats the name of the elected cand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or concatenates all of the names of all of the winners? If it's this one, can you do this with a .join statement on a list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not know you could do this with a join! It's slick. Added.
| support_transferred : dict[str, dict[str, float]] = {} | ||
| if round_number == len(election): | ||
| # If it's the last round, don't worry about the transferred votes | ||
| support_transferred = {cand: {} for cand in elected_candidates} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you init support_transferred on line 91 to be what you need in line 94? why start with it empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I added this to appease mypy. It liked that the type was clearly declared to make sure the type didn't depend on the control flow.
| support_transferred=support_transferred, | ||
| quota=election.threshold, | ||
| message=message, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
never seen this way of init-ing a dict, does it make all of the keywords without you having to put it in strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly.
src/votekit/animations.py
Outdated
| message=message, | ||
| ) | ||
| ) | ||
| elif len(eliminated_candidates) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can only be 1, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that should be true. Just hedging my bets I suppose.
src/votekit/animations.py
Outdated
| event_type = "elimination" | ||
| eliminated_candidates_str = eliminated_candidates[0] | ||
| for candidate_name in eliminated_candidates[1:]: | ||
| eliminated_candidates_str += ", " + candidate_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, only one can be eliminated, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This code has been tightened up.
|
Since I am not a manim guy, I'm going to skip looking at that code for now since the animation clearly works. But the same overall review comments would apply; aim for super tiny private methods that build into bigger ones (up to reason). @peterrrock2 will check out your manim code when he gets a chance. |
… 'Rounds 3-7: 5 candidates eliminated.'
…left and right sides.
…ariable names to use consistent terminology, etc.
|
Thanks for the code review! I addressed or responded to all the comments. Some changes since the last comments:
Regarding the long methods: the longest two are Given an STV election object from votekit.animations import STVAnimation
animation = STVAnimation(
election,
title = "STV Election for Two Seats",
focus = ['W2', 'W5'] # Elected candidates are automatically focused, so we don't have to list them
)
animation.render(preview = True)produces this animation: ElectionScene.mp4If we like the look of the animation, I can start making snapshot tests! Tasks remaining:
Thanks! |
|
Looking good to @cdonnay and I so far! Let us know if you need more support on this, but so far you are killing it 😁 |
Pursuant to issue #222. This PR adds support for animating STV elections using Manim. Given a votekit STV election called
election, the user can render an animation using, for example,Here is an example of a rendered scene.
ElectionScene2.mp4
This PR is a draft. I have yet to write robust testing for the animation pipeline (such as snapshot testing). I also have yet to implement a few requested features:
Before implementing these I wanted some feedback on my code so far.
Thanks!