STV last round use "Final Round Surplus" rather than inactive

[artoonie]

There are a few options for implementation here.

I chose to treat the final round surplus similar to how we treat inactive ballots, rather than how we treat residual surplus. Residual surplus is treated as votes that sort of "disappear," showing up in the transfer list but not elsewhere. In this case, we want the final round to not make these votes disappear, but rather, land in a non-candidate and non-inactive bucket.

I played around with other ideas but this felt both the most correct and led to the most natural implementation.

Closes #855

[yezr]

Testing

• Confirmed for all the races that had summary.csv changes that the new value in Final Round Transfer exactly matched the difference that no longer went to other inactive buckets

Need to figure out what is up with src/test/resources/network/brightspots/rcv/test_data/test_set_allow_only_one_winner_per_round/test_set_allow_only_one_winner_per_round_expected_summary.json issue during code review

[alyssahursh]

Nice to meet you today, Armin! Sorry to drop so abruptly without a proper goodbye. I had a few extra comments for you on this PR. If you find them useful, great! If not, that's fine too :)

[alyssahursh]

There's something about this code (both the old and the new) that has me scratching my head. What would you think about moving these "key" strings into the StatusForRound enum as, say, jsonKey? Then the logic here might look like this:

private Map<String, BigDecimal> getInactiveJsonMap(RoundTally roundTally) {
    List<StatusForRound> statusesToPrint = new ArrayList<>();
    statusesToPrint.add(INVALIDATED_BY_OVERVOTE);
    statusesToPrint.add(INVALIDATED_BY_SKIPPED_RANKING);
    statusesToPrint.add(EXHAUSTED_CHOICE);
    statusesToPrint.add(INVALIDATED_BY_REPEATED_RANKING);

   if (config.usesSurpluses() && roundTally.getRoundNumber() == numRounds) {
      statusesToPrint.add(FINAL_ROUND_SURPLUS);
    }

    return statusesToPrint.stream()
        .collect(Collectors.toMap(StatusForRound::jsonKey, status -> roundTally.getBallotStatusTally(status)));
}

[artoonie]

Note that we have different strings for different outputs -- the JSON file uses camelCase and the CSV uses Title Plain Text (with spaces). I fear that this difference doesn't belong inside StatusForRound itself (e.g. plaintextString vs camelCaseString), though could see an argument for standardizing the text across the two formats. That would have a lot of downstream side-effects. Thoughts?

[yezr]

Though #856 doesn't require different strings, I could see other future output formats possibly requiring other strings to match to each of the StatusForRound enums. The .json vs .csv formats are similar enough that it seems silly to have both, but other formats could be different enough and this spot seems as good a place as any.

[alyssahursh]

@artoonie You have more experience in this code than I do, so I'll certainly defer to your judgment. I'm just running off intuition here, and my intuition says that all of the different ways of titling these statuses should be held somewhere in common rather than spread around in different files. My concern is that by spreading these different strings around, we're likely to encourage a third or fourth formatting standard to crop up unnecessarily. But my familiarity with this code base is very thin, and I'm not here to block you!

[artoonie]

Got it! I tried it out and I do think it's cleaner as you suggested, though we previously had three different string types not two:

• camel case, brief (e.g. "overvote")
• title case, using "invalidated" (e.g. "Invalidated by Overvote")
• title case, using "inactive" (e.g. "Inactive by Overvote")

The "invalidated" phrase was only used in the logs. @yezr, I don't recall if that was an intentional choice to use it in the audit logs but not in the output CSV. Are you okay using "inactive" for both, or shall I add a third string to the enum in the last commit here?

[yezr]

Using "inactive" for both will work.

[artoonie]

Great, then I'd like a final review on the last commit from Alyssa then I think we're ready to merge all three?

Thanks for the great feedback, Alyssa! This process has been super helpful.

[yezr]

LGTM. All code review comments addressed

• StatusForRound enum now includes single source of truth for which ones are active/inactive and their audit.log text
• logic for whether or not the configuration of a contest will require a Final Round Surplus is pushed up into the ContestConfig
• active/inactive count in Summary fixed to properly account for FINAL_ROUND_TRANSFER

[yezr]

I approved thinking that I had seen all of Alyssa's comments but for some reason some of them were not showing when I was looking at the diff of just the two commits since our code review. The ones I weren't seeing were the last two that were suggestions. I thought the only outstanding PR conversations were simply comments on how the updates fixed exactly what we had talked about in the code review. Since those two are suggestions for updates and not requirements for changes I will leave it to Armin to resolve those

I used the shift + click to select the last two commits but it didn't two of Alyssa's four comments were not visible in that diff screen. Weird...

[alyssahursh]

I know that I was able to approve prior versions, but I somehow can't approve today ("Only users with explicit access to this repository may approve pull requests.") I left two more comments, but I'm not requesting changes. Consider this comment my approval!

[alyssahursh]

I'm going to leave one more comment on this, but I want to be clear that I'm not requesting additional changes in this PR. It's easy to start pulling on a thread and not know where to stop! If I were responsible for this code, I would move this list to a static final field on the class:

private static final List<StatusForRound> STATUSES_TO_PRINT = List.of(
        StatusForRound.INVALIDATED_BY_OVERVOTE,   
        StatusForRound.INVALIDATED_BY_SKIPPED_RANKING, 
        StatusForRound.INVALIDATED_BY_SKIPPED_RANKING, 
        StatusForRound.INVALIDATED_BY_SKIPPED_RANKING);

And then I would reuse the same list starting in line 1027.

List.of() returns an ImmutableList, so you can't conditionally add the FINAL_ROUND_SURPLUS to it. I'll leave a second comment on that section of the code with additional thoughts.

[alyssahursh]

I see that you didn't love the stream suggestion! Since you're using a modifiable map here, you could use the static final immutable list described above like this:

private Map<String, BigDecimal> getInactiveJsonMap(RoundTally roundTally) {
    Map<String, BigDecimal> result = new HashMap<>();
    for (StatusForRound statusToPrint : STATUSES_TO_PRINT) {
        result.put(statusToPrint.getCamelCaseKey(), roundTally.getBallotStatusTally(statusToPrint));
    }

    if (config.usesSurpluses() && roundTally.getRoundNumber() == numRounds) {
        result.put(FINAL_ROUND_SURPLUS.getCamelCaseKey(), roundTally.getBallotStatusTally(FINAL_ROUND_SURPLUS));
    }

    return result;
}

Again, not asking for changes in this PR! Just pointing out what I see :) If it's useful to you, great! Overall, I think the changes that have already been made are a step in the right direction.

[yezr]

I know that I was able to approve prior versions, but I somehow can't approve today ("Only users with explicit access to this repository may approve pull requests.") I left two more comments, but I'm not requesting changes. Consider this comment my approval!

I got to get the approval stuff figured out. Right now anyone can approve and that allows a merge. We don't have a more intermediate "I am done reviewing all the changes" that still allows a later "official" approval for the merge to make its way to dev. For now if you can just comment in the PR like this to flag that a review is finished.

The update to make this list final since then LGTM. Approved