search

basvandijk / scientific

Arbitrary-precision floating-point numbers represented using scientific notation
BSD 3-Clause "New" or "Revised" License
73 stars 40 forks source link

Fractional instance is misleading. #64

Closed xnomagichash closed 6 years ago

xnomagichash commented 6 years ago

I realize this is documented, but it appears unsafe to use any method of Fractional with Scientific. What is the rationale for keeping it as an instance of Fractional in that case?

basvandijk commented 6 years ago

It was added to support floating literals of type Scientific.

xnomagichash commented 6 years ago

I definitely understand the benefits of that.

The trouble is this: we wrote a function that takes Fractional in our library code. An engineer working on app-level code passed a Scientific from an Aeson.Number into the function at the app-level and it type-checked. The engineer had no way of knowing that several layers of abstraction down division was being done.

Instead of raising an error or crashing threads on our server would hang seemingly at random on certain payloads. Debugging this required peeling away layers of abstraction and tracing practically every line of code because a non-termination bug in a multi-threaded application doesn't result in any sort of usable debugging output.

We see no good resolution for this in our code because Aeson uses Scientific so we can't avoid it entirely. The alternative would be to abandon Fractional in our library code and force Double, but we deal with multiple precision in our data so it would be a troubling solution.

I would propose that instead of having non-terminating Fractional methods in Scientific, one of three solutions should be adopted:

  1. Detect non-terminating values and fail.
  2. Fail outright with error on any potentially unsafe method.
  3. Coerce to Double internally for potentially unsafe operations, sacrificing exactness for guaranteed termination.

If any of these is palatable to you we are happy to contribute a patch.

basvandijk commented 6 years ago

I'm sorry to hear this.

I'm beginning to think it's best to just drop the Fractional instance and provide unsafeFromRational :: Rational -> Scientific. We would loose floating literals but that's a small price to pay compared to your debugging time.

basvandijk commented 6 years ago

I just realized that the RealFrac instance requires a Fractional instance. So if I drop the Fractional instance I also have to drop the RealFrac instance. That won't be nice because I think round and some other methods of RealFrac are heavily used.

Maybe it's better to go with your solution 1. For detecting non-termination we can use fromRationalRepetend which detects repeating decimals (which I think is the same as non-termination).

basvandijk commented 6 years ago

e7461bc7a14a33f95b43406923da8cddb0e4f9f6 should do it.

basvandijk commented 6 years ago

fromRational from scientific-0.3.6.1 now throws an error when applied to a repeating decimal.