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

STV Updates #111

Closed
wants to merge 3 commits into from
Closed

STV Updates #111

wants to merge 3 commits into from

Conversation

cdonnay
Copy link
Collaborator

@cdonnay cdonnay commented Nov 16, 2023

When running an STV election, I noticed that you cannot use the run_election method more than once. Moreover, I noticed that after run_election, the original preference profile was edited, not just the one stored in self.state.profile.

  1. I implemented a reset method in the Election class that resets self.state to round 0 and the original preference profile.
  2. The get_ballots method was returning the PreferenceProfile list of ballots, which was why running elections altered the original profile. I used deepcopy to return a copy of the ballots, which fixed the issue.
  3. I implemented a step parameter in the STV run_step method that allows users to choose which step they want to run. It defaults to None, in which case it just runs the next step.
  4. I added a ValueError to run_step so that if you try to run a step after all seats are filled, it raise the error.

@jamesturk @drdeford @jgibson517 @jennjwang @ziglaser

@cdonnay
Copy link
Collaborator Author

cdonnay commented Nov 16, 2023

If folks like the reset/choose step functionality, I'm happy to add it to other election types in a different PR.

@jgibson517
Copy link
Member

I noticed that issue about re-running elections as well. I think a good fix is to cache the results of run_election, you can see what this looks like here (e59103f). Once a user calls run_election the results are cached so they can be return if that method is called again.

On point 1) I think the reset method is good for users who want to re-run certain rounds of an election.

For 3), deepcopy was originally how we handled that but it kills the computation time, especially when you simulate a lot elections in row so, I'm hesitant to add it back in.

On 3) and 4), I think this is less intuitive from a user's point of view, but implementation-wise it makes mores sense to have step parameter to the run_election method, since to run step 5 of a 12 round election, steps 1-5 also have to be run. @jamesturk any thoughts here?

@jamesturk
Copy link
Collaborator

Taking a look at the code, I'd agree that step should be a parameter to run_election (or a separate function altogether like run_to_step), this keeps run_step clean so that it only needs to run a single step (the branch that calls it recursively in the step>0 case seems like a different function to me).

@cdonnay
Copy link
Collaborator Author

cdonnay commented Nov 28, 2023

I'm going to close this PR and implement the suggestions above in a new one. I will be making a reset method, a run_to_step method, and I believe Jack has fixed the deepcopy issue in a smarter way in a different PR.

@cdonnay cdonnay closed this Nov 28, 2023
@cdonnay cdonnay deleted the election_class branch November 30, 2023 18:52
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