Skip to content

Implement floating point conversion #110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Implement floating point conversion #110

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b38ff2b
Implement toBiggestFloat
rotu Jul 26, 2022
6c73172
don't use BiggestInt in implementation of toBiggestFloat
rotu Jul 27, 2022
b08bbbe
Add tests for infinity
rotu Jul 27, 2022
7fe47ad
support for other floating point types
rotu Jul 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/bigints.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
## Arbitrary precision integers.

import std/[algorithm, bitops, math, options]
import std/[algorithm, bitops, fenv, math, options]

type
BigInt* = object
Expand Down Expand Up @@ -1140,6 +1140,11 @@ func fastLog2*(a: BigInt): int =
return -1
bitops.fastLog2(a.limbs[^1]) + 32*(a.limbs.high)

func toBiggestFloat*(x: BigInt): BiggestFloat =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func toBiggestFloat*(x: BigInt): BiggestFloat =
func toFloat*[T: SomeFloat](x: BigInt): Option[T] =

IMO there's no reason to special case BiggestFloat (which is float64 currently). The implementation should work the same with 32 bit floats. I also think returning an Option makes more sense, since not every BigInt can be represented as a float (unless we just round).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for the Option (see recently closed PR for BigInt -> integer type conversion).
For the examples, you want to test the saturation values and the one above.
See : https://en.wikipedia.org/wiki/Double-precision_floating-point_format#Double-precision_examples for some values.

Copy link
Contributor

@dlesnoff dlesnoff Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to provide +Inf, -Inf, subnormal values instead of an Option though. I prefer the Option solution, since we lose (almost) all information by converting. The option may warn the user of the conversion problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The reason to special case BiggestFloat is because it's modeled after https://nim-lang.org/docs/system.html#toBiggestFloat%2CBiggestInt
  2. The API for toFloat is ugly since there is no way to deduce the type parameter. I'm avoiding a generic interface since there's no clear guidance on when to use generics versus typedesc parameters. [RFC] guidelines for when to use typedesc vs generics RFCs#40
  3. BigInt to integer is a very, very different case. Floating point math is, by nature, approximate. It is expected to be a lossy conversion, so rounding is the right thing to do here. I added some tests for the Infinite cases.

BTW, in looking into this, I also discovered this bug nim-lang/Nim#20102

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The reason to special case BiggestFloat is because it's modeled after https://nim-lang.org/docs/system.html#toBiggestFloat%2CBiggestInt

I don't see that as a good reason to repeat this here.

  1. The API for toFloat is ugly since there is no way to deduce the type parameter. I'm avoiding a generic interface since there's no clear guidance on when to use generics versus typedesc parameters. [RFC] guidelines for when to use typedesc vs generics RFCs#40

Wether that's ugly or not is quite subjective. I don't see a problem with toFloat[float64](...) (it isn't much more to type than toBiggestFloat). I don't see why we should special case BiggestFloat (aka float64).

  1. BigInt to integer is a very, very different case. Floating point math is, by nature, approximate. It is expected to be a lossy conversion, so rounding is the right thing to do here. I added some tests for the Infinite cases.

That's a fair point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. So why proc toFloat[T:SomeFloat](x: BigInt):T and not proc toFloat(x: BigInt, T: type SomeFloat):T or even proc to(x:BigInt, T:type SomeFloat):T?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because imo you should use generics for passing type parameters. I know that not everyone agrees with this though. If we ever get better type inference (which I really hope we do), the generic parameter here could even be inferred, based on subsequent uses of the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to func to*(x: BigInt, T:type SomeFloat): T

This has the benefit of not repeating "float" in calls (x.toFloat[float32]()).

I didn't use generics for the following reasons:

  1. Generic specialization does not work. (Error: overloaded 'to' leads to ambiguous calls if I try to use generics and add a to[SomeInteger]).
  2. In method call syntax, it's ambiguous with an indexer (x.to[:float32]() leads to an unintuitive error)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I would actually support to implement toBiggestFloat instead of conversion for all types of float.

The reason to special case BiggestFloat is because it's modeled after https://nim- lang.org/docs/system.html#toBiggestFloat%2CBiggestInt

I don't see that as a good reason to repeat this here.

For me that is a good reason. It means having a consistent API across nim ecosystem (and indeed bigints api is modeled after system api). It would not be a good reason if toBiggestFloat in system was originally a bad api (so we should not copy it), but I do not see why that would be the case.

I don't see why we should special case BiggestFloat (aka float64).

Because the use case call for converting to a float and it make sense to convert to biggest float you have.
I do not think we should support conversion to multiple floats, since that would mean that the user would have to think: "which float should I use? why would I want to convert to a smaller float?"

let l = mantissaDigits(BiggestFloat)+1
let shift = max(fastLog2(x) - l, 0)
let mantissa = BiggestFloat(toInt[BiggestInt](x shr shift).get())
result = mantissa * pow(2.0, BiggestFloat(shift))

func invmod*(a, modulus: BigInt): BigInt =
## Compute the modular inverse of `a` modulo `modulus`.
Expand Down
10 changes: 10 additions & 0 deletions tests/tbigints.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,16 @@ proc main() =
doAssert pred(a, 3) == initBigInt(4)
doAssert succ(a, 3) == initBigInt(10)

block: # to float
doAssert toBiggestFloat(initBigInt("0")) == 0.0
doAssert toBiggestFloat(initBigInt("1")) == 1.0
doAssert toBiggestFloat(initBigInt("-1")) == -1.0
doAssert toBiggestFloat(initBigInt(BiggestInt.high)) == BiggestInt.high.toBiggestFloat
doAssert toBiggestFloat(initBigInt(BiggestInt.low)) == BiggestInt.low.toBiggestFloat
doAssert toBiggestFloat(initBigInt(BiggestInt.high) * initBigInt(4)) == BiggestInt.high.toBiggestFloat * 4.0
doAssert toBiggestFloat(initBigInt(BiggestInt.low) * initBigInt(4)) == BiggestInt.low.toBiggestFloat * 4.0
doAssert toBiggestFloat(initBigInt("17976931348623157") * initBigInt(10).pow(292)) == 17976931348623157e292


static: main()
main()