Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implement PJR checker #8160

Merged
38 commits merged into from
Mar 11, 2021
Merged

Implement PJR checker #8160

38 commits merged into from
Mar 11, 2021

Conversation

coriolinus
Copy link
Contributor

@coriolinus coriolinus commented Feb 19, 2021

Continuation of #7191. Adds a function which, given the complete set of voting data and a proposed solution, can determine whether or not that solution satisfies t-PJR for some selected t.

  • implement checker
  • determine an appropriate "standard" value for t which we can use at least in the tests develop an intuition for what t means, to aid in test scenario construction
  • implement fuzz test which ensures that seq-phragmen without fuzzing always passes the checker

@coriolinus coriolinus added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Feb 19, 2021
@coriolinus coriolinus self-assigned this Feb 19, 2021
@kianenigma
Copy link
Contributor

determine an appropriate "standard" value for t which we can use at least in the tests develop an intuition for what t means, to aid in test scenario construction

Also, note that this will probably be a configuration in any case. So we don't need to decide on this now and here.

coriolinus and others added 4 commits February 19, 2021 16:48
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Note that this currently fails. I hope that this can be rectified
by calculating the threshold instead of choosing some arbitrary number.
Comment on lines 161 to 163
/// `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. However,
/// this test is incomplete: while every solution which passes this test satisfies t-PJR, not every
/// solution which is t-PJR passes this test. We therefore look to a more accurate test.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. However,
/// this test is incomplete: while every solution which passes this test satisfies t-PJR, not every
/// solution which is t-PJR passes this test. We therefore look to a more accurate test.
/// `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR.

I think this whole sentence is misleading and either need to be reworked or removed. I know that you are trying to address the accuracy issues. But you are phrasing it in a way as if somehow, sometimes, a PJR solution is suddenly no longer PJR. A better assumption is: If a solution S is PJR and is no longer PJR due to accuracy issues, it is simply because it is no longer S but rather S', a variant thereof.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, that was in response to #8160 (comment). I think the actual intent is more like, "There exist solutions which do not pass this test but are nevertheless t-PJR."

I'll give this some thought and try to clarify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of the phrasing introduced in eb94375?

Copy link
Contributor

Choose a reason for hiding this comment

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

Better, but I think it is important to clarify why: accuracy errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I think it's possible for solutions to fail (in edge cases) for reasons that don't derive from accuracy errors. I'm basing that on this text:

The test implies the condition, but not every solution that satifies the condition passes the test.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM modulo a few small nits.

coriolinus and others added 9 commits March 2, 2021 09:21
This isn't ideal; more realistic numbers would be about twice these.
However, either case generation or voting has nonlinear execution
time, and doubling these values brings iteration time from ~20s to
~180s. Fuzzing 6x as fast should make up for fuzzing cases half the size.
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

looks very clean to me

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
@coriolinus
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Mar 11, 2021

Waiting for commit status.

@ghost ghost merged commit fcab5a3 into master Mar 11, 2021
@ghost ghost deleted the prgn-npos-pjr branch March 11, 2021 09:06
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants