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

Changed map to filter map so that Phragmen ignores empty voters #7378

Merged
4 commits merged into from Dec 3, 2020
Merged

Changed map to filter map so that Phragmen ignores empty voters #7378

4 commits merged into from Dec 3, 2020

Conversation

jaxter03
Copy link
Contributor

@jaxter03 jaxter03 commented Oct 23, 2020 •
edited

Fixed #7172

@cla-bot-2020
Copy link

cla-bot-2020 bot commented Oct 23, 2020

@ksr30 it looks like you have not signed our contributor license aggreement yet. Please visit this link to sign our agreement. This pull request cannot be merged until the agrement is signed.

@cla-bot-2020
Copy link

cla-bot-2020 bot commented Oct 23, 2020

@ksr30, Your signature has been received.

kianenigma
kianenigma suggested changes Oct 23, 2020
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment •
edited

Choose a reason for hiding this comment

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

Has major flaws.

@bkchr
Copy link
Member

bkchr commented Oct 30, 2020

@kianenigma so what do we want to do?

@kianenigma
Copy link
Contributor

Waiting for @ksr30 to address the comment. Can take over it later myself as well.

@jaxter03
Copy link
Contributor Author

Hey @kianenigma , sorry for late reply ... I will work on this tomorrow

@jaxter03
Copy link
Contributor Author

Hey @kianenigma , I pushed required changes ... please have a look

@jaxter03
Copy link
Contributor Author

jaxter03 commented Nov 3, 2020

Hey @kianenigma , is everything fine with PR. I see few checks are failing ... but maybe its because of nightly version. Otherwise all tests are passing on my system.

@jaxter03
Copy link
Contributor Author

jaxter03 commented Nov 9, 2020

Hey @kianenigma ,sorry to disturb you again ... is everything good with PR or you want me to change anything?

@bkchr
Copy link
Member

bkchr commented Nov 9, 2020

@ksr30 as a start, please merge master.

@jaxter03
Copy link
Contributor Author

jaxter03 commented Nov 9, 2020

Hey @bkchr .. sorry i didnt get

@bkchr
Copy link
Member

bkchr commented Nov 9, 2020

Merge master into your branch.

kianenigma
kianenigma suggested changes Nov 9, 2020
View reviewed changes
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.

Please write a test case that actually tests the logic change of the PR.

@jaxter03
Copy link
Contributor Author

@kianenigma @bkchr sorry for delay ... please have a look

thiolliere
thiolliere approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@thiolliere thiolliere left a comment

Choose a reason for hiding this comment

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

as far as I see, removing empty voters doesn't harm phragmms nor seq_phragmen

primitives/npos-elections/src/tests.rs Show resolved Hide resolved
primitives/npos-elections/src/lib.rs
@@ -629,7 +629,7 @@ pub(crate) fn setup_inputs<AccountId: IdentifierT>(
})
.collect::<Vec<CandidatePtr<AccountId>>>();

let voters = initial_voters.into_iter().map(|(who, voter_stake, votes)| {
let voters = initial_voters.into_iter().filter_map(|(who, voter_stake, votes)| {
Copy link
Contributor

Choose a reason for hiding this comment

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

doc of function could be changed to include this change.

primitives/npos-elections/src/lib.rs Show resolved Hide resolved
@jaxter03
Copy link
Contributor Author

Thanks @thiolliere

@jaxter03
Copy link
Contributor Author

@kianenigma @bkchr if PR is okay ... then can we merge it now 😅

@kianenigma kianenigma added 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. A8-mergeoncegreen labels Nov 24, 2020
@thiolliere
Copy link
Contributor

@ksr30 can you merge master again ? so that CI is happy

@kianenigma
Copy link
Contributor

@ksr30 ping

@jaxter03
Copy link
Contributor Author

jaxter03 commented Dec 2, 2020

@ksr30 ping

Sorry @kianenigma missed the notification ... I will merge master by tomorrow

@jaxter03
Copy link
Contributor Author

jaxter03 commented Dec 3, 2020

@kianenigma @thiolliere ... I think we can merge it now

@thiolliere
Copy link
Contributor

bot merge

@ghost
Copy link

ghost commented Dec 3, 2020

Trying merge.

@ghost ghost merged commit 675cbe9 into paritytech:master Dec 3, 2020
darkfriend77 pushed a commit to mogwaicoin/substrate that referenced this pull request Jan 11, 2021
@jaxter03 @darkfriend77
Changed map to filter map so that Phragmen ignores empty voters (pari…
d78553b 
…tytech#7378)

* Changed map to filter map so that Phragmen ignores empty voters

* Resolve flaws and added test case

* Updated test
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kianenigma kianenigma kianenigma approved these changes

@thiolliere thiolliere thiolliere approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Phragmen should ignore "empty" voters
4 participants
@jaxter03 @bkchr @kianenigma @thiolliere