Skip to content
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.

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

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

Closed
mikmoore opened this issue Nov 30, 2023 · 5 comments · Fixed by #54314
Closed

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

mikmoore opened this issue Nov 30, 2023 · 5 comments · Fixed by #54314
Labels
domain:maths Mathematical functions kind:bug Indicates an unexpected problem or unintended behavior kind:correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing

Comments

@mikmoore
Copy link
Contributor

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).
@mikmoore mikmoore changed the title round(T, x, ::RoundingMode) violates written semantics round(::Type{<:AbstractFloat}, x, ::RoundingMode) violates written semantics Nov 30, 2023
@mikmoore mikmoore changed the title round(::Type{<:AbstractFloat}, x, ::RoundingMode) violates written semantics round(::Type{<:AbstractFloat}, x, ::RoundingMode) violates docstring Nov 30, 2023
@nsajko
Copy link
Contributor

nsajko commented Dec 24, 2023

Should maybe have "correctness bug" label.

@mikmoore
Copy link
Contributor Author

@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.

@nsajko nsajko added kind:correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing kind:bug Indicates an unexpected problem or unintended behavior domain:maths Mathematical functions labels Apr 29, 2024
@LilithHafner
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

LilithHafner added a commit that referenced this issue May 16, 2024
- fix #52355 using option 4 (round to nearest representable integer)
- update docstrings *including documenting convert to Inf behavior even
though Inf is not the "closest" floating point value*
- add some assorted tests

---------

Co-authored-by: mikmoore <95002244+mikmoore@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:maths Mathematical functions kind:bug Indicates an unexpected problem or unintended behavior kind:correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants