Aliases as a replacement for candidate codes #639

artoonie · 2023-03-15T21:38:28Z

Implementation Details

An alias is just a general case of the old concept of a "Code". Candidates now have three fields: name (aka display name, or canonical name), code (only used in the output CSVs - see Question below), and aliases (generalized codes).

In the tabulation logic, all names encountered are translated from whatever form they're in to the canonical name. All logic is now done on the canonical name, not on the code or any alias.

I have designed this so that the concept of code can be removed entirely in time, but have not done so yet.

Question: removing the concept of "Code" entirely

The current logic on develop is for output CSVs to print the Candidate code rather than the name. In all other cases, code is treated exactly like an alias.

To ensure 1-to-1 consistency with develop, I have opted to leave both "Code" and "Aliases" internally, even though the UI now only shows "Aliases." Removing the concept of "Code" will be trivial, and would clean up the code, but would result in test files differing. I'm not sure if there are other consequences.

Question: Do you agree with the current implementation (keeping "Code" internally so there are no changes to any output files), or do you prefer a cleaner implementation (done by removing "Code" and updating all tests that used "Code").
Answer: In a separate pull request, we'll get rid of code

Screenshot: an old Dominion config.

Notice the "Code" is successfully shown as an "Alias"

Screenshot: multiple aliases

Semicolon-separated, which will be easy to translate back and forth into a list when the Table becomes editable.

Test details (on both JSON and XLSX tests)

Ensures the non-alias name is the name used in the JSON summary output
Ensures that both aliases and names are treated identically otherwise
Ensures unused aliases don't cause issues
Ensures unused names don't cause issues (i.e. the "Code" use case, where only the alias is used in the CVR and not the name)

Still TODO: - UI (test, remove Candidate Code) - Serialization (test) - Decide whether we need to keep Candidate Codes for the output files or not

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

HEdingfield · 2023-03-18T20:32:53Z

Question: Do you agree with the current implementation (keeping "Code" internally so there are no changes to any output files), or do you prefer a cleaner implementation (done by removing "Code" and updating all tests that used "Code").

I personally don't see a big benefit to keeping the "Code" field around at all. We're guaranteed that names will be unique, so why not just display that in the output? Let's confirm with @rrojas350 though.

HEdingfield

Haven't reviewed everything yet, but here are some comments to get your started!

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

src/main/resources/network/brightspots/rcv/GuiConfigLayout.fxml

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

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

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

HEdingfield · 2023-03-18T20:54:24Z

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

@@ -315,23 +315,25 @@ private int parseCvrFile(
            }
            Integer candidateId = (Integer) rankingMap.get("CandidateId");
            String candidateCode = candidateId.toString();
-            Set<String> candidates = contestIdToCandidateCodes.get(contestId);
-            if (!candidates.contains(candidateCode)) {
+            String candidateName = config.getNameFor(candidateCode);


Are you still calling this candidateCode on purpose?

Yes, Dominion uses a code which we translate here to the display name. In other files I use candidateName not candidateDisplayName, but I'm happy to rename the variable here to clarify (or add a comment).

How about we just call this candidateId or dominionCandidateId then, since that's what it's apparently called in the Dominion format? Just trying to clear the way for #663 so there's no confusion around this there.

Not sure if you missed this comment, but I think it'd be nice to take care of this still.

Looks like your comment didn't submit before :) both your comments show as appearing 5 days ago. Yes, I can rename this variable dominionCandidateId

Oh weird, sorry about that. Looks like we got our wires crossed a bit on this, so I just added a commit to clear it up. Please give it a look over to verify that it makes sense.

HEdingfield · 2023-03-20T00:16:53Z

artoonie force-pushed the feature/issue-591_aliases branch from cb1080d to f0cec41
3 days ago
@artoonie
Make CastVoteRecord use 50% less memory

Not sure why this was done, as that code hasn't been reviewed yet. Could you please keep it out of this branch?

artoonie · 2023-03-20T00:42:01Z

Ugh sorry, let me do some git cleanup. I'm using the github desktop client for the first time and must have made some mistake.

Update: Fixed. Sincere apologies. Will avoid the GitHub desktop client for now since it seems to do a lot behind the scenes that I don’t understand.

ggilber1 · 2023-03-20T19:20:48Z

Can someone explain to me in English (and without acronyms) what is being done here? Thanks.
I'll assign this to @rrojas350 since you are probably at at least an intermediate level of technical comprehension.

artoonie · 2023-03-20T19:31:36Z

Can someone explain to me in English (and without acronyms) what is being done here? Thanks. I'll assign this to @rrojas350 since you are probably at at least an intermediate level of technical comprehension.

Check out the Issue linked to this pull request here: #591
In sum: there are often cases where a candidate has multiple names across different CVRs -- for example, write-ins, or multi-vendor tabulations. This provides support for those cases: a candidate can now have aliases, and we will look for both their name and their aliases in CVRs.

ggilber1 · 2023-03-20T19:53:08Z

Got the overall idea. I am unclear about some of the details, eg., how this is being handled in regard to vendor systems which use candidate codes rather than candidate names in their CVRs. I very much favor pulling the candidate names into the config, which it appears you are doing. Every election administrator wants to be able to look one place to ck names on a ballot vs. candidate lists. Also need to understand how this issue relates to the issue of auto-populating candidate names in the config tile. Thanks, Armin. George Gilbert ***@***.*** 336-906-0047

…

On Mon, Mar 20, 2023 at 3:31 PM Armin Samii ***@***.***> wrote: Can someone explain to me in English (and without acronyms) what is being done here? Thanks. I'll assign this to @rrojas350 <https://github.com/rrojas350> since you are probably at at least an intermediate level of technical comprehension. Check out the Issue linked to this pull request here: #591 <#591> In sum: there are often cases where a candidate has multiple names across different CVRs -- for example, write-ins, or multi-vendor tabulations. This provides support for those cases: a candidate can now have aliases, and we will look for both their name and their aliases in CVRs. — Reply to this email directly, view it on GitHub <#639 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A6MJAYUILWLP5MOGZ55SVMLW5CWCJANCNFSM6AAAAAAV4M5F2Y> . You are receiving this because you commented.Message ID: ***@***.***>

artoonie · 2023-03-20T20:03:00Z

@ggilber1 It now treats Candidate Codes and Aliases in the same manner. An election administrator using ballots with candidate codes would enter the Code in the new textbox. An election administrator using aliases would enter the aliases in that same textbox. They can also mix and match codes and aliases if they have a multi-vendor tabulation that needs both codes and aliases.

This probably does not affect the auto-populating of candidate names in any way, though I haven't given that enough thought to be confident about it.

rrojas350 · 2023-03-20T20:23:49Z

@HEdingfield If I'm understanding correctly, I'm ok getting rid of candidate code in the output.

Judging from @artoonie's reply to @ggilber1, it sounds like candidate codes on the front end shouldn't cause any issues when trying to tabulate Hart and Dominion CVRs, which is the only concern I have regarding candidate codes.

ggilber1 · 2023-03-20T20:31:13Z

Thanks again, Armin. I need to walk through it step by step (in my head at this time) and compare to what we were doing. I think I like what you guys have done but need to be able to describe it simply to an election programmer. I'm working with Rene on #591, Instructions and Documentation, and am trying to get back up to speed on RCTab. George Gilbert ***@***.*** 336-906-0047

…

On Mon, Mar 20, 2023 at 4:03 PM Armin Samii ***@***.***> wrote: @ggilber1 <https://github.com/ggilber1> It now treats Candidate Codes and Aliases in the same manner. An election administrator using ballots with candidate codes would enter the Code in the new textbox. An election administrator using aliases would enter the aliases in that same textbox. They can also mix and match codes and aliases if they have a multi-vendor tabulation that needs both codes and aliases. This probably does not affect the auto-populating of candidate names in any way, though I haven't given that enough thought to be confident about it. — Reply to this email directly, view it on GitHub <#639 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A6MJAYWXCAZYKDQK4AMWBUTW5CZYBANCNFSM6AAAAAAV4M5F2Y> . You are receiving this because you were mentioned.Message ID: ***@***.***>

andyanderson · 2023-03-20T21:41:07Z

Also need to understand how this issue relates to the issue of auto-populating candidate names in the config tile.
…

Currently the software ignores CVRs for candidates who aren’t pre-populated in the software, i.e. write-ins. Auto-populating them eliminates the need for an administrator to enter them by hand. Then all they need to do is correlate them in the alias table if there are multiple variants (likely).

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

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

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

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

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

HEdingfield · 2023-04-28T06:45:22Z

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

+     *
+     * @return a potentially-empty string
+     */
+    public String getSemicolonSeparatedAliases() {


The way I kind of had it working before was double-clicking the cell would allow you to edit it and type right in there, then you had to press enter to commit it. Definitely open to other designs though. It'd probably actually be really cool if we had a modal pop up with the full text area to edit, but that would take some work.

Maybe we can go with a happy medium, like a pipe | since that's at least typeable on a standard keyboard and my gut tells me it's less likely to appear in an alias than a semicolon??

Alternatively, a no-semicolon check would work too. I'm happy either way. @chughes297 might have useful thoughts on this.

src/main/resources/network/brightspots/rcv/GuiConfigLayout.fxml

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

HEdingfield

Very close now! I think we'll all breathe a sigh of relief once this one is submitted.

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

...rk/brightspots/rcv/test_data/clear_ballot_kansas_primary/clear_ballot_kansas_primary.cvr.csv

...rightspots/rcv/test_data/clear_ballot_kansas_primary/clear_ballot_kansas_primary_config.json

src/test/resources/network/brightspots/rcv/test_data/aliases-xlsx/aliases-xlsx_config.json

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

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

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

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

HEdingfield · 2023-04-30T18:51:09Z

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

+     *
+     * @return a potentially-empty string
+     */
+    public String getSemicolonSeparatedAliases() {


Sounds good. I do like the semicolons from a UX perspective, because non-technical people are used to seeing that.

I propose this: we leave the semicolons as delimiters in the column, but we plan to do a pop-up modal with a text area for editing. Then, maybe in the backend, it always makes sure to submit the newline version for saving, even though it's displaying the semicolons to users in the column. Then we can just split on newlines when saving it.

Does that make sense? I believe this could open up a path for us to allow for semicolons in aliases.

Noting this discussion in #590.

artoonie · 2023-04-30T20:59:45Z

All comments resolved. Thanks for catching the accidentally-updated test -- perhaps we should stop the UI from forcing you to re-save a config if the version is "TEST"? It's a pain to constantly revert those. I can make a ticket for that if you think it'd help y'all as well.

HEdingfield · 2023-05-01T00:24:34Z

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

@@ -315,23 +315,25 @@ private int parseCvrFile(
            }
            Integer candidateId = (Integer) rankingMap.get("CandidateId");
            String candidateCode = candidateId.toString();
-            Set<String> candidates = contestIdToCandidateCodes.get(contestId);
-            if (!candidates.contains(candidateCode)) {
+            String candidateName = config.getNameFor(candidateCode);


Not sure if you missed this comment, but I think it'd be nice to take care of this still.

...st/resources/network/brightspots/rcv/test_data/aliases_cdf_json/aliases_cdf_json_config.json

HEdingfield · 2023-05-01T00:27:54Z

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

+     *
+     * @return a potentially-empty string
+     */
+    public String getSemicolonSeparatedAliases() {


Yeah, that sounds perfect. Does that require code changes to this PR, or is it purely something you'd want to handle in future work around this?

HEdingfield · 2023-05-03T05:53:35Z

Happy to add a commit that takes care of this last bit if you'd like (also happy to wait if you'd rather do it yourself), just let me know.

tarheel

Sorry for taking forever to comment on this. It makes sense to me! And I definitely agree that we should dump candidate codes.

tarheel · 2023-05-03T06:16:43Z

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

+
+    /**
+     * A stream of all aliases (which is guaranteed to be unique) and the candidate name
+     * (which is not guaranteed to be unique, i.e. it may exist in the list twice)


why might the name be present twice?

It's valid for a candidate name to be equal to a candidate code, and therefore for a candidate name to be equal to an alias.

tarheel · 2023-05-03T06:18:38Z

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

+    if (candidateNames == null) {
+      candidateNames = new HashSet<>();
+    }


nit: why not just initialize this when you declare it?

Nothing else is initialized when declared

This was a request from @HEdingfield to add this:

#639 (comment)
#639 (comment)

(Looks like the direct links may not work...but, see comments above on ContestConfig.java

I think either way would probably work fine. Also agree with @artoonie that we should make a pass on all this at some point to make it consistent.

tarheel · 2023-05-03T06:19:00Z

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

+    candidateAliasesToNameMap = new HashMap<>();
+    candidateAliasesToCodeMap = new HashMap<>();
+    candidateNames = new HashSet<>();


similarly, why not just initialize these when you declare them?

Just following the existing model, a la validateCandidates. I don't mind changing these to init when declared, but perhaps we should have a larger movement to do that consistently across this and other files?

tarheel · 2023-05-03T06:26:48Z

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

+     *
+     * @return a potentially-empty string
+     */
+    public String getSemicolonSeparatedAliases() {


You're saying that it'll just use standard JSON list format, right?

… logic around `dominionCandidateId`.

HEdingfield

Ok, I think this is all set! I added a small commit that fixes a couple small bugs and cleans stuff up more, so will let you go ahead and look that over before you merge it. Great work on this!

HEdingfield · 2023-05-07T23:42:58Z

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

@@ -315,23 +315,25 @@ private int parseCvrFile(
            }
            Integer candidateId = (Integer) rankingMap.get("CandidateId");
            String candidateCode = candidateId.toString();
-            Set<String> candidates = contestIdToCandidateCodes.get(contestId);
-            if (!candidates.contains(candidateCode)) {
+            String candidateName = config.getNameFor(candidateCode);


Oh weird, sorry about that. Looks like we got our wires crossed a bit on this, so I just added a commit to clear it up. Please give it a look over to verify that it makes sense.

HEdingfield · 2023-05-07T23:48:36Z

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

+    if (candidateNames == null) {
+      candidateNames = new HashSet<>();
+    }


I think either way would probably work fine. Also agree with @artoonie that we should make a pass on all this at some point to make it consistent.

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

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

Alias for Candidates #591

Closed

artoonie force-pushed the feature/issue-591_aliases branch from 7b53d73 to 6fd568c Compare March 16, 2023 00:09

WIP: Aliases instead of (in addition to) candidate codes

06d81cc

Still TODO: - UI (test, remove Candidate Code) - Serialization (test) - Decide whether we need to keep Candidate Codes for the output files or not

artoonie force-pushed the feature/issue-591_aliases branch from 6fd568c to 06d81cc Compare March 16, 2023 00:14

UI + config serialization

8f759f3

artoonie force-pushed the feature/issue-591_aliases branch from 2c8e5f7 to 8f759f3 Compare March 16, 2023 01:57

artoonie removed the WIP label Mar 16, 2023

artoonie commented Mar 16, 2023

View reviewed changes

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

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

artoonie mentioned this pull request Mar 16, 2023

Alias for Candidates #591

Closed

artoonie force-pushed the feature/issue-591_aliases branch 3 times, most recently from 92edaee to cb1080d Compare March 16, 2023 03:19

Clean up fxml diff; misc bugfixes

f0cec41

artoonie force-pushed the feature/issue-591_aliases branch from cb1080d to f0cec41 Compare March 16, 2023 03:22

HEdingfield requested review from HEdingfield and tarheel March 18, 2023 07:37

HEdingfield requested changes Mar 18, 2023

View reviewed changes

artoonie force-pushed the feature/issue-591_aliases branch from d8d35a8 to 0fba220 Compare March 19, 2023 20:29

Code review

fa76ba5

artoonie force-pushed the feature/issue-591_aliases branch from 0fba220 to fa76ba5 Compare March 20, 2023 01:01

artoonie changed the title ~~WIP: Aliases instead of (in addition to) candidate codes~~ Aliases instead of (in addition to) candidate codes Mar 20, 2023

This was referenced Mar 22, 2023

Basic CDF validation #644

Merged

Handle multiple Candidate IDs in CDF #645

Closed

add test for aliases

467dbcf

HEdingfield reviewed Apr 28, 2023

View reviewed changes

artoonie mentioned this pull request Apr 28, 2023

Remove notion of "Candidate Codes" altogether #663

Closed

address PR comments

3949e57

artoonie changed the title ~~Aliases instead of (in addition to) candidate codes~~ Aliases as a replacement for candidate codes Apr 29, 2023

HEdingfield reviewed Apr 30, 2023

View reviewed changes

add additional test with XLSX

242b9b7

artoonie force-pushed the feature/issue-591_aliases branch from 2389ed1 to 242b9b7 Compare April 30, 2023 20:36

Rename tests

2c9d81c

HEdingfield reviewed May 1, 2023

View reviewed changes

tarheel reviewed May 3, 2023

View reviewed changes

artoonie and others added 2 commits May 5, 2023 17:18

Rename candidateName to dominionCandidateId

dbd7bcf

Fixed bug where contestId was being rendered as an int; clarified…

9835239

… logic around `dominionCandidateId`.

HEdingfield approved these changes May 7, 2023

View reviewed changes

artoonie merged commit b1367a2 into develop May 8, 2023

artoonie deleted the feature/issue-591_aliases branch May 8, 2023 17:13

HEdingfield mentioned this pull request May 14, 2023

Fixes DominionCvrReader bad merge #665

Merged

HEdingfield mentioned this pull request Jun 12, 2023

deprecate the concept of "codes" (almost) entirely #690

Merged

yezr mentioned this pull request Dec 15, 2023

Support for multiple CVR formats #779

Closed

Aliases as a replacement for candidate codes #639

Aliases as a replacement for candidate codes #639

Conversation

artoonie commented Mar 15, 2023 • edited Loading

Implementation Details

Question: removing the concept of "Code" entirely

Screenshot: an old Dominion config.

Screenshot: multiple aliases

Test details (on both JSON and XLSX tests)

HEdingfield commented Mar 18, 2023

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

Choose a reason for hiding this comment

artoonie May 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HEdingfield commented Mar 20, 2023

artoonie commented Mar 20, 2023 • edited Loading

ggilber1 commented Mar 20, 2023 • edited Loading

artoonie commented Mar 20, 2023

ggilber1 commented Mar 20, 2023 via email

artoonie commented Mar 20, 2023

rrojas350 commented Mar 20, 2023

ggilber1 commented Mar 20, 2023 via email

andyanderson commented Mar 20, 2023

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

artoonie commented Apr 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HEdingfield commented May 3, 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

artoonie May 5, 2023 • edited Loading

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

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

artoonie commented Mar 15, 2023 •

edited

Loading

artoonie May 5, 2023 •

edited

Loading

artoonie commented Mar 20, 2023 •

edited

Loading

ggilber1 commented Mar 20, 2023 •

edited

Loading

artoonie May 5, 2023 •

edited

Loading