Skip to content

Fix bug when rounding large numbers to floating point types #54314

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

Merged
merged 7 commits into from
May 16, 2024

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner added bugfix This change fixes an existing bug backport 1.11 Change should be backported to release-1.11 labels Apr 30, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: mikmoore <[email protected]>
@KristofferC KristofferC mentioned this pull request May 6, 2024
59 tasks

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@LilithHafner
Copy link
Member Author

CI is very red, but all of the failures look very unrelated (internet or file system) and master has been red for a while.

@LilithHafner LilithHafner merged commit e7a1def into master May 16, 2024
@LilithHafner LilithHafner deleted the lh/round-to-float branch May 16, 2024 11:50
@mikmoore
Copy link
Contributor

Oops. Only now did I realize that a docstring update may still be needed. I expect there are situations where an InexactError may still be thrown (or could be for some other types), so that part should still be there, but the current docstrings don't address the new behavior introduced in this PR.

@LilithHafner
Copy link
Member Author

I expect there are situations where an InexactError may still be thrown

I don't think so

julia> Base.infer_effects(round, Tuple{Type{Float64}, Int})
(+c,+e,+n,+t,+s,+m,+u)

or could be for some other types

Yes:

julia> floor(UInt8, 1000)
ERROR: InexactError: trunc(UInt8, 1000)

We also have

floor(T, x) converts the result to type T, throwing an InexactError if the floored value is not representable a T.

in the docstring already.


the current docstrings don't address the new behavior introduced in this PR.

Do you have a specific behavior that isn't documented? For the most part I think this PR is doing a bugfix to change behavior to match what was already quasi-documented.

@mikmoore
Copy link
Contributor

mikmoore commented May 16, 2024

Sorry. You're right. It's all covered. I just looked at this first thing in the morning and my brain wasn't working. It talks about rounding to representable values.

KristofferC pushed a commit that referenced this pull request May 20, 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 <[email protected]>
(cherry picked from commit e7a1def)
KristofferC pushed a commit that referenced this pull request May 23, 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 <[email protected]>
(cherry picked from commit e7a1def)
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label May 28, 2024
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Jul 12, 2024
…g#54314)

- fix JuliaLang#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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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