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

When a user specifies an invalid carry value (bigger than the multipl… #65

Closed
wants to merge 3 commits into from

Conversation

OlivierSohn
Copy link
Contributor

@OlivierSohn OlivierSohn commented Mar 27, 2018

…han the multiplicator : force the carry value to be smaller than the multiplicator by using modulo.

Makes the implementation conform to the algorithm as described here.

Might be seen as a breaking change for users specifying out-of-range values for the carry, because they will get different results than before.

…icator), it is moduloed by the multiplicator.
@OlivierSohn
Copy link
Contributor Author

Also, closes #63

@OlivierSohn
Copy link
Contributor Author

@Shimuuar by any chance, did you have the time to look at this PR?

@Shimuuar
Copy link
Collaborator

Shimuuar commented Apr 9, 2018

I didn't sorry. Hopefully I'll do it this week

@Shimuuar
Copy link
Collaborator

I finally got to review of patch. This is indeed a bug and change have low impact. By default generator is initiialized with fixed carry 362436 < aa so any subsequent carry will be less than aa. So I will merge it.

One thing. aa = 1540315826 and 1540315826 ≠ 0x303EEE84. Where did you get that number. Still
your proof holds irrespective of value of aa

Thanks for catching this!

@OlivierSohn
Copy link
Contributor Author

Good catch, I think I took the other value from the "non complementary" version of mwc256, here : #33, because at the time I was also investigating differences between the 2 algorithms, and trying to understand which of the 2 was implemented here (btw, I think here it is the complementary version that is implemented after all)

I'll update the comments to use the right hexa which is 0x5BCF5AB2.

@OlivierSohn
Copy link
Contributor Author

The comments are fixed now!

@Shimuuar
Copy link
Collaborator

Actually proof could be simplified and generalized to arbitrary aa. Let b = 2^32, a < b and c < a. Then for every q :: Word32 c' < a

  1. ax <= a(b-1)
  2. ax + c <= a(b-1) + c = ab - a + c < ab

therefore ax + c / b < a

@OlivierSohn
Copy link
Contributor Author

Right, this is a more general formulation.

Do you mean we should update the comments?

In the code, I liked having actual numbers because IMO it makes it easier to follow the reasonning.

@Shimuuar
Copy link
Collaborator

As you wish.

I plan to merge this commit, cherry pick optimizations from #66, and make 0.14 release tomorrow.

@OlivierSohn
Copy link
Contributor Author

Ok, I'll add a commit to mention that the proof holds for any Word32 aa, but keep the rest as it is now.

@OlivierSohn
Copy link
Contributor Author

@Shimuuar is it ok for you? I'll close #66 once you cherry picked what you needed from it.

@Shimuuar
Copy link
Collaborator

Rebased and merged! Sorry for delay. I'll need to update documentation as well I think. Also please keep #66 open for a while

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.

2 participants