Cdf Json Fixes #506
Merged
Cdf Json Fixes #506
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
update unisyn cvr test to use the full (but still incorrect) cvr data. This test is expected to fail. add candidate parsing code to xml CDF parser
…e constraints on validation as all other file types. The un-used candidate parsing code is left in for implementing future helper functions which will automatically populate config files.
# Conflicts: # src/main/java/network/brightspots/rcv/CommonDataFormatReader.java
fix ranking parsing add example Unisyn contests
tests share a single cvr input file
- all static election objects are parsed at beginning of cvr processing - during processing contest selections are linked to candidate objects - candidates are stored in config - not pulled from CDF file - fixed NIST test set 0 config and CDF json to match the NIST 1500-103 UML - verified tabulation results are correct for Nist test set 0 test
…and link to them from ContestSelection objects. verified updated NIST test 0 cvr output
# Conflicts: # src/main/java/network/brightspots/rcv/CommonDataFormatReader.java
Candidate validation and CandidateId validation are done while parsing CVRs (not during static parsing) xml parser throws unrecognizedCandidatesException (instead of just CVRParserException) Add json getCvrContest helper json parser logic upgrades: handle writeIns in both places check for rank in both places check for HasIndication check for IsAllocable synthesize cvrId
tarheel
reviewed
Sep 20, 2020
There was a problem hiding this comment.
Could you explain a bit what the changes are to CommonDataFormatReader? I'm having trouble looking at this PR and figuring out how much is just moving code around and how much is substantive changes to the logic.
Also, would it be possible to make it an abstract class and then create a separate concrete child class for each format? I'm finding it hard to understand which logic is shared and which is format-specific. You're already sort of doing this just with the way you're naming the internal methods, so maybe this won't be a difficult refactor.
| private static String getCdfIdForCandidateCode(String code) { | ||
| String id = candidateCodeToCdfId.get(code); | ||
| // generates an internal ContestSelectionId based on a candidate code | ||
| private static String getContestSelectionIdForCandidateCode(String code) { |
There was a problem hiding this comment.
please keep "Cdf" in this method's name for clarity
There was a problem hiding this comment.
Yeah sorry about that @tarheel! I did not give you much explanation.
Among the important things:
- All static election objects (Candidates, Contests, ContestIds) are parsed at beginning of cvr processing. This simplifies and clarifies the parsing somewhat.
- CVRContestSelections now contain links to Candidate objects. During cvr processing these links are used to look up the linked Candidate. From there, Candidate Name field is used to assign the rankings and create the CastVoteRecord for tabulation. Previously, Candidate names were embedded directly in the CVRContestSelection objects using a "Code" object with a 'vendor-label' field on it. This is non-standard, and while it's conceivable a vendor might do this (Code objects allow flexibility) it's not the primary way CDF encodes information about a Candidate. The primary way is by linking to a Candidate object.
- Candidate names are stored in config - not pulled from CDF file at tabulation time. This normalizes candidate logic across formats.
- Configs do not use CDF internal ids to as candidate codes. It's important to understand that CDF internal object links (e.g. CandidateIds contained in a CVRContestSelection in CDF should be used only within the context of the CDF. They are generated by the cvr report generating program, and should not appear in our config files.
- Filter contest tabulation by contest ID as specified in the config file
So to your second question: yes it would be possible for these methods to share an interface. I think the real move is for all reader objects to share an interface. I don't think what you propose would be difficult, nevertheless it's outside the scope of this PR.
There was a problem hiding this comment.
Could you create a follow-up issue (not for this deadline) to refactor this class?
There was a problem hiding this comment.
Sure, we already have such a ticket: #251
src/main/java/network/brightspots/rcv/CommonDataFormatReader.java
Outdated
Show resolved
Hide resolved
moldover
commented
Sep 21, 2020
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "tabulatorVersion": "TEST", | |||
| "outputSettings": { | |||
| "contestName": "Test Set 1", | |||
| "contestName": "test set 1", | |||
There was a problem hiding this comment.
@tarheel this is still just for output.
moldover
commented
Sep 21, 2020
| @@ -15,23 +15,23 @@ | |||
| "idColumnIndex" : "", | |||
| "precinctColumnIndex" : "", | |||
| "provider" : "CDF", | |||
| "contestId": "contest-001" | |||
| "contestId": "test set 1" | |||
There was a problem hiding this comment.
Tabulator will filter Contest to tabulate based on this.
moldover
commented
Sep 21, 2020
| } ], | ||
| "candidates" : [ { | ||
| "name" : "A", | ||
| "code" : "cs-a", | ||
| "name" : "Candidate A Name", |
There was a problem hiding this comment.
Here I removed the internal cdf CandidateId usage, and changed the name to something that is more clearly a Name.
moldover
commented
Sep 21, 2020
| @@ -799,45 +799,44 @@ | |||
| "Election" : [ { | |||
| "@id" : "election-001", | |||
| "@type" : "CVR.Election", | |||
| "Candidate" : [ | |||
There was a problem hiding this comment.
Here you see a very simple example of top-level "static" Candidate objects. One could attach more Candidate info here e.g. a PartyId.
moldover
commented
Sep 21, 2020
| "Value" : "A" | ||
| } ] | ||
| "@type" : "CandidateSelection", | ||
| "CandidateIds" : [ "candidate-a" ] |
There was a problem hiding this comment.
@tarheel Here we replace a 'Code' object with a link to a Candidate object. Note that confusingly it is actually an array, and is correspondingly named CandidateIds. This is to support "party ticket" options, and is a rare thing, but we all have to pay the price. It's important to note the multiplicity of links in the CDF UML spec.
tarheel
reviewed
Sep 22, 2020
src/main/java/network/brightspots/rcv/CommonDataFormatReader.java
Outdated
Show resolved
Hide resolved
src/main/java/network/brightspots/rcv/CommonDataFormatReader.java
Outdated
Show resolved
Hide resolved
HEdingfield
reviewed
Sep 22, 2020
tarheel
approved these changes
Sep 24, 2020
tarheel
reviewed
Sep 24, 2020
Comment on lines
+428
to
+433
| if (candidateId == null || candidateId.isEmpty()) { | ||
| Logger.severe( | ||
| "Undeclared write-in candidate found while parsing CVR, however undeclared " | ||
| + "write-in label has not been defined in config."); | ||
| throw new CvrParseException(); | ||
| } |
There was a problem hiding this comment.
@moldover I added an inline comment here but GitHub's crappy UI seems to have discarded it...? Anyway, the comment was that you added the same six lines in four different places, so please factor it out into a helper. Also, you can use isNullOrBlank.
moldover
added a commit
that referenced
this pull request
Aug 12, 2022
* plumbing for xml cdf reading * add contestId to json and xml CDF parse logic update unisyn cvr test to use the full (but still incorrect) cvr data. This test is expected to fail. add candidate parsing code to xml CDF parser * dont parse Candidate data from CDF at runtime - this enforces the same constraints on validation as all other file types. The un-used candidate parsing code is left in for implementing future helper functions which will automatically populate config files. * Handle ContestSelections in XML correctly * add NIST example 2 (seems to be correctly formed) * fill out CDF XML class definitions needed for tabulation * add some helpers * cleanup pre-processing code * add more class fields around contest selections fix ranking parsing add example Unisyn contests * handle write-ins * get first 2 Unisyn regression tests working :) * fix first NIST CDF XML test * cleanup test names * parse GpUnit from CVRs * remove un-used assets * updates for PR #504 * updated data from Unisyn with GpUnit (precinct) parsing validated * add more Unisyn regression tests tests share a single cvr input file * tabulate all elections - in practice we should not see more than one * updates for PR #504 * better logging * first cut at fixing JSON CDF reader: - all static election objects are parsed at beginning of cvr processing - during processing contest selections are linked to candidate objects - candidates are stored in config - not pulled from CDF file - fixed NIST test set 0 config and CDF json to match the NIST 1500-103 UML - verified tabulation results are correct for Nist test set 0 test * update ResultsWriter CVR generation code to create Candidate objects and link to them from ContestSelection objects. verified updated NIST test 0 cvr output * updates for PR #504 * fix typo * update test asset with contest name * better handling for cdf reader parse errors * update all CDF json assets * Updates to CDF parsing logic: Candidate validation and CandidateId validation are done while parsing CVRs (not during static parsing) xml parser throws unrecognizedCandidatesException (instead of just CVRParserException) Add json getCvrContest helper json parser logic upgrades: handle writeIns in both places check for rank in both places check for HasIndication check for IsAllocable synthesize cvrId * cleanup for PR #506 * updates for PR #505 * Throw if undeclared write-in is found but label has not been defined.
moldover
added a commit
that referenced
this pull request
Aug 15, 2022
* Changed package from com.rcv to network.brightspots.rcv. * remove unused tests: fix for #197 * add tiebreak test #197 * Allows user to cancel during interactive tiebreaker (fixes #132); adds example config file for testing interactive tiebreaker with sequential multi-seat enabled; adds commented command in build.gradle for executing CLI arguments. * add missing precinct test * Reconciles branch with `develop`. * Re-adds interactive tiebreaker sample input. * Continues fixing recently broken tests. * Reverts name back to "Universal RCV Tabulator". * add new random seed config field * updated tests * incorporate new option into tabulation logic and add test case * onlyOneWinnerPerRound -> allowOnlyOneWinnerPerRound * update test case to include a tie-break * update tests * address comments * tests * add test case * update one test for each winnerElectionMode option * updating tests * fix tests * fix minor output bug and add test case for previousRoundCountsThenRandom tie-break option * fix minor issue with transfer tallies * update test configs * update a ton of JSON files! * fix bad source path * Increment version from 0.1.0 to 0.1.1. (#373) * Increment version from 0.1.1 to 0.1.2 * Increment version from 0.1.2 to 1.0.0 (#384) * v1.0.0 * Increment version from 1.0.0 to 1.0.1 (#389) * Fix broken tests (#393) * Increments version from "1.0.1" to "1.0.1_20200320_internal" (#423) * Adds multiSeatBottomsUpPercentageThreshold (#428) * Tests for Dominion CSV conversion routine (#429) * test for Dominion csv conversion routine #415 * 2 more tests for cvr conversion * updates for Dominion conversion tests: PR #425 * fix for PR #425 Co-authored-by: jonmoldover <jonmoldover@gmail.com> * Adds AUTOMATED_TEST_VERSION code so tests can remain stable as app version updates (fixes #426). (#435) * Fixes Dominion conversion output filenames and path (#436) * Dominion conversion output files now output to same directory as input files and include timestamp in filename (#408). * Updates Dominion tests to account for new filename schema; Dominion test files are now cleaned up automatically; fixes bug with fileCompare not closing readers. * All delete operations in `TabulatorTests` now handle exceptions with useful error messages if they fail. * read and write precinct portion and precinct (#433) * read and write precinct portion + precinct * edits for precinct portion PR#433 * Update src/main/java/network/brightspots/rcv/DominionCvrReader.java Co-Authored-By: HEdingfield <HEdingfield@users.noreply.github.com> * Update src/main/java/network/brightspots/rcv/DominionCvrReader.java Co-Authored-By: HEdingfield <HEdingfield@users.noreply.github.com> Co-authored-by: HEdingfield <HEdingfield@users.noreply.github.com> * UWI test (#440) * add UWI test files * add an additional test to validate my fix of #397 * use a better name for the first test * make batch elimination work properly with singleSeatContinueUntilTwoCandidatesRemain (#441) * make batch elimination work properly with singleSeatContinueUntilTwoCandidatesRemain * cleaned up wording * v1.1.0 (#454) ### New features: * Added support for converting Dominion JSON CVRs to generic .csv format (including precinct portions) (#404, #406, #407, #408, #415, #439) * Added multiSeatBottomsUpPercentageThreshold option (#403) * Added CLI option to convert Dominion CVR to generic .csv (#408) * New GUI menu and conversion options (can now convert to CDF and convert Dominion to generic via the GUI) (#408, #421) * Added Dominion Alaska CVR to sample_input folder ### Bug fixes: * Batch elimination now works properly with singleSeatContinueUntilTwoCandidatesRemain (#396) * In a multi-seat contest, if someone wins in the first round, we now automatically eliminate undeclared write-ins before we eliminate any other candidates; previously, we treated UWIs like a normal candidate, which meant we potentially eliminated other candidates with lower tallies first (#397) * If UWI exceeds the winning threshold in the initial count, we no longer mistakenly elect this candidate (#398) ### Backend updates: * Updated dependencies to latest version: JDK, JavaFX, Checkstyle google_checks.xml, Checkstyle plugin, org.openjfx.javafxplugin, org.beryx.jlink, org.apache.commons:commons-csv, org.apache.poi:poi-ooxml, com.fasterxml.jackson.core:jackson-* * Added special code to test configs to obviate the need to update the version with each increment (#426) * Updated tests and improved test coverage * Copyright update (#414) * Code cleanup * Hart (#458) * add contest id to config * add xml parsing module * First cut at Hart CVR reader + test assets. * cleanup paths and add expected results * add full cvr data * refactor contestId from Integer to String * add school board test * fix xml parsing * logging * re-add accidentally removed import * cleanup for PR#548 * updates for PR #458 * updates for PR #458 * test other file reading options * more cleanup for PR #458 * more cleanup for PR #458 * Fixes tests not working in Windows due to mangled UTF-8 BOM marker. * Handles potential NPE if chosen directory contains no files. Co-authored-by: HEdingfield <hylton@groupagree.com> * Update dependencies (#459) * Updates to latest version: * Gradle 6.5.1 * JDK 14.0.1 * JavaFX 14.0.1 * Checkstyle google_checks.xml 8.34 * Checkstyle plugin 8.34 * org.openjfx.javafxplugin 0.0.9 * org.beryx.jlink 2.20.0 * com.fasterxml.jackson.core:jackson-* 2.11.1 * org.junit.jupiter.junit-jupiter-* 5.6.2 * Gets tests working with Gradle 6.5.1 (fixes #283) * Updates `APP_VERSION` to temporary value 1.2.0_alpha * Addresses Checkstyle warnings * Fixes versions in Hart tests * make contestId a real config field (#463) * make contestId a real config field * update .txt * Providers dropdown (#462) * Changes "Provider" field for CVR source to be required, bases it on an enum of valid values, and changes it to a ChoiceBox at the start of the "CVR Files" tab in the GUI (progress on #460 and #461). * Adds log message to help identify which CVR source validation errors pertain to. * Gets rid of `SimpleObjectProperty` warnings during compile (fixes #280). * Hart GUI (#466) * Provider ComboBox now dynamically enables and disables fields and buttons when different providers are selected. (Progress on #461... actually progress on GUI phase 2!) * Enables `DirectoryChooser` for Hart so it now functions through the GUI. (Fixes #460) * Validation improved to fail if `firstVoteColumnIndex`, `firstVoteRowIndex`, `idColumnIndex`, `precinctColumnIndex` are defined for CVRs from non-ES&S providers. * Removes superfluous logging. * direct Dominion tabulation (#470) * direct Dominion tabulation * enable specifying a Dominion CVR source path in the UI * output generic CSV and add test coverage * remove extra log line * filter out CVRs from other contests during the loading * Changes `contestId` so it's specified on a per-CVR basis (fixes #456) (#474) * First cut at Clear Ballot rcv reader (#475) * First cut at Clear Ballot rcv reader Add Kansas Dem Primary regression test Add Clear Ballot reference files #400 * updates for PR #475 * updates for PR #475 * updates for PR #475 * Enable Clear Ballot tabulation in GUI (progress on #400) (#480) * GUI redesign phase 1 (#492) * Splits Output tab into new Contest Info and Output tabs. * Redesigns GUI CVR Files tab, adds Clear button, and changes Add button so it only clears the file path to make it easier to manually enter multiple sources that share fields. * Improves visual presentation of Candidate tab; adds Clear button and adds checkBoxCandidateExcluded when adding a candidate. * Reorganizes presentation of rules in "Winning Rules" and "Voter Error Rules" tabs. * Winner Election Mode and Tiebreak Mode now start undefined with all relevant fields disabled; choosing specific modes enables applicable fields. * Changes Winner Election Modes and Tiebreak Modes to be more user-friendly, including necessary migration logic to update older config files. * Adds `continueUntilTwoCandidatesRemain` boolean (fixes #481). * Expands footprint of GUI window to 1200x1000. * Implements bordered boxes. * remove Dominion conversion feature (#487) * update test files * remove conversion feature from code * update test configs * remove unnecessary check * allow overvote delimiter for ES&S source files (#503) * test * update winner election mode logic * comments * final comments * Read and Tabulate Unisyn Xml Cdf (#504) * plumbing for xml cdf reading * add contestId to json and xml CDF parse logic update unisyn cvr test to use the full (but still incorrect) cvr data. This test is expected to fail. add candidate parsing code to xml CDF parser * dont parse Candidate data from CDF at runtime - this enforces the same constraints on validation as all other file types. The un-used candidate parsing code is left in for implementing future helper functions which will automatically populate config files. * Handle ContestSelections in XML correctly * add NIST example 2 (seems to be correctly formed) * fill out CDF XML class definitions needed for tabulation * add some helpers * cleanup pre-processing code * add more class fields around contest selections fix ranking parsing add example Unisyn contests * handle write-ins * get first 2 Unisyn regression tests working :) * fix first NIST CDF XML test * cleanup test names * parse GpUnit from CVRs * remove un-used assets * updates for PR #504 * updated data from Unisyn with GpUnit (precinct) parsing validated * add more Unisyn regression tests tests share a single cvr input file * tabulate all elections - in practice we should not see more than one * updates for PR #504 * updates for PR #504 * Sundry UX fixes (#509) * Converts `overvoteRule` from a `ChoiceBox` to an array of `RadioButton`s; changes `overvoteRule` string display in config files and adds migration logic. * Disables `decimalPlacesForVoteArithmetic` and `nonIntegerWinningThreshold` except when `winnerElectionMode` is "Multi-winner allow only one winner per round" or "Multi-winner allow multiple winners per round" (fixes #500). * Adds suggested values for `overvoteLabel`, `undervoteLabel`, and ES&S column and row indices as requested by @chughes297 (overriding what was in the original GUI mock-ups). * Clarifies language to address confusing UX in `passesBasicCvrSourceValidation()` for CVR-specific errors. * Cdf Json Fixes (#506) * plumbing for xml cdf reading * add contestId to json and xml CDF parse logic update unisyn cvr test to use the full (but still incorrect) cvr data. This test is expected to fail. add candidate parsing code to xml CDF parser * dont parse Candidate data from CDF at runtime - this enforces the same constraints on validation as all other file types. The un-used candidate parsing code is left in for implementing future helper functions which will automatically populate config files. * Handle ContestSelections in XML correctly * add NIST example 2 (seems to be correctly formed) * fill out CDF XML class definitions needed for tabulation * add some helpers * cleanup pre-processing code * add more class fields around contest selections fix ranking parsing add example Unisyn contests * handle write-ins * get first 2 Unisyn regression tests working :) * fix first NIST CDF XML test * cleanup test names * parse GpUnit from CVRs * remove un-used assets * updates for PR #504 * updated data from Unisyn with GpUnit (precinct) parsing validated * add more Unisyn regression tests tests share a single cvr input file * tabulate all elections - in practice we should not see more than one * updates for PR #504 * better logging * first cut at fixing JSON CDF reader: - all static election objects are parsed at beginning of cvr processing - during processing contest selections are linked to candidate objects - candidates are stored in config - not pulled from CDF file - fixed NIST test set 0 config and CDF json to match the NIST 1500-103 UML - verified tabulation results are correct for Nist test set 0 test * update ResultsWriter CVR generation code to create Candidate objects and link to them from ContestSelection objects. verified updated NIST test 0 cvr output * updates for PR #504 * fix typo * update test asset with contest name * better handling for cdf reader parse errors * update all CDF json assets * Updates to CDF parsing logic: Candidate validation and CandidateId validation are done while parsing CVRs (not during static parsing) xml parser throws unrecognizedCandidatesException (instead of just CVRParserException) Add json getCvrContest helper json parser logic upgrades: handle writeIns in both places check for rank in both places check for HasIndication check for IsAllocable synthesize cvrId * cleanup for PR #506 * updates for PR #505 * Throw if undeclared write-in is found but label has not been defined. * make some fields per-source instead of per-config (#517) * create new class * helpful errors in CLI; don't allow newer version in config than in app * make fields per-source * comments * Jon's changes * fix almost all tests * fix Dominion tests * update validation and documentation for 4 fields that moved to CVR level (#518) * update validation and documentation for 4 fields that moved to CVR level * Fixes broken tests and a couple of typos. Co-authored-by: HEdingfield <hylton@groupagree.com> * camelCase strings for enums in config files (#519) * use camelCase for enum values in configs and update migration logic accordingly * disable editing of CVR source rows in GUI table * Final 1.2.0 polish (#526) * Updates checkstyle from 8.34 to 8.36.2, and google_checks.xml. * Creates and implements separate `checkstyle-suppressions.xml` file to suppress Checkstyle warnings that don't make sense for our project (fixes #489). * Addresses relevant Checkstyle warnings (fixes #490). * Addresses IntelliJ warnings. * Pixel-pushing for Linux (get thicc, boi) * Changes all logging to use new `fine()`, `info()`, `warning()`, and `severe()` methods. * Gets rid of now-unused `onEditCommit` functions for the CVR Files and Candidates tables. * Makes util classes `final`, with private constructors to prevent instantiation. * Renames `TieBreakMode`, `tieBreakMode`, `TieBreak`, and `tieBreak` to `TiebreakMode`, `tiebreakMode`, `Tiebreak`, and `tiebreak`, respectively. This word has now lost all meaning. * Removes unnecessary usages of `.toString()`. * Standardizes exceptions as `exception` instead of `e` to comply with VVSG requirement on variable names (5.2.5 paragraph c) that only index loops are allowed to have single-character variable names. * Fixes broken test. * Minor fixes to hints. * Get rid of the Devilish Double Spaces after periods. * Minor cleanups (#527) * fix README * CDF JSON: throw if a CandidateObject can't be found during parsing. * Cleanup CDF class (type) names as output from ResultsWriter. * fix label * Handle missing precinct data for older Dominion data sets #533 (#534) * Handle missing precinct data for older Dominion data sets #533 * Add test data with missing precinct data for older Dominion data sets #533 * Fix for #536 - allow multiple CDF files (#537) * Fix for #536 - allow multiple CDF files Add regression test with multiple CDF files * fix overvote delimiter logic and update test (#539) * Fixes for bad merge between `master` and `develop`. * Add README and LICENSE for split repo * latest updates from develop * update cdf output names for #584 * add dominion multi file tests * add licence file #551 * remove test_data as submodule #612 * remove test_data submodule #612 * remove submodule reference from docs #612 Co-authored-by: HEdingfield <hylton+git@groupagree.com> Co-authored-by: hedingfield <hylton@groupagree.com> Co-authored-by: Louis Eisenberg <tarheel@gmail.com> Co-authored-by: HEdingfield <HEdingfield@users.noreply.github.com> Co-authored-by: Armin Samii <artoonie@gmail.com> Co-authored-by: Armin Samii <armin.samii@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses #505 by fixing our Cdf Json reader and writer implementation. This is essentially the analogous work to #504 but in Json. It also updates all existing regression test assets to reflect the fixes.
Might be best to start by looking at how one of the regression test input cdf cvr files has changed. That will demonstrate the linkage between Candidate objects and ContestSelection objects.
There are a couple small things remaining to do, but I wanted to get this PR going as the core tabulation code, and test assets are ready to go.