Add shouldBeNear expectation by erikd · Pull Request #23 · hspec/hspec-expectations

erikd · 2016-08-17T08:18:43Z

Comparing floating point numbers for equalty is a bad idea. Instead
they should tested to see if they are close enough to one another.

erikd · 2016-09-03T02:27:28Z

Ping?

sol · 2016-09-03T03:11:14Z

Hey, sorry for the delay. For this to be useful this would at least need tests and more detailed documentation. Specifically I don't understand

why is the first case necessary
what happens on division by zero, e.g. when comparing 1e-20 and -1e-20

erikd · 2016-09-03T08:53:18Z

For this to be useful this would at least need tests

There's no tests listed in the cabal file. I've found the tests in the repo and I'm adding tests, but how do I run them?

why is the first case necessary

This is short circuit evaluation for the case where they are actually equal.

what happens on division by zero, e.g. when comparing 1e-20 and -1e-20

The code for the third case says (abs actual + abs expected) which could only be zero if both values are zero and that would be caught be the second case.

sol · 2016-09-03T14:16:44Z

I use ghci or sensei to run tests

Sent from mobile

On 3 Sep 2016, at 4:53 PM, Erik de Castro Lopo notifications@github.com wrote:

For this to be useful this would at least need tests

There's not tests listed on the cabal file. How do I run them?

why is the first case necessary

This is short circuit evaluation for the case where they are actually equal.

what happens on division by zero, e.g. when comparing 1e-20 and -1e-20

The code for the third case says (abs actual + abs expected) which could only be zero if both values are zero and that would be caught be the second case.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

sol · 2016-09-06T07:02:00Z

Tests are running on Travis now. Can you rebase on current master and make sure the build is green.

I also wouldn't mind if you remove some of the code duplication, e.g. by factoring common stuff out into a where-clause.

Then I would love to discuss the actual stuff, that is the formula and the chosen value for epsilon.

erikd · 2016-09-06T09:02:59Z

Ok, the build has passed.

Since the current code uses guards, its not possible to refactor common code into a where clause, but I could make it a separate top level function (and then not export it).

sol · 2016-09-06T09:16:53Z

I think you can use a where clause with guards, no?

Sent from mobile

On 6 Sep 2016, at 5:03 PM, Erik de Castro Lopo notifications@github.com wrote:

Ok, the build has passed.

Since the current code uses guards, its not possible to refactor common code into a where clause, but I could make it a separate top level function (and then not export it).

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

erikd · 2016-09-06T09:28:55Z

It works! Fixed, rebased and pushed.

erikd · 2016-09-06T11:17:43Z

For some reason I couldn't get one the tests working with ghc 7.10 even though it passed on all the other versions.

sol · 2016-09-07T04:10:57Z

      src
  ghc-options: -Wall
+  if impl(ghc >= 8.0)
+    ghc-options: -fno-warn-redundant-constraints


I fixed this on master, so this is no longer necessary.

Ok, dropped that patch.

sol · 2016-09-07T04:39:47Z

+    -- If they actually are exactly equal we're done.
+    | actual == expected = reportFail (actual == expected)
+    -- If either input is zero, we check if the absolute difference is less than epsilon..
+    | actual == 0 || expected == 0 = reportFail (abs (actual - expected) < 1e-15)


source code comments are an anti-pattern, rather than commenting make your code more expressive, e.g.

otherwise = reportFail (relativeDifference < epsilon)

Agreed, that is neater.

sol · 2016-09-07T05:03:41Z

+    | actual == 0 || expected == 0 = reportFail (absDifference < epsilon)
+    | otherwise =  reportFail (relativeDifference < epsilon)
+    where
+        epsilon = 1e-15


for me it's much easier to work with code that does not use abbreviations, the reason for this is that I have to use speech recognition for programming due to RSI. for that reason I would much prefer absoluteDifference here

sol · 2016-09-07T06:22:24Z

+actual `shouldBeNear` expected
+    -- Short circuit if they are actually equal.
+    | actual == expected = reportFail (actual == expected)
+    | actual == 0 || expected == 0 = reportFail (absoluteDifference < epsilon)


now finally regarding the interesting stuff, the formula, looking at the following example

x `shouldBeNear` 1e-300

I haven't tried, but what happens if x is

0.9e-300

0

would (1) fail and (2) pass?

should we always normalize against the expected value instead?

actual `shouldBeNear` expected | delta < epsilon = return () | otherwise = actual `shouldBe` expected where epsilon = 1e-15 absoluteDifference = abs (expected - actual) relativeDifference = absoluteDifference / abs expected delta | expected == 0 = absoluteDifference | otherwise = relativeDifference

I haven't tried, but what happens if x is

0.9e-300

0

would (1) fail and (2) pass?

That is what currently happens, and I agree, at first sight, doesn't make much sense, but this is the usual recommended way to compare floating point numbers.

However, now that I think about it,

1e-300 `shouldBeNear` 0.0

should pass (because actual is close to expected), but I think maybe

0.0 `shouldBeNear` 1e-300

should fail.

Let me think about this some more.

I have encoded my expectations in the tests.

Comparing floating point numbers for equalty is a bad idea. Instead they should tested to see if they are close enough to one another.

erikd · 2016-09-15T08:22:56Z

@sol Does this seem ok?

declension · 2020-11-07T09:40:31Z

+    absoluteDifference = abs (expected - actual)
+    relativeDifference = absoluteDifference / (abs actual + abs expected)
+    reportFail =
+      expectTrue ("expected: " ++ show expected ++ "\n but got: " ++ show actual)


+1 on this change BTW!

Wouldn't it be nicer to report things like expected 123 ± 1% but got 125 or similar, as the expectation is of it being within a tolerance of the value not being the value?

istathar · 2022-03-16T04:15:00Z

@erikd is this still a valid approach or have you found a different way to tackle comparing Doubles?

erikd · 2022-03-16T05:52:12Z

@istathar I stopped using hspec about 6 years ago, in favor of hedgehog. However, I suspect this may still be a valid approach.

erikd mentioned this pull request Aug 17, 2016

Advice on comparing Double hspec/hspec#267

Closed

erikd mentioned this pull request Sep 4, 2016

Can't run the tests #25

Closed

erikd force-pushed the master branch 2 times, most recently from c5210c1 to f8582f5 Compare September 4, 2016 07:10

erikd force-pushed the master branch 2 times, most recently from dec8922 to 8e4cc5a Compare September 6, 2016 08:28

erikd force-pushed the master branch from 8e4cc5a to bae7936 Compare September 6, 2016 09:28

erikd force-pushed the master branch 3 times, most recently from e43ce35 to 69bfa94 Compare September 6, 2016 10:09

sol reviewed Sep 7, 2016
View reviewed changes

erikd force-pushed the master branch 2 times, most recently from 263f489 to 4c7b60b Compare September 7, 2016 04:25

sol reviewed Sep 7, 2016
View reviewed changes

erikd force-pushed the master branch from 4c7b60b to c31526a Compare September 7, 2016 04:55

sol reviewed Sep 7, 2016
View reviewed changes

erikd force-pushed the master branch from c31526a to 9a4788b Compare September 7, 2016 05:21

sol reviewed Sep 7, 2016
View reviewed changes

Add shouldBeNear expectation

8ae0dbe

Comparing floating point numbers for equalty is a bad idea. Instead they should tested to see if they are close enough to one another.

erikd force-pushed the master branch from 9a4788b to c7705b9 Compare September 8, 2016 10:09

erikd added 2 commits September 8, 2016 20:11

Add tests for shouldBeNear

84aec40

.travs.yml: Use ghc-7.10.3 instead of 7.10.2

a10d9d2

erikd force-pushed the master branch from c7705b9 to a10d9d2 Compare September 8, 2016 10:12

declension reviewed Nov 7, 2020

View reviewed changes

Base automatically changed from master to main January 21, 2021 03:48

Uh oh!

Conversation

erikd commented Aug 17, 2016

Uh oh!

erikd commented Sep 3, 2016

Uh oh!

sol commented Sep 3, 2016

Uh oh!

erikd commented Sep 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sol commented Sep 3, 2016

Uh oh!

sol commented Sep 6, 2016

Uh oh!

erikd commented Sep 6, 2016

Uh oh!

sol commented Sep 6, 2016

Uh oh!

erikd commented Sep 6, 2016

Uh oh!

erikd commented Sep 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sol Sep 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikd commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istathar commented Mar 16, 2022

Uh oh!

erikd commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erikd commented Sep 3, 2016 •

edited

Loading

sol Sep 7, 2016 •

edited

Loading

erikd commented Mar 16, 2022 •

edited

Loading