`round(::Type{<:AbstractFloat}, x, ::RoundingMode)` violates docstring

[mikmoore]

trafficstars

I don't have the development branch checked out right now, so forgive me for manually-interpreting this code and let me know if any of my concerns are due solely to my mistakes. This issue is in response to new functionality introduced in #50812, which has not been included in a release version (as of v1.9).

I take issue with the implementation

round(::Type{T}, x, r::RoundingMode) where T = convert(T, round(x, r))

and the docstrings related to rounding functions. For example, ?floor in #50812 states

floor(x) returns the nearest integral value of the same type as x that is less than or equal to x. floor(T, x) converts the result to type T, throwing an InexactError if the floored value is not representable a T.

(note the minor typo "not representable a T", but that's not why I'm here)

However, under the above implementation we have floor(Float32, 0xffff_ffff) == 2f0^32 > 0xffff_ffff. The floor(0xffff_ffff) part is executed correctly (no-op because integer argument), but the subsequent convert(Float32, 0xffff_ffff) results in upward rounding and yields an invalid result (being larger than the input). By the current docstring, the result should be an InexactError since 0xffff_ffff is not representable as a Float32. Alternatively, one might suggest that the largest not-greater Float32 be returned (in this case prevfloat(2f0^32)) but that would require a docstring change.

Possible solutions include:

• Change the docstring to remark that convert is naively applied to the result of floor(x), so that this behavior is explainable under the stated semantics.
• Throw an InexactError, as the written docstring indicates should happen.
• Remove the unrestricted Type argument from the rounding functions. Retain some special cases like Type{<:Integer} (which already exist in release versions) where such issues cannot arise.
  • On current release v1.9, floor(Float32, 0xffff_ffff) is a MethodError
  • Is floor(T, x) really that much better than T(floor(x)) or convert(T, floor(x))? I'd be happy to have it except that it's wrong here. At least with the latter two the return value is explainable via the rounding behavior of T conversion.
• Adjust the implementation to return prevfloat(floor(T,x)) when floor(T,x) > x and T<:AbstractFloat, with similar changes to other rounding functions. Adjust the docstrings accordingly to accomodate this possibility. Something in the flavor of "return the largest integral-valued T that is less than or equal to x".
  • This might be somewhat expensive since Julia doesn't currently give control over hardware rounding modes (to get the correct result in the first place) and integer-float comparisons aren't cheap (to fix the result in post-processing).

[nsajko]

Should maybe have "correctness bug" label.

[mikmoore]

@LilithHafner forgive the ping, but since the changes discussed here come from your PR and look to be landing in v1.11, I was hoping you could take a glance at this issue in case you have some commentary before a change would be breaking.

[LilithHafner]

Thanks for the ping! I didn't see this before.

Option 1 would be disappointing. I like option 2 Option 3 re-introduces #50778 Option 4 is even better than option 2, but much harder to implement, though it would be possible to implement 2 in 1.10 and 4 in a later 1.x.

I'm going to look into performant options for 4.

[LilithHafner]

Option 2 is easy:

julia> function exact_convert(::Type{T}, x) where T
           t = convert(T, x)
           t == x || throw(InexactError(:exact_convert, T, x))
           t
       end
exact_convert (generic function with 1 method)

julia> @b convert(Float32, $0xffff_ff00)
1.988 ns

julia> @b exact_convert(Float32, $0xffff_ff00)
2.263 ns

julia> exact_convert(Float32, 0xffff_ff00)
4.294967f9

julia> convert(Float32, 0xffff_ff00)
4.294967f9

julia> convert(Float32, 0xffff_ffff)
4.2949673f9

julia> exact_convert(Float32, 0xffff_ffff)
ERROR: InexactError: exact_convert(Float32, 0xffffffff)
Stacktrace:
 [1] exact_convert(::Type{Float32}, x::UInt32)
   @ Main ./REPL[27]:3
 [2] top-level scope
   @ REPL[33]:1

[LilithHafner]

Option 4 is also not that hard. Performance of the naive approach is plenty fast on my computer, and only slows down the round(::Type{<:AbstractFloat}, x, r) case which was previously an error and is now buggy. PR forthcoming.