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

change the magic number 16807 to 16811 #947

Closed
wants to merge 2 commits into from
Closed

change the magic number 16807 to 16811 #947

wants to merge 2 commits into from

Conversation

blueloveTH
Copy link

@blueloveTH blueloveTH commented Apr 6, 2022

Checklist

  • The new code additions passed the tests (npm test).
  • The linter ran and found no issues (npm run-script lint).

Description

I detected a problem in the combination of ink v1.0.0 and ink.js v2.0.0.
If the length of a shuffle list is 7, it will always return the first element.
Such as:

{~1|2|3|4|5|6|7}
LIST lst = (a),(b),(c),(d),(e),(f),(g)
{LIST_RANDOM(lst)}

which always get 1 and a. However, the problem disappears for other lengths such as 6 or 8. It made me confused.

So I inferred that there is a magic number.
By checking the source, I identified it is 16807.

This number is a multiple of 7.
If this.seed * 16807 < 2147483647, PRNG.next() % 7 should always be zero.
In the implmentation of LIST_RANDOM, it uses PRNG.next() % list.Count.
So I think this is why we always get the first element on length == 7.

My solution is to replace 16807 with 16811. The latter is a prime.
I tested on our usecases to confirm that this modification solves the magical problem.

@blueloveTH
Copy link
Author

Update the assertion value under 16811 to pass npm run test.

    expect(story.Continue()).toEqual("14\n");
    expect(story.Continue()).toEqual("-10\n");

@smwhr
Copy link
Collaborator

smwhr commented Apr 11, 2022

Thanks for your contribution @blueloveTH !

16807 is actually a number that's usually recommended as the a in the PRNG we use (see original paper here) because it has "good random properties" while still cycling and now that you point it out, I think we have an error in the algorithm, there should be a -1 somewhere to break the %7 = 0 chain.

@@ -155,7 +155,7 @@ describe("Logic", () => {
it("should generate random numbers", () => {
story.ChoosePathString("logic.random");

expect(story.Continue()).toEqual("15\n");
Copy link
Collaborator

@smwhr smwhr Apr 12, 2022

Choose a reason for hiding this comment

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

-    expect(story.Continue()).toEqual("15\n");
-    expect(story.Continue()).toEqual("-24\n");
+    expect(story.Continue()).toEqual("27\n");
+    expect(story.Continue()).toEqual("8\n");

@@ -8,7 +8,7 @@ export class PRNG {
if (this.seed <= 0) this.seed += 2147483646;
}
public next(): number {
return (this.seed = (this.seed * 16807) % 2147483647);
Copy link
Collaborator

Choose a reason for hiding this comment

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

-    return (this.seed = (this.seed * 16807) % 2147483647);
+    return (this.seed = (this.seed * 48271) % 2147483647);

@smwhr
Copy link
Collaborator

smwhr commented Apr 12, 2022

After further investigation :

  • the %7 problem happens when the story seed is low enough
  • we ported as is
storySeed = (new Random (timeSeed)).Next () % 100;

which makes the seed low enough

  • since 1993, everyone recommends using 48271 instead of 16807
  • 48271 works well in the local tests I did

So if that's ok with you @blueloveTH, I commented your code with the value expected for the tests with 48271.

@y-lohse @ephread I think we could merge this one (because that's a quick fix) while work is being done in #552

@blueloveTH
Copy link
Author

Thanks for your investigation @smwhr !
Through your explanation, I have roughly understood the idea of this PRNG algorithm. 48271 is a well-chosen prime number with good random properties. It should work better!

@y-lohse
Copy link
Owner

y-lohse commented May 7, 2022

Happy to merge this if we do the changes suggested by @smwhr , thanks for the investigation :)

@smwhr smwhr changed the base branch from master to fix/prng-magic-number May 7, 2022 12:03
@smwhr
Copy link
Collaborator

smwhr commented May 7, 2022

Closing this one in favour of #952
Thanks @blueloveTH for the initial push in the right direction !

@smwhr smwhr closed this May 7, 2022
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.

3 participants