mggg / VoteKit

A Swiss Army Knife for computational social choice research
https://votekit.readthedocs.io/en/latest/
MIT License
10 stars 12 forks source link

STV Updates #111

Closed cdonnay closed 9 months ago

cdonnay commented 9 months ago

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 commented 9 months ago

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

jgibson517 commented 9 months ago

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 (https://github.com/mggg/VoteKit/pull/107/commits/e59103f9c786d4bc2f81f50ce5f5fcd4e6b7ff0a). 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 commented 9 months ago

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 commented 9 months ago

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.