Improve random() #269

Merged
merged 3 commits into from Oct 12, 2016

Projects

None yet

2 participants

@CapacitorSet
Contributor
CapacitorSet commented Oct 9, 2016 edited

This change makes the random generator rely on existing, internal source code, rather than relying on direct access to the internal randomness source. This code is more robust, since it guarantees that random(x) will never return x (if x != 0).

Fixes #267

@Pomax
Member
Pomax commented Oct 10, 2016

Can you add a description of what this fixes and how it does this? This helps with the review (especially to make sure the code matches what the intent of the PR).

One tip for PRs on github, if you add fixes #... to the initial comment (where the ... part is an issue number) github will automatically crosslink the PR to the issue, and close if when the PR lands.

@CapacitorSet
Contributor
CapacitorSet commented Oct 10, 2016 edited

Thanks for the PR tip! I used the "fixes #x" syntax in the commits themselves (the only usage I knew), do you know if it will also fix the issues automatically?

@Pomax
Member
Pomax commented Oct 10, 2016

Hmm, as far as I know it needs to be in the PR comment for github to do its "magic" properly. At the very least, putting it in the PR comment also lets reviewers check the original issue that's being fixed, which lets them easily verify the PR indeed fixes what the original issue was about.

src/P5Functions/Math.js
- if(arguments.length === 1) {
- return internalRandomGenerator() * arguments[0];
+ var aMin, aMax;
+ switch (arguments.length) {
@Pomax
Pomax Oct 10, 2016 edited Member

Switches are great for long chains, but in this case we can use two if statements instead:

p.random = function(aMin, aMax) {
  if (arguments.length === 0) {
    aMin = 0;
    aMax = 1;
  } else if (arguments.length === 1) {
    aMax = aMin;
    aMin = 0
  }
  ...
src/P5Functions/Math.js
+ break;
+ default:
+ aMin = arguments[0];
+ aMax = arguments[2];
@Pomax
Pomax Oct 10, 2016 edited Member

Are you sure you meant arguments[2] here? As second argument, this would be argument[1]. That said, with the rewrite above this would no longer be an issue

@CapacitorSet
Contributor

Thanks for the feedback. I can't believe I missed that arguments[2]!

Anyway, I both updated the test suite to catch this error (now tests for a <= x < b) and applied your suggestions.

@Pomax
Member
Pomax commented Oct 10, 2016 edited

Nice! I'm flying back home tomorrow evening, where I will be in a better spot to run the full suite, but this looks pretty good. Can you explain what the unit test was doing "wrong" that necessitated fixing it btw? (it looks like the new code does exactly the same as the old code, and unit tests should not need to be rewritten, they're designed to test whether code does "what it should do", so those tests should still be valid)

test/unit/random.pde
@@ -48,15 +48,15 @@ randomSeed(14);
var minSample = 30, maxSample = 40;
for(int i=0;i<attempts;++i) {
var sample = r1[i];
- if(sample < 0 || sample >= 1) _checkTrue(false); // bad random with no args
+ if (!(sample < 1 && sample >= 0)) _checkTrue(false); // bad random with no args
@Pomax
Pomax Oct 10, 2016 edited Member

hm, the old test code should have already done the right thing: if sample is less than 0 or it's greater or equal to one, it should throw an error, by exploiting the fact that false will obviously fail a checkTrue check.

Can you explain how this rewrite improves on it? (it has certainly become harder to understand at a glance with this new ordering and effective double negative)

@CapacitorSet
Contributor
CapacitorSet commented Oct 10, 2016 edited

The problem I introduced was that random with two arguments (say, random(0, 10)) would return NaN. The previous test did the following:

if (result < 0 || result >= 10) _checkTrue(false);

Because NaN < 0 is false and NaN >= 10 is false, the test would pass, which is obviously the wrong thing to do.

What it does now is:

if (!(result >= 0 && result < 10)) _checkTrue(false);

For all numeric values of result, it behaves exactly the same as before; for result equal to undefined or NaN, it behaves correctly (it calls _checkTrue(false)).

A cleaner way would be to simply call _checkTrue(result >= 0 && result < 10), but then it would appear that attempts = 10000 tests had been run (whereas the current test suite will report 0 tests run for the for loop in question). So, for instance, if the test at L76 failed, it would be reported as test #30003 (rather than test 3).

Perhaps the tests can be rewritten to use assertions instead?

Edit: or some other way to do 10k checks, while counting as one test, anyway.

@Pomax
Member
Pomax commented Oct 10, 2016 edited

That's actually a reason to add a new test, rather than change old tests: random(), no matter the input, should never yield NaN if we do our argument handling properly.

Instead, we'll need to find out why it can yield NaN, and then fix that, while also adding the unit test code to catch it so that it can't silently regress in the future =)

@CapacitorSet
Contributor

Do you want me to revert the previous commit and add a check that verifies that ŧypeof result === "number"?

@Pomax
Member
Pomax commented Oct 10, 2016

Yeah, that's probably a good idea.

@CapacitorSet
Contributor

Done.

@Pomax
Pomax approved these changes Oct 11, 2016 View changes
@Pomax
Member
Pomax commented Oct 11, 2016

Nice, this looks good to me, but let me verify it when I get back home in about 12 hours

@Pomax
Member
Pomax commented Oct 12, 2016 edited

verified to fix the original issue, so in it goes, nice work and thanks!

On a pull request note, the generally best practice around fixing issues is to checkout master and then create a branch off that that like issue267 or fix-random, making your changes in that branch, and then pushing that up and filing a PR from it rather than master, so that you always have a master branch that you can locally revert to without crazy git commands.

@Pomax Pomax merged commit bfcbc2e into processing-js:master Oct 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment