Base: correctly rounded floats constructed from rationals

[nsajko]

Constructing a floating-point number from a Rational should now be correctly rounded.

Implementation approach:

1. Convert the (numerator, denominator) pair to a (sign bit, integral significand, exponent) triplet using integer arithmetic. The integer type in question must be wide enough.

2. Convert the above triplet into an instance of the chosen FP type. There is special support for IEEE 754 floating-point and for BigFloat, otherwise a fallback using ldexp is used.

As a bonus, constructing a BigFloat from a Rational should now be thread-safe when the rounding mode and precision are provided to the constructor, because there is no access to the global precision or rounding mode settings.

Updates #45213

Updates #50940

Updates #52507

Fixes #52394

Closes #52395

Fixes #52859

[nsajko]

Regarding performance:

1. the ldexp calls are not essential in the cases of the IEEE floating-point formats, ldexp could be replaced with a bit of bit twiddling for those common cases. I can do that now or in a future PR.
2. BigInt should have it's own implementation for performance reasons, although I'm not sure whether it'd be better to put that in Base or in MutableArithmetics.

[Seelengrab]

We have lots of screen space - please, let's use it not just vertically. We are fine with 92 characters per line (sometimes more if it's just a few characters over) after all.

[nsajko]

The second commit is the mentioned optimization of using bit twiddling instead of ldexp for IEEE floats. If you're fine with having it in this PR, I can squash them, otherwise I'll move it to a subsequent PR.

Regarding the 92 character line length limit, does it apply to comments equally? I usually try to have comments be narrower than code, but if you want I'll rewrap them to 92 columns.

[Seelengrab]

Thank you for sticking with this! I haven't added the style points to every occurence, but please apply it uniformly.

We can worry about performance afterwards, since this PR got started with correctness. Talking about potential performance improvements without benchmarks is a bit of a moot point.

[Seelengrab]

Looks good!

I think the gain in correctness is nice, now let's hope this doesn't cost too much performance 😬

[nsajko]

performance

Profiling reveals that a considerable amount of time is spent in the line

y = Rational(promote(numerator(x), denominator(x))...)

The Rational call spends almost all of its time in divgcd, however this is unnecessary and could be avoided if we called unsafe_rational instead. I think this will be the single biggest performance win, but there are other possible optimization opportunities that should be looked into, such as optimizing rational_to_float_components_impl to do just one division and then round manually (like it was before), and the optimization of replacing ldexp with bit-level operations for IEEE floats.

[Seelengrab]

Which cases spend their time in promotion in particular? IIRC this should be a noop if there's nothing to pomote/convert 👀

[nsajko]

Which cases spend their time in promotion in particular? IIRC this should be a noop if there's nothing to pomote/convert

The problem is not promote, it's the Rational call in the same line. I already replaced it with unsafe_rational locally, to avoid the costly but unnecessary divgcd.

[nsajko]

JET shows that this line causes run time dispatch, I think because the type of rm is unknown:

julia/base/mpfr.jl

Line 329 in dd48fdd

c = rational_to_float_components(num, den, prec, BigInt, rm)

││││┌ @ mpfr.jl:329 Base.MPFR.rational_to_float_components(%18, %27, prec, Base.MPFR.BigInt, %102)
│││││ runtime dispatch detected: Base.MPFR.rational_to_float_components(%18::Int64, %27::Int64, prec::Int64, Base.MPFR.BigInt, %102::RoundingMode)::Base.FloatComponentsResult{BigInt, Int64}

There doesn't seem to be a way around this without introducing a new type for roundings and using it in rational_to_float_components_impl instead of RoundingMode. This doesn't seem too difficult but it may be overkill?

[Seelengrab]

This is just for BigFloat, right? I don't think this dispatch will be particularly problematic then - any computation involving BigFloat of sizes/precisions where it's worth using BigFloat instead of something like Float128 will probably be bottlenecked by MPFR/GMP operations under the hood instead of this dispatch (not to mention the other allocating BigFloat/BigInt operations in here).

[nsajko]

Is there still something to do here from my side?

[nsajko]

Another small optimization would be using ndigits0zpb instead of ndigits. Not sure if I should do this now or wait until this PR is merged. EDIT: probably top_set_bit would be even better than ndigits.

[Seelengrab]

Other than these, yeah, looks good from my POV 👍

[Joel-Dahne]

Let me try to take a closer look at this today!

Regarding the comment

do we actually care about giving the user the ability to specify rounding modes here? Most other functions in Base don't, and giving up that seems like it would simplify the code a lot.

Indeed most other functions in Base don't allow you to control the rounding, and I don't believe they should! Getting rounding correct is difficult (hence this PR) and it would probably fit better in a separate package. However, Base current does support rounding when converting to Float64, e.g. Float64(1 // 3, RoundUp). Since it is already supported I think it is a valid goal to also make it correct! For example this is used in IntervalArithmetic.jl when converting a rational interval to a Float64 interval.

[nsajko]

Wow! I had no idea the rounding mode argument was already supported for converting rationals to floats! I always assumed the rounding mode argument was only accepted when converting to BigFloat. Then clearly:

1. We do need the rounding modes to be supported explicitly in this PR
2. The PR needs to actually override the rounding mode methods

[Joel-Dahne]

I would also propose to say that this change makes constructing a float from a rational "correctly rounded", instead of "exact". It seems more clear to me at least, since most rationals are not exactly representable as floats.

[Joel-Dahne]

In general I think the change looks good! As far as I can tell the most important methods are to_float_components, to_floating_point_fallback and the to_floating_point_impl implementation for machine floats. The rest is more or less gluing code and tests (both of which there are a lot of).

In general the coding style is quite different from my personal one. That is of course fine, but I have some general comments:

• Often times temporary names are introduced only to be used once. For example the exp variable in to_floating_point_fallback.
• There are a lot of let-blocks that I don't really see the point of. For example in to_floating_point_fallback again.
• There are many type annotations that I don't really see the point of. To take to_floating_point_fallback as an example again, surely the compiler can verify zero(T)::T without the type annotation?

[Joel-Dahne]

Here I think it would be natural to make an early return on these checks. So replace them with

num_is_zero & !den_is_zero && return zero(T)
!num_is_zero & den_is_zero && return sb * T(Inf)
num_is_zero & den_is_zero && return T(NaN)

That removes the need for the if-statement further down and , which, for me, simplifies the flow.

Similar things could be done in the other implementations, though it makes less of a difference there.

[nsajko]

I prefer to avoid return when possible. It's a jump/goto statement, breaking nice control-flow properties known as structured programming. I do use jumps in control flow, but only when necessary.

[nsajko]

I used to be a Go nut (as in "Golang aficionado"), so I see where you're coming from, though.

[Joel-Dahne]

It's only a matter of style of course. I'm not going to fight you over it 😄

[Joel-Dahne]

This variable is only used once, I think it is more natural to just use the union directly. Though in this case I guess it does make the method declaration below slightly too long to fit on one line.

[nsajko]

My reasoning here was basically:

1. I'm not exactly sure what's the preferred way to break lines (or format code in general) in Julia source in the Julia project, so I prefer to avoid it completely
2. Adding a constant is basically free (as long as it's not huge, doesn't mess with precompilation somehow, etc.)

[Joel-Dahne]

I understand, it also explains some of the style of code in other places!

[Joel-Dahne]

I also made a quick benchmark comparing this implementation for BigFloat with the one I proposed above. With the version in this PR I get

julia> @benchmark BigFloat($(5 // 7))
BenchmarkTools.Trial: 10000 samples with 130 evaluations.
 Range (min … max):  743.838 ns …  3.561 ms  ┊ GC (min … max):  0.00% … 31.21%
 Time  (median):       1.072 μs              ┊ GC (median):     0.00%
 Time  (mean ± σ):     1.920 μs ± 46.631 μs  ┊ GC (mean ± σ):  18.36% ±  0.86%

   ▄▆▄▂ ▁    ▃▆▇▇█▅▃▁▁  ▁▁    ▁▁ ▁▁  ▂▁        ▂▂    ▁▁▁▁      ▂
  ███████▇▆▅███████████████▆▆██████▇████▇▇▇▇▆▅▇███▆▆▇█████▇▇▅▇ █
  744 ns        Histogram: log(frequency) by time      2.09 μs <

 Memory estimate: 1.09 KiB, allocs estimate: 40.

julia> @benchmark BigFloat($(big(2)^200 // big(3)^200))
BenchmarkTools.Trial: 10000 samples with 63 evaluations.
 Range (min … max):  792.905 ns …  4.627 ms  ┊ GC (min … max):  0.00% … 49.30%
 Time  (median):       1.005 μs              ┊ GC (median):     0.00%
 Time  (mean ± σ):     1.959 μs ± 57.511 μs  ┊ GC (mean ± σ):  20.56% ±  0.71%

  ▁▄▄▄▆▇▆▃▃▆█▇▆▄▂ ▂▁▁▄▄   ▁▁▂▂▂▂▂▃▂▂▂        ▁▁▂▁▂▂▂▂▂▂▂▂▁▁    ▂
  ██████████████████████▇▇████████████▇▅▅▄▅▅▇███████████████▇▇ █
  793 ns        Histogram: log(frequency) by time      1.97 μs <

 Memory estimate: 1.16 KiB, allocs estimate: 38.

With my version I get

julia> @benchmark Base.MPFR.__BigFloat($(5 // 7))
BenchmarkTools.Trial: 10000 samples with 968 evaluations.
 Range (min … max):   78.898 ns …  27.099 μs  ┊ GC (min … max):  0.00% … 99.52%
 Time  (median):      88.857 ns               ┊ GC (median):     0.00%
 Time  (mean ± σ):   102.076 ns ± 454.203 ns  ┊ GC (mean ± σ):  12.20% ±  2.97%

            ▁▂▂▂▁       ▃█▁▂▆▄                                   
  ▂▃▅▆▆▇▇▇████████▇▅▄▄▃▄██████▇▇█▅▄▄▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▁▂▂▂▂▂▂▂▂ ▄
  78.9 ns          Histogram: frequency by time          111 ns <

 Memory estimate: 184 bytes, allocs estimate: 4.

julia> @benchmark Base.MPFR.__BigFloat($(big(2)^200 // big(3)^200))
BenchmarkTools.Trial: 10000 samples with 204 evaluations.
 Range (min … max):  376.975 ns … 498.437 μs  ┊ GC (min … max): 0.00% … 44.14%
 Time  (median):     410.777 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   551.419 ns ±   6.129 μs  ┊ GC (mean ± σ):  6.40% ±  0.58%

    ▁▆█▄                                                         
  ▃▅████▅▃▂▂▂▃▆█▆▄▃▅▃▃▂▂▂▃▂▂▂▂▂▃▃▃▃▃▂▂▂▂▂▂▂▁▂▂▂▁▂▂▂▂▂▂▁▂▂▁▂▁▁▂▂ ▃
  377 ns           Histogram: frequency by time          764 ns <

 Memory estimate: 368 bytes, allocs estimate: 10.

So 10 times faster for machine size integers and 2 times faster for large integers. Even more if one counts the GC time, which one probably should for BigFloat.

I don't think these performance gains are essential though, either version would be fine I think.

[nsajko]

Often times temporary names are introduced only to be used once. For example the exp variable in to_floating_point_fallback.

Got rid of exp, thanks.

There are a lot of let-blocks that I don't really see the point of. For example in to_floating_point_fallback again.

Yeah, some of the lets were definitely redundant. There's less of them now. Some of the remaining ones are because I wanted to give a local scope to if blocks that have their own variables, as the lack of a new scope can be surprising there.

There are many type annotations that I don't really see the point of. To take to_floating_point_fallback as an example again, surely the compiler can verify zero(T)::T without the type annotation?

There is no way to declare a return type for a function (as in, all the function's methods) in Julia. We don't even have a guarantee for the return type of constructors (#42372)! Thus I use a type assertion after calling some function whenever I can to catch bugs earlier, and to help contain possible type-instability (which can sometimes be introduced by the caller). Note that Julia is good with eliminating type assertions based on type inference, so they're basically free when they're not helpful.

So 10 times faster for machine size integers and 2 times faster for large integers.

Thanks, I'll profile this and potentially follow up.

[Joel-Dahne]

Sounds good! I don't have any further comments at this point!

[nsajko]

Part of the reason for this PR being slow for bignums is that the current div and divrem (not touched by this PR) are unnecessarily slow. Looking at the allocation profiler, divrem(::BigInt, ::Int, ::RoundingMode{:ToZero}) is really slow and allocates too much. It calls div, which does an unnecessary promotion on the denominator, then divrem does unnecessary copying arithmetic. And GMP actually provides mpz_tdiv_q_ui that directly calculates both the quotient and the remainder. There's a bunch more examples like this, but that's clearly a matter for a separate PR. To make matters worse, touching base/div.jl seems fraught with peril, as the dispatch situation between all the different function variants (div, rem, divrem, fld, cld, fldmod) is horrifying...

[oscardssmith]

I think the base/div.jl issues are significantly improved recently.

[nsajko]

Fixed conflict. Could this be merged as per #53641 (comment) (caused by correctness fix, and the performance regression mainly applies to bignums)? The performance for bignums can be fixed later, this PR is already quite large

[nsajko]

The test failures are real.

[DilumAluthge]

Is this PR still desired? If so, @nsajko can you rebase and fix the merge conflicts?

[vtjnash]

Looks like the conflicts are all superficial (just deciding whether to place the new functions before or after other new functions), and this seems already approved so you can add merge me tag after rebasing?

[DilumAluthge]

I do see that @LilithHafner previously requested changes, so I think it'd be good to get a re-review to make sure those requests were addressed.