nathanllww / slivka-irv

MIT License
1 stars 0 forks source link

Handling unbreakable ties #8

Closed abugler closed 3 years ago

abugler commented 3 years ago

I don't this is a good way of handling unbreakable ties: https://github.com/nathanllww/slivka-irv/blob/master/irv/irv.py#L360

The ValueError should not be used for when the algorithm reached a fail state, it should be used when it is passed an invalid value. An election that has an unbreakable tie is not invalid data, but rather not an election we can resolve.

I was going also say that this "fail state" the algorithm finds itself in shouldn't be an exception, but there are similar examples to this in numpy we can point to where something very similar happens. (numpy.linalg.inv throws if passed a singular matrix) However, this stops the execution of the command line main function. So we either must:

  1. Remove the exception
  2. Handle the exception in the main

This also opens a bigger question: Is the sole purpose of the irv program to find a winner? Because if it is, then the exception makes a lot of sense, because we have failed to deliver a winner. However, this program does much more than that! At the very least, it gives steps, so while we would fail to give a winner, I wouldn't mark it as an exception. So I'm leaning towards the first options.

LMK what you think

nathanllww commented 3 years ago

That's a very good point! The steps are useful for transparency at a minimum, and may be useful in resolving the election. Perhaps the ideal behavior is the program, on encountering an unbreakable tie, to halt the counting and just write out the steps (with some sort of different than standard output to indicate what's happened).

I think I'd lean towards keeping, but handling and changing the type of, the exception, simply because we need a way for break_ties to inform run that it should be done, and an exception seems cleaner that returning a boolean up the stack. Even neater (imo) would be to store winner and steps as class variables and handle the exception when writing, but this would only work if write_results calls run, as otherwise the exception will miss write_results. This would change write_results to more like run_and_write_results, and remove any output from run which isn't my favorite, but I think would be only a few LOCs to implement and only one conditional.

I think this second option is the way to go, but I'm not tied to this approach or even the exception handling, so I'll wait to hear your thoughts before implementing.

abugler commented 3 years ago

I was thinking of another solution: Make the unbreakable tie exception a warning instead, so we can complete the function and return no confidence and the steps. I think this is the cleanest way of completing this. We have to pass data up the stack regardless, if we want to save steps, so this might be the best way. I would not like it if where steps are saved changes if we hit an unbreakable tie.

nathanllww commented 3 years ago

Ooooo returning "no confidence" as the winner (or maybe "no confidence (unbreakable tie)" to be especially clear) seems very neat.

abugler commented 3 years ago

I think after we resolve this, I would be comfortable with releasing