UI and config updates for Stop Tabulation Early #636

artoonie · 2023-03-14T20:23:21Z

No logic changes to tabulation yet, just UI + configuration changes.

Being my first Pull Request with a substantial feature, I'm asking for a more detailed review than I'd likely need in the future. I've called out specific decisions throughout the code that I'd like feedback on.

If all looks good, I believe this is ready to merge.

tarheel

I can look more closely at your code later, but some initial responses:

Style. Is there an automatic linter or styleguide?

Yes, please use Checkstyle. (The config is under config/checkstyle. Not sure about other IDEs, but it integrates well with IntelliJ.)

Config. I followed the paradigm of a specific keyword to specify "max" in the config, which translates to checkboxes in the UI. Is that reasonable?

I would prefer to match the UI we use for other optional fields: we just have a single text box, and the user leaves it blank (which corresponds to null internally) if they don't want to specify it.

Bumping version to update config -- any unforeseen issues there?

I don't believe so...? Hylton might remember something.

Btw, unrelated to your PR, but while looking at your screenshot I noticed that we have an asterisk next to "Random seed." This is inaccurate: that field is only required if the config selects one of the random tie-break modes. You might as well fix it while you're here.

src/main/java/network/brightspots/rcv/Main.java

artoonie · 2023-03-14T21:14:41Z

Thank you -- that all makes sense. Updated the main post with the new UI.

What do you use to edit the .fxml files? I'm using SceneBuilder but it changes the formatting and creates a too-large diff. For these commits I'm manually editing the files.

PS I prefer force-pushes + clean commit history, but force pushing dirties Pull Requests. LMK if you favor additional commits over force-pushes, or if you prefer me to push less frequently.

src/main/java/network/brightspots/rcv/ContestConfig.java

src/main/resources/network/brightspots/rcv/hints_winning_rules.txt

.../brightspots/rcv/test_data/stop_tabulation_early_test/stop_tabulation_early_test_config.json

artoonie · 2023-03-15T03:04:08Z

...ts/rcv/test_data/stop_tabulation_early_test/stop_tabulation_early_test_expected_summary.json

+    },
+    "tallyResults" : [ {
+      "eliminated" : "George Gervin",
+      "transfers" : { }


Reading the legislation, having no transfers on this round seems correct. Let me know if anybody disagrees. (It doesn't make sense to add in last-round transfers to me, but I did want to flag just in case.)

Not sure how to answer this. @tarheel?

I'm inclined to agree, since transfers would only be relevant if you were going to then proceed and show the tallies for the next round. But @chughes297 and @rrojas350 may have an opinion.

I agree, you'd only show the results of the surplus transfer in the next round.

HEdingfield

Great first PR!

src/main/java/network/brightspots/rcv/Main.java

src/main/java/network/brightspots/rcv/ContestConfig.java

HEdingfield · 2023-03-18T07:55:24Z

...ts/rcv/test_data/stop_tabulation_early_test/stop_tabulation_early_test_expected_summary.json

+    },
+    "tallyResults" : [ {
+      "eliminated" : "George Gervin",
+      "transfers" : { }


Not sure how to answer this. @tarheel?

.../brightspots/rcv/test_data/stop_tabulation_early_test/stop_tabulation_early_test_config.json

src/main/resources/network/brightspots/rcv/hints_winning_rules.txt

src/main/java/network/brightspots/rcv/GuiConfigController.java

src/main/java/network/brightspots/rcv/ContestConfig.java

HEdingfield · 2023-03-18T08:35:47Z

src/main/java/network/brightspots/rcv/ContestConfig.java

@@ -947,6 +962,12 @@ boolean isContinueUntilTwoCandidatesRemainEnabled() {
    return rawConfig.rules.continueUntilTwoCandidatesRemain;
  }

+  Integer getStopTabulationAtRound() {


Nit: getStopTabulationEarlyOnRound

This still says "at" instead of "on", so recommend changing it to be consistent with everything else.

src/main/java/network/brightspots/rcv/ContestConfig.java

HEdingfield · 2023-03-18T08:42:53Z

Btw, unrelated to your PR, but while looking at your screenshot I noticed that we have an asterisk next to "Random seed." This is inaccurate: that field is only required if the config selects one of the random tie-break modes. You might as well fix it while you're here.

@tarheel I believe this is correct actually, and would argue we should leave it as-is. We don't dynamically generate these asterisks, so it looks like they're used whenever something is required in certain cases as well.

What do you use to edit the .fxml files? I'm using SceneBuilder but it changes the formatting and creates a too-large diff. For these commits I'm manually editing the files.

Yeah, this sucks. I sometimes use SceneBuilder in IntelliJ to get a basic wireframe in place (or if I'm confused about how to add a particular element), but then I'll go back into the code, copy the main chunk it added, revert the file, then paste it in and manually tweak it until it looks good.

PS I prefer force-pushes + clean commit history, but force pushing dirties Pull Requests. LMK if you favor additional commits over force-pushes, or if you prefer me to push less frequently.

We do squash commits on our PRs, so I think that means either way would be fine?

artoonie · 2023-03-18T17:49:26Z

Thanks for the review! All changes made.

artoonie · 2023-03-18T17:51:57Z

Btw, unrelated to your PR, but while looking at your screenshot I noticed that we have an asterisk next to "Random seed." This is inaccurate: that field is only required if the config selects one of the random tie-break modes. You might as well fix it while you're here.

@tarheel I believe this is correct actually, and would argue we should leave it as-is. We don't dynamically generate these asterisks, so it looks like they're used whenever something is required in certain cases as well.

FYI this was already merged: #637
LMK if you'd like me to revert that

HEdingfield

Great work on this! Just one more nit I suggest fixing and then I think you're good to merge.

HEdingfield · 2023-03-18T20:01:29Z

Btw, unrelated to your PR, but while looking at your screenshot I noticed that we have an asterisk next to "Random seed." This is inaccurate: that field is only required if the config selects one of the random tie-break modes. You might as well fix it while you're here.

@tarheel I believe this is correct actually, and would argue we should leave it as-is. We don't dynamically generate these asterisks, so it looks like they're used whenever something is required in certain cases as well.

FYI this was already merged: #637 LMK if you'd like me to revert that

Yes, please revert, per #637 (comment).

tarheel

I added a few inline thoughts.

One bigger question (and apologies for not thinking of this earlier): does this actually satisfy the requirement from the legislation?

"The voting system must allow the user to decide whether to pause the tabulation session after each round or to continue until a winner is determined or a manual tie break for elimination is required."

To me, that sounds like we actually want a boolean setting for this -- and when it's enabled, the tabulator stops after every round and lets the user decide whether to proceed.

cc @chughes297 and @rrojas350 for this question also.

tarheel · 2023-04-03T00:34:17Z

src/main/java/network/brightspots/rcv/ContestConfig.java

+    if (fieldOutOfRangeOrNotInteger(
+        getStopTabulationEarlyOnRoundRaw(),
+        "stopEarlyOnRound",
+        MIN_NUMBER_OF_ROUNDS,


Shouldn't this be 1? If they say to stop after 0 rounds, that's the same as just not running the tabulation at all, isn't it?

Yes, I figured that might be useful in some cases -- you'd still get the output files with zero rounds (similar to running a Zero Report?). I can change it to 1 though if you prefer.

tarheel · 2023-04-03T00:35:39Z

src/main/java/network/brightspots/rcv/Tabulator.java

@@ -412,7 +412,9 @@ private boolean shouldContinueTabulating() {
    int numEliminatedCandidates = candidateToRoundEliminated.size();
    int numWinnersDeclared = winnerToRound.size();
    // apply config setting if specified


let's just delete this comment; it's ambiguous and doesn't add anything anyway

Can do. (Note: It wasn't part of this PR.)

tarheel · 2023-04-03T00:36:37Z

src/main/java/network/brightspots/rcv/ContestConfig.java

@@ -789,6 +799,10 @@ private String getNumberOfWinnersRaw() {
    return rawConfig.rules.numberOfWinners;
  }

+  private String getStopTabulationEarlyOnRoundRaw() {


If I'm not mistaken, this setting means that we'd stop after the specified number of rounds, right? If so, let's call it that instead to make it more clear. Stop on a round makes it sound like maybe we're stopping at the beginning of that round.

tarheel · 2023-04-03T00:43:10Z

...ts/rcv/test_data/stop_tabulation_early_test/stop_tabulation_early_test_expected_summary.json

+    },
+    "tallyResults" : [ {
+      "eliminated" : "George Gervin",
+      "transfers" : { }


I'm inclined to agree, since transfers would only be relevant if you were going to then proceed and show the tallies for the next round. But @chughes297 and @rrojas350 may have an opinion.

artoonie · 2023-04-03T02:38:08Z

#648 addresses all the comments here

rrojas350 · 2023-04-03T17:19:02Z

I added a few inline thoughts.

One bigger question (and apologies for not thinking of this earlier): does this actually satisfy the requirement from the legislation?

"The voting system must allow the user to decide whether to pause the tabulation session after each round or to continue until a winner is determined or a manual tie break for elimination is required."

To me, that sounds like we actually want a boolean setting for this -- and when it's enabled, the tabulator stops after every round and lets the user decide whether to proceed.

cc @chughes297 and @rrojas350 for this question also.

Pause gives me some pause. I hadn't thought of it that way until you mentioned it Louis and I believe a boolean setting would be better here. However, so long as we can stop after a round and produce results, I think we still fall within the law here, even if the admin would have to rerun with the tabulation.

chughes297 · 2023-04-03T17:25:03Z

Sorry to have missed earlier conversations here, but I don't see a use case for telling tabulation to stop at a specific round of counting. Like Louis, I read the law as requiring software that can be told to stop after each round of counting, not to stop at an arbitrary round of counting. When I read the requirement I think of something like what Alaska did for their Nov 2022 results livestream. That seems to require far more than just what is being considered in this issue though.

artoonie · 2023-04-03T18:33:09Z

Let me know if this needs to be reworked. For now, what Rene said seems to meet the requirements:

Election official enters "1" to stop after Round 1
Does
Repeat with "2" to stop after Round 2
etc

tarheel · 2023-04-03T19:16:16Z

I think the other way would be preferable for sure, but if folks think that the implementation here is technically sufficient, maybe we make the revised approach a P1 or P2 for this release.

rrojas350 · 2023-04-04T17:55:33Z

I reached out to the CO SOS's office to get their opinion on this item but for now we can hold off on making any further changes. We may revisit this item in v1.5, depending on their feedback and requirements but as far as 1.4, we're good.

tarheel reviewed Mar 14, 2023

View reviewed changes

src/main/java/network/brightspots/rcv/Main.java Outdated Show resolved Hide resolved

UI and config updates for Stop Tabulation Early

35f2a5c

artoonie force-pushed the feature/issue-630_stop-tabulation-early branch from f51e0cb to 35f2a5c Compare March 14, 2023 20:43

artoonie force-pushed the feature/issue-630_stop-tabulation-early branch 2 times, most recently from b58ce76 to 59e8eb2 Compare March 14, 2023 21:30

Simplify UI based on comments; adjust version number

b2e92b0

artoonie force-pushed the feature/issue-630_stop-tabulation-early branch from 59e8eb2 to b2e92b0 Compare March 14, 2023 21:32

artoonie commented Mar 14, 2023

View reviewed changes

src/main/java/network/brightspots/rcv/ContestConfig.java Outdated Show resolved Hide resolved

src/main/resources/network/brightspots/rcv/hints_winning_rules.txt Outdated Show resolved Hide resolved

artoonie added the WIP label Mar 15, 2023

Stop tabulation early logic + test

d7e79c7

artoonie force-pushed the feature/issue-630_stop-tabulation-early branch from 34a0c85 to d7e79c7 Compare March 15, 2023 02:59

artoonie added v2.0 requirement and removed WIP labels Mar 15, 2023

artoonie linked an issue Mar 15, 2023 that may be closed by this pull request

Stop tabulation at a specified round #630

Closed

artoonie commented Mar 15, 2023

View reviewed changes

HEdingfield self-requested a review March 18, 2023 07:50

HEdingfield requested changes Mar 18, 2023

View reviewed changes

Code Review Changes

81f4b74

artoonie force-pushed the feature/issue-630_stop-tabulation-early branch from 98444aa to 81f4b74 Compare March 18, 2023 18:06

HEdingfield mentioned this pull request Mar 18, 2023

Re-save all test config files #642

Open

HEdingfield approved these changes Mar 18, 2023

View reviewed changes

HEdingfield mentioned this pull request Mar 18, 2023

Random Seed is optional; mark it as such in the UI #637

Merged

PR Comment

e3b30ac

artoonie merged commit 3830067 into develop Mar 19, 2023

artoonie deleted the feature/issue-630_stop-tabulation-early branch March 19, 2023 02:36

tarheel reviewed Apr 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI and config updates for Stop Tabulation Early #636

UI and config updates for Stop Tabulation Early #636

artoonie commented Mar 14, 2023 •

edited

tarheel left a comment

artoonie commented Mar 14, 2023 •

edited

artoonie Mar 15, 2023

HEdingfield Mar 18, 2023

tarheel Apr 3, 2023

chughes297 Apr 3, 2023

HEdingfield left a comment

HEdingfield Mar 18, 2023

HEdingfield Mar 18, 2023

HEdingfield Mar 18, 2023

HEdingfield commented Mar 18, 2023

artoonie commented Mar 18, 2023

artoonie commented Mar 18, 2023

HEdingfield left a comment

HEdingfield commented Mar 18, 2023

tarheel left a comment

tarheel Apr 3, 2023

artoonie Apr 3, 2023

tarheel Apr 3, 2023

artoonie Apr 3, 2023

tarheel Apr 3, 2023

tarheel Apr 3, 2023

artoonie commented Apr 3, 2023

rrojas350 commented Apr 3, 2023

chughes297 commented Apr 3, 2023

artoonie commented Apr 3, 2023

tarheel commented Apr 3, 2023

rrojas350 commented Apr 4, 2023

UI and config updates for Stop Tabulation Early #636

UI and config updates for Stop Tabulation Early #636

Conversation

artoonie commented Mar 14, 2023 • edited

tarheel left a comment

Choose a reason for hiding this comment

artoonie commented Mar 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HEdingfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HEdingfield commented Mar 18, 2023

artoonie commented Mar 18, 2023

artoonie commented Mar 18, 2023

HEdingfield left a comment

Choose a reason for hiding this comment

HEdingfield commented Mar 18, 2023

tarheel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artoonie commented Apr 3, 2023

rrojas350 commented Apr 3, 2023

chughes297 commented Apr 3, 2023

artoonie commented Apr 3, 2023

tarheel commented Apr 3, 2023

rrojas350 commented Apr 4, 2023

artoonie commented Mar 14, 2023 •

edited

artoonie commented Mar 14, 2023 •

edited