purescript-contrib / purescript-rationals

Rational numbers for PureScript
MIT License
13 stars 13 forks source link

Avoid encouraging use of Ratio Int #33

Closed hdgarrood closed 1 year ago

hdgarrood commented 4 years ago

Ratio Int is much more susceptible to Int overflow than it might first appear, because numerators and denominators of operands are multiplied before reducing.

For example, 4.444444 + 5.555555 comes out as -1.158570352349116 with Rational arithmetic, because of overflow:

https://try.purescript.org/?gist=43e97dbd483518af42fe176e778e5ffb

With this number of decimal places, Number still performs very well; when performing the same calculation with Number, the result is within 1e-16 of the exact answer.

This doesn't just affect small numbers with more decimal places, it affects large integers too. For example, 100,000 squared comes out as 1,410,065,408 when performed with Ratio Int.

This library encourages the user to use Ratio Int via the Rational type synonym. I think Ratio Int is likely to be worse than Number in many cases, so I don't think it makes sense to use Ratio with anything other than an arbitrary precision integer type. I'd suggest:

nwolverson commented 3 years ago

It's also possible to avoid some overflow by not simply performing operations and reducing, e.g.

a/b * c/d = (a/gcd_ad)*(c/gcd_bc) / ((d/gcd_ad)*(b/gcd_bc))

See e.g. rust num-rational for this approach.

(I find the example of multiplying 100,000 is a little misleading, as this is simply saying Ratio Int is not better than Int - I guess the point is that the type alias obscures this, though arguably Int itself does the same; I'd actually suggest removing the alias or making the underlying type part of the alias/newtype if it's still useful)

gbagan commented 2 years ago

Is there a plan to integrate this change? I agree with @hdgarrood and think Rational should be of type Ratio BigInt (no idea if it should be a newtype or not).

newlandsvalley commented 2 years ago

When I submitted the PS 0.14 PR, I discussed this a little with @anttih - see https://github.com/anttih/purescript-rationals/pull/35. He seemed keen to get it done then, and we introduced BigInts to the tests as a first step.

anttih commented 2 years ago

I have no objections to doing this other than that I've very much liked that the implementation is tiny and doesn't have a lot of dependencies.

f-f commented 1 year ago

Fixed in #42