Fail gracefully when there is no precinct information #650

artoonie · 2023-04-04T20:23:44Z

See the comments in #621.

In sum: tabulating by precinct fails because there is no precinct on these files. How should we handle that? Should we set "Precinct Portion" to be equal to "Precinct"? Should we prevent tabulating by precinct altogether?

This will require updating the tests that have precinct blank: https://github.com/BrightSpots/rcv/blob/develop/src/test/resources/network/brightspots/rcv/test_data/dominion_wyoming/dominion_wyoming_contest_1_expected.csv

Note: I will add a test when the design decisions are validated.

artoonie · 2023-04-04T20:24:57Z

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

@@ -275,6 +278,10 @@ private int parseCvrFile(
      String precinctPortion = this.precinctPortions.get(precinctPortionId);
      String ballotTypeId = adjudicatedData.get("BallotTypeId").toString();

+      if (precinct == null && precinctPortion != null) {
+        precinct = precinctPortion;
+      }


Based on the comments on line 261, this is a workaround to have precincts populated for older files that only has precinctPortion

HEdingfield · 2023-05-21T01:49:35Z

@tarheel @chughes297 any thoughts on @artoonie's questions here?

tarheel · 2023-05-28T16:56:43Z

You'll need to talk to @chughes297 et al about this. My guess is that it's not valid to take the "precinct portion" and use it as the precinct, and that the tabulator should just fail gracefully whenever the user tries to tabulate by precinct but the precinct info is incomplete (i.e. not present for every single CVR) or missing entirely.

Btw, I don't think that @chughes297 is seeing his GitHub notifs, so I'd recommend emailing him directly and also seeing if he can adjust his notif settings for the future.

chughes297 · 2023-05-30T13:35:50Z

Agree with @tarheel, I'm hesitant about populating this info because I don't know how precinct portion and precinct differ. I would really rather not produce precinct-level results that are inaccurate because of a mistaken assumption about how those two files relate to one another.

Also I just updated my notification settings so I should get emails just when I get tagged. I was getting emails for every comment on the rctab repo which was not helpful for actually seeing things when I got tagged...

artoonie · 2023-05-31T00:02:36Z

Great, so new approach would be to allow "Tabulate by Precinct," and if some CVRs don't have a precinct, gracefully fail.

chughes297 · 2023-05-31T14:21:40Z

Sounds good to me.

…

On Tue, May 30, 2023 at 8:02 PM Armin Samii ***@***.***> wrote: Great, so new approach would be to allow "Tabulate by Precinct," and if some CVRs don't have a precinct, gracefully fail. — Reply to this email directly, view it on GitHub <#650 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFPLQMDU46MSCBHCQ3G67W3XI2DCPANCNFSM6AAAAAAWTGC76Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Christopher W. Hughes, Esq. (he / him / his) p: 440-773-1224 e: ***@***.*** Set up a meeting with me <https://calendly.com/cwh297/30min?month=2022-03>

…no-dominion-precincts

…re/issue-621_no-dominion-precincts

artoonie · 2023-06-05T18:54:49Z

This is ready for review.

I copied and pasted the dominion_no_precinct_data test. I wonder if we should begin allowing non-standalone tests: allow a relative path from one test to another, because this 86,000 line copy-pasta feels terrible. The new test would do well to just point to the other test's data (or to create a shared_test_data folder?)
This now fails gracefully with an error message about not having precinct data, instead of an NPE.

# Conflicts: # src/main/java/network/brightspots/rcv/BaseCvrReader.java # src/main/java/network/brightspots/rcv/ClearBallotCvrReader.java # src/main/java/network/brightspots/rcv/CommonDataFormatReader.java # src/main/java/network/brightspots/rcv/CsvCvrReader.java # src/main/java/network/brightspots/rcv/DominionCvrReader.java # src/main/java/network/brightspots/rcv/StreamingCvrReader.java # src/main/java/network/brightspots/rcv/Tabulator.java # src/main/java/network/brightspots/rcv/TabulatorSession.java

HEdingfield · 2023-06-09T17:47:55Z

src/test/java/network/brightspots/rcv/TabulatorTests.java

@@ -598,4 +598,11 @@ void genericCsvTest() {
  void noOneMeetsMinimumTest() {
    runTabulationTest("no_one_meets_minimum", TabulationAbortedException.class.toString());
  }
+
+  @Test
+  @DisplayName("gracefully fail when tabulate-by-precinct option set without any precincts in CVR")


@artoonie said:

I copied and pasted the dominion_no_precinct_data test. I wonder if we should begin allowing non-standalone tests: allow a relative path from one test to another, because this 86,000 line copy-pasta feels terrible. The new test would do well to just point to the other test's data (or to create a shared_test_data folder?)

Agreed, and I like this idea. The only potential issue is that it makes the tests non-hermetic (i.e. changing the input file for one could cause others to unintentionally fail). I don't think this is a good reason not to do this though.

Could you please create a follow-up issue for this (linking to this comment), and maybe we can sneak it into v1.4.0 if it seems relatively low effort?

Yep! #692

I think we can avoid the first issue by only sharing CVRs in a common directory, so it's clear that those CVRs are shared.

HEdingfield · 2023-06-09T17:58:24Z

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

+        if (precinctIds.size() == 0) {
+          Logger.severe("\"Tabulate by Precinct\" enabled, but CVRs don't list precincts.");
+          throw new TabulationAbortedException(false);
+        }


Wouldn't it be more efficient to put this in the constructor just before initPrecinctRoundTallies() is called?

Yes but the constructor doesn't currently throw a TabulationAbortedException (nor do I think it should?), so it felt natural here since it's also where we check for other precinct-level issues.

Let me know if you think that's a good enough reason, or if there's another natural spot for this check to occur.

I don't see any reason not to have the constructor throw a TabulationAbortedException (in fact, it's already caught in the place where the constructor is called).

I'd personally move it there because I like cleaner logs and not going forward with other calcs to no end, but it's not a super big deal. Will go ahead and approve and let you make the call either way. 👍

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

HEdingfield · 2023-06-09T18:32:32Z

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

+        if (precinctIds.size() == 0) {
+          Logger.severe("\"Tabulate by Precinct\" enabled, but CVRs don't list precincts.");
+          throw new TabulationAbortedException(false);
+        }


I don't see any reason not to have the constructor throw a TabulationAbortedException (in fact, it's already caught in the place where the constructor is called).

I'd personally move it there because I like cleaner logs and not going forward with other calcs to no end, but it's not a super big deal. Will go ahead and approve and let you make the call either way. 👍

Populate older dominion files with precinct information

53ada05

artoonie added v2.0 requirement WIP labels Apr 4, 2023

artoonie commented Apr 4, 2023

View reviewed changes

artoonie linked an issue Apr 4, 2023 that may be closed by this pull request

NPE when tabulating by precinct #621

Closed

artoonie mentioned this pull request Apr 4, 2023

NPE when tabulating by precinct #621

Closed

more error handling

507f3e0

artoonie force-pushed the feature/issue-621_no-dominion-precincts branch from 7918366 to 507f3e0 Compare April 4, 2023 20:39

artoonie added help wanted question labels Apr 6, 2023

HEdingfield requested review from tarheel and chughes297 May 21, 2023 01:49

artoonie added 3 commits June 5, 2023 13:40

Generate precinct ID where it makes sense and reduce clutter

9b6387d

Merge remote-tracking branch 'origin/develop' into feature/issue-621_…

23b3b58

…no-dominion-precincts

Merge remote-tracking branch 'origin/cleanup-precinct-ids' into featu…

3c9cf40

…re/issue-621_no-dominion-precincts

artoonie changed the base branch from develop to cleanup-precinct-ids June 5, 2023 18:46

artoonie removed help wanted question WIP labels Jun 5, 2023

Gracefully fail if "Tabulate by Precinct" enabled w/o precincts

296d1f6

artoonie force-pushed the feature/issue-621_no-dominion-precincts branch from 3d5e5a3 to 296d1f6 Compare June 5, 2023 18:48

artoonie marked this pull request as ready for review June 5, 2023 18:53

Base automatically changed from cleanup-precinct-ids to develop June 9, 2023 17:26

Merge fix.

767e72a

HEdingfield reviewed Jun 9, 2023

View reviewed changes

artoonie mentioned this pull request Jun 9, 2023

Create a common test suite for shared CVRs #692

Closed

artoonie changed the title ~~Populate older dominion files with precinct information~~ Fail gracefully when there is no precinct information Jun 9, 2023

HEdingfield approved these changes Jun 9, 2023

View reviewed changes

move precinct check to constructor

86f9762

artoonie merged commit a8308e8 into develop Jun 9, 2023
1 check passed

artoonie deleted the feature/issue-621_no-dominion-precincts branch June 9, 2023 19:09

artoonie mentioned this pull request Jun 16, 2023

consolidate all duplicate CVRs #708

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail gracefully when there is no precinct information #650

Fail gracefully when there is no precinct information #650

artoonie commented Apr 4, 2023 •

edited

artoonie Apr 4, 2023

HEdingfield commented May 21, 2023

tarheel commented May 28, 2023

chughes297 commented May 30, 2023

artoonie commented May 31, 2023

chughes297 commented May 31, 2023 via email

artoonie commented Jun 5, 2023

HEdingfield Jun 9, 2023

artoonie Jun 9, 2023

HEdingfield Jun 9, 2023

artoonie Jun 9, 2023

HEdingfield Jun 9, 2023

HEdingfield Jun 9, 2023

Fail gracefully when there is no precinct information #650

Fail gracefully when there is no precinct information #650

Conversation

artoonie commented Apr 4, 2023 • edited

artoonie Apr 4, 2023

Choose a reason for hiding this comment

HEdingfield commented May 21, 2023

tarheel commented May 28, 2023

chughes297 commented May 30, 2023

artoonie commented May 31, 2023

chughes297 commented May 31, 2023 via email

artoonie commented Jun 5, 2023

HEdingfield Jun 9, 2023

Choose a reason for hiding this comment

artoonie Jun 9, 2023

Choose a reason for hiding this comment

HEdingfield Jun 9, 2023

Choose a reason for hiding this comment

artoonie Jun 9, 2023

Choose a reason for hiding this comment

HEdingfield Jun 9, 2023

Choose a reason for hiding this comment

HEdingfield Jun 9, 2023

Choose a reason for hiding this comment

artoonie commented Apr 4, 2023 •

edited