Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consolidate all duplicate CVRs #708

Merged
merged 10 commits into from Jun 21, 2023
Merged

Conversation

artoonie
Copy link
Collaborator

closes #692

Note: one of the original goals was to specifically get rid of the duplicate files in #650, but I opted to skip that: while most files are the same, one test removes the Precinct file, and the other doesn't, meaning the test sets aren't identical. I'm sure we could solve that with some added complexity, but I'm not sure that complexity is worth it.

Copy link
Contributor

@HEdingfield HEdingfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Placing a hold on this until we get the parent branch merged. Will review once that's done.

@artoonie artoonie force-pushed the feature/issue-692_shared-tests branch 2 times, most recently from 977a128 to 15d2534 Compare June 18, 2023 22:05
@artoonie artoonie force-pushed the feature/issue-692_shared-tests branch from 15d2534 to a147699 Compare June 19, 2023 14:19
@artoonie artoonie force-pushed the feature/issue-692_shared-tests branch from a147699 to 7080eaf Compare June 19, 2023 14:27
@artoonie artoonie force-pushed the feature/issue-703_convert-to-cdf branch 2 times, most recently from 940d45d to b441466 Compare June 19, 2023 19:14
Base automatically changed from feature/issue-703_convert-to-cdf to develop June 20, 2023 05:16
# Conflicts:
#	src/main/java/network/brightspots/rcv/TabulatorSession.java
#	src/test/java/network/brightspots/rcv/TabulatorTests.java
Copy link
Contributor

@HEdingfield HEdingfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great clean-up! Just a few questions before merging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question on this... wondering why this was around if it apparently wasn't being used?

Copy link
Collaborator Author

@artoonie artoonie Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Response to all four comments:

The two unused files were duplicates, which is why the two unshared CVRs were moved to _shared.

In particular, ../_shared/skipped_ranking_cvr.xlsx == test_set_2_overvote_skip_to_next_cvr.xlsx and
../_shared/minneapolis_multi_seat_cvr.xlsx == ../minneapolis_multi_seat_threshold/minneapolis_multi_seat_cvr.xlsx.

It seems as though the XLSX CVRs are indeed never used -- they're not related to the CDF CVRs in any way I can discern. Of particular note, the candidate names in the XLSX are not the same as what's in the config.

To summarize:

  1. The files in _shared were duplicate files, but
  2. One of the duplicates was never used, so
  3. I've moved the files out of _shared and left their corresponding duplicates deleted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good deal, thanks for examining this so closely!

@artoonie artoonie force-pushed the feature/issue-692_shared-tests branch from b8270de to 80ba4d8 Compare June 20, 2023 15:06
Copy link
Contributor

@HEdingfield HEdingfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job! Left a few last nit comments, but pre-approving.

@@ -10,7 +10,7 @@
"generateCdfJson" : false
},
"cvrFileSources" : [ {
"filePath" : "skip_to_next_test_cvr.xlsx",
"filePath" : "skipped_ranking_cvr.xlsx",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Is it worth renaming this one? Have a slight preference to just revert these two files so the name stays aligned with the directory / config file naming.

@@ -11,7 +11,7 @@
},
"cvrFileSources": [
{
"filePath" : "minneapolis_multi_seat_threshold_cvr.xlsx",
"filePath" : "minneapolis_multi_seat_cvr.xlsx",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: same question here -- can we just revert this and the file to stay aligned with directory name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good deal, thanks for examining this so closely!

@artoonie artoonie merged commit 62513c7 into develop Jun 21, 2023
1 check passed
@artoonie artoonie deleted the feature/issue-692_shared-tests branch June 21, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a common test suite for shared CVRs
2 participants