Fix Test.assert(Not)ApproxEquals

[DonaldKellett]

Fix Test.assert(Not)ApproxEquals

This pull request fixes codewars/codewars.com#962

Summary

This pull request only affects the assert(Not)ApproxEquals methods of the JavaScript CW-2 testing framework. New, relevant tests have been added to test/runners/javascript_spec.js right below the original Test.assert(Not)ApproxEquals tests which have passed locally.

Concerns

Despite the changes proposed in this PR pass all the new tests included (and all the old ones as well), it concerns me that the code for Test.assertNotApproxEquals is now no longer directly comparable to that of Test.assertApproxEquals which leads me to believe that there may be a logical flaw in either one of these methods. By that, I mean that Test.assertApproxEquals now has a simple if/else conditional but Test.assertNotApproxEquals now has an if/else if/else conditional. Please help me review the new tests added to ensure that they are rigorous enough before merging this PR.

Further Considerations

Apart from the improvements proposed by this PR, please consider the following before merging this PR:

• Currently, Test.assert(Not)ApproxEquals may be considered bugged whenever the expected (or unexpected) value is very small. For example, if the (un)expected value is 1e-9 then the assertion will expect the actual value to be exactly 1e-9 as well since the relative error is 1e-9 which means the absolute error range is 1e-9 * 1e-9 === 1e-18 but JavaScript float precision only allows for floats down to 1e-15. In such edge cases, perhaps using 1e-9 as an absolute error range would be more suitable? If so, I would recommend defining such “edge cases” as whenever Math.abs((un)expected) <= 1.

[kazk]

Doing floating point comparison correctly is extremely hard and has lots of edge cases :( It's also nearly impossible to be robust while keeping the interface easy to use. I usually write my own with adjustments depending on tested values.

Please help me review the new tests added to ensure that they are rigorous enough before merging this PR.

I assumed that these are just for convenience and didn't consider robustness in last PR.
How far are these supposed to cover?
I think this one still fails on:

• Infinities
• How to handle expected NaN?
• Near zeros

(If you want to go super deep, you can use DataView and ArrayBuffer to compare bit patterns. I did this few years ago just for fun :))

---

This is just my opinion and I might have missed something, but the implementation looks too complicated. Why not just negate?

function isAlmostEqual(a, b, relError) {
  // handle a == b
  // handle near zeros
  // compare with relative error
}

function assertApproxEquals(actual, expected, msg, options) {
  const s = 'Expected ... to ...';
  Test.expect(isAlmostEqual(actual, expected, 1e-9), Test.display.message(s, msg), options);
}
function assertNotApproxEquals(actual, unexpected, msg, options) {
  const s = 'Expected ... not to ...';
  Test.expect(!isAlmostEqual(actual, expected, 1e-9), Test.display.message(s, msg), options);
}

---

Some links that I've used as references before:

• The Floating-Point Guide - Comparison
  • NearlyEqualsTest.java
• Theory behind floating point comparisons (boost)
• What every computer scientist should know about floating-point arithmetic (PDF)
• Source of D std.math

[DonaldKellett]

First of all, many thanks for the links; I have just read 2 of them 😄

Doing floating point comparison correctly is extremely hard and has lots of edge cases :(

I fully concur. I previously thought that the floating point assertion as described in the Codewars Codex was already as good as it gets but then I forgot that the expected value generated in random tests might be dangerously close to 0 but not exactly 0 (as intended).

I think this one still fails on:

• Infinities
• How to handle expected NaN?
• Near zeros

AFAIK this assertion method is not intended for comparing against Infinity or NaN so hope you don't mind if I do not cover these 2 edge cases in this PR. However, for near zeros, I'll devise another method to compare them effectively (described later).

This is just my opinion ... [with code example]

I've just skimmed through two of the links you have provided me at the bottom of your comment and the current Test.assert(Not)ApproxEquals implementation already follows most of what those 2 links describe (well, except for near zeros, of course).

For near zeros, I would like to propose the following implementation:

• If Math.abs((un)expected) <= 1 then treat 1e-9 as an absolute error margin, e.g. if expected === 1e-8 then accept all values in the range 1e-8 +- 1e-9
• Otherwise, if Math.abs((unexpected)) > 1 then treat 1e-9 as a relative error margin and scale it up as necessary, e.g. when expected === 1000000 then accept all values in the range 1000000 +- 1e-3

@kazk What do you think?

[kazk]

AFAIK this assertion method is not intended for comparing against Infinity or NaN so hope you don't mind if I do not cover these 2 edge cases in this PR.

I don't mind as long as it's intentional and documented. I just don't want confused users.
See if @jhoffner agrees with your decisions.

This is just my opinion ... [with code example]

I've just skimmed through two of the links you have provided me at the bottom of your comment and the current Test.assert(Not)ApproxEquals implementation already follows most of what those 2 links describe (well, except for near zeros, of course).

I meant for the if/else vs if/else if/else concern you had. I thought you can use a function to implement both?

For near zeros, I would like to propose the following implementation:

Using absolute tolerance is most common approach when comparing against zero as far as I know. I'm not sure how good you want to make this, but my knowledge is limited in this area so I hope someone else can help you improve them.

---

Can you change the title of this PR to include Test.assertApproxEquals? Descriptive title helps when viewing the list of PRs.

[DonaldKellett]

I don't mind as long as it's intentional and documented.

Sure 😄 I'd just like to know when and where I can contribute to the new Docs.

See if @jhoffner agrees with your decisions

I'll take the liberty of going ahead and committing the changes I proposed; if he disagrees then I can always make another commit 😉 (but I won't "spam" this PR with many useless commits, don't worry)

I meant for the if/else vs if/else if/else concern you had. I thought you can use a function to implement both?

Ah right, I see, your code example looks a bit cleaner indeed. But once I make the proposed changes, my if/else versus if/else if/else issue should be fixed and I doubt a few lines of similar code warrants an entire test method to be created.

Can you change the title of this PR to include Test.assertApproxEquals? Descriptive title helps when viewing the list of PRs.

Done 😄

[DonaldKellett]

@kazk @jhoffner Did you recently change some of the rules in ESLint? All the images are failing due to dozens upon dozens of indentation errors (and some unnecessary escape characters) and most of the files affected aren't even modified in this PR.

[kazk]

Sure 😄 I'd just like to know when and where I can contribute to the new Docs.

It's waiting for @jhoffner's decision on licensing and review.

Did you recently change some of the rules in ESLint?

I think that's caused by ESLint updating to 4.0.0. I should've version locked that :(
I'll look into them. Thanks for pointing it out!

[kazk]

@DonaldKellett

Opened #437 locking ESLint version.
Travis will still fail for you, but don't worry if it's ESLint errors.

[DonaldKellett]

Bump

@kazk Would you mind helping me restart the Travis CI build on this one and request @jhoffner to approve it if it passes this time? The previous build failed due to ESLint errors that are unrelated to the changes proposed in this pull request. Thank you very much 😄

[OverZealous]

@DonaldKellett I restarted it for you. :-)

[OverZealous]

Looks like that's not going to help—you just need to merge in the latest master and push it back up (or rebase it, if you prefer).

[DonaldKellett]

Many thanks for the help @OverZealous - I'll try to do git merge origin/master and see if that helps 😄

UPDATE: When I attempted git merge origin/master, I was sent to the Git Editor to add my commit message and I didn't know how to save the message and exit the editor so I did the "merge" through GitHub instead which may have resulted in an extra (unnecessary) commit. Furthermore, when I was resolving the conflicts, I accidentally added a newline to one of the files so I had to make a third commit on top of that to remove the newline that I just added. I apologise if this pull request is starting to look a little bit messy now ☹️