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

fix rounding edge cases on cld, fld, and rem #45910

Merged
merged 3 commits into from
Jul 5, 2022
Merged

Conversation

t-bltg
Copy link
Contributor

@t-bltg t-bltg commented Jul 3, 2022

cc @eschnett for removal of cld def introduced in #8111.

Two tests marked broken before are now passing at

@test rem(T(4), floatmin(T) * 2, mode) == 0 broken=(T == BigFloat && mode in (RoundUp,RoundFromZero))

One test had its sign modified (0.0 -> -0.0): isequal(rem(nextfloat(typemin(T)), T(2), RoundUp), -0.0).

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

This feels like two separate PRs, one fixes cld and the other is whitespace.

base/div.jl Outdated Show resolved Hide resolved
base/div.jl Outdated Show resolved Hide resolved
base/div.jl Outdated Show resolved Hide resolved
@t-bltg
Copy link
Contributor Author

t-bltg commented Jul 4, 2022

This feels like two separate PRs, one fixes cld and the other is whitespace.

Thanks, I've rebased the PR so that one commit denotes the actual fix and the second one contains whitespace, style & docstring modifications: I don't think splitting this into two PRs is needed.

@t-bltg t-bltg force-pushed the cld branch 2 times, most recently from 80857c9 to e6360e2 Compare July 4, 2022 09:18
@t-bltg t-bltg closed this Jul 4, 2022
@t-bltg t-bltg reopened this Jul 4, 2022
@t-bltg t-bltg marked this pull request as draft July 4, 2022 13:26
@t-bltg t-bltg marked this pull request as ready for review July 4, 2022 14:41
@t-bltg t-bltg changed the title fix cld fix div Jul 4, 2022
@t-bltg t-bltg force-pushed the cld branch 2 times, most recently from ad3fdcc to 6b5afd0 Compare July 4, 2022 17:19
t-bltg and others added 2 commits July 4, 2022 23:49
Co-authored-by: Lilith Orion Hafner <60898866+LilithHafner@users.noreply.github.com>
test/numbers.jl Outdated Show resolved Hide resolved
Co-authored-by: Lilith Orion Hafner <60898866+LilithHafner@users.noreply.github.com>
@LilithHafner
Copy link
Member

Pending CI, this looks good to me. Do you think it is ready to merge?

@t-bltg
Copy link
Contributor Author

t-bltg commented Jul 5, 2022

Pending CI

I keep re-triggering CI, but unrelated spurious failures keep showing up :/.

Do you think it is ready to merge?

Yes.

@LilithHafner LilithHafner changed the title fix div fix rounding edge cases on cld, fld, and rem Jul 5, 2022
@LilithHafner LilithHafner merged commit bac82a9 into JuliaLang:master Jul 5, 2022
@LilithHafner
Copy link
Member

The CI failure seems unrelated to me.

Thanks for your contribution! It's nice to see # FIXME: The broken case erroneously returns -Inf get removed.

@t-bltg t-bltg deleted the cld branch July 5, 2022 23:42
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
Also, improve operator spacing and docstring formatting
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
Also, improve operator spacing and docstring formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange cld behaviour
2 participants