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

Improve randomness uniformity #281

Conversation

rodrigopinto
Copy link

This PR is for improving the randomness calculation for the percentage of actors. It's a port of the implementation on fetlife/rollout#120.

As the new implementation reduces the fails in a really small decimal part, I haven't added a spec for this scenario. I also believe that it could become an expensive spec.

On the rollout implementation, there is a benchmark exposing the approximate accuracy of this implementation.

Closes #272

@jnunemaker
Copy link
Collaborator

Hey thanks! Sorry I haven't responded to this. I was gone on vacation. Thank you so much for tackling it! I really apprecate the help. I'll try to look it over this weekend or first thing next week.

One question: will I need to note this as a breaking change in the changelog? I'm assuming that individual actors who were enabled before, may or may not be following this change. I understand that the same percentage will be enabled, but would upgrading flipper with any % of actors enablements were not 100% cause the set of enabled actors to differ? Just want to make sure I note it correctly for people.

I'll probably test it with 100 or 1k actors or something and get the enabled ones before this change and after and then do a set diff. If you have time to tackle that and drop the work here in a comment, it would help getting this merged faster, but if not I'm happy to look at it as soon as I have time.

@rodrigopinto
Copy link
Author

rodrigopinto commented Sep 7, 2017

Hey @jnunemaker, so, it was my turn to be on vacation, sorry for the delayed answer.

As you mentioned, the percentage will probably be similar, but with a really small decimal difference. In few cases, it will be higher & other cases lower than the old implementation %, so it makes sense to mention on the changelog this possibility.

I can do the test batch, just give me some time, so I work on it and drop the result here. Not sure about this weekend, but I do my best to get it done asap. :-)

@jnunemaker
Copy link
Collaborator

@rodrigopinto sounds great. If I get a chance I'll whip something together first, but if not I'd love the help. Thanks!

@rodrigopinto
Copy link
Author

It has a really long time since I opened this PR and it got stale, so I will close it but keep the fork implementation in case I come back to work with this lib again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Randomness Uniformity
2 participants