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

Refactor rounding (including bugfixes) and document API #50812

Merged
merged 19 commits into from
Aug 31, 2023

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner commented Aug 6, 2023

This PR implements and documents a cohesive, extensible rounding interface. The code that implements it is in base/rounding.jl

trunc(x; kwargs...) = round(x, RoundToZero; kwargs...)
floor(x; kwargs...) = round(x, RoundDown; kwargs...)
 ceil(x; kwargs...) = round(x, RoundUp; kwargs...)
round(x; kwargs...) = round(x, RoundNearest; kwargs...)

trunc(::Type{T}, x; kwargs...) where {T} = round(T, x, RoundToZero; kwargs...)
floor(::Type{T}, x; kwargs...) where {T} = round(T, x, RoundDown; kwargs...)
 ceil(::Type{T}, x; kwargs...) where {T} = round(T, x, RoundUp; kwargs...)
round(::Type{T}, x; kwargs...) where {T} = round(T, x, RoundNearest; kwargs...)

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

And an explanation of how to use it is in docs/src/manual/interfaces.md.

Fixes #50778
Fixes #42060, though maybe this is a bad thing.
Closes #47128
Fixes #51113

This PR also refactors some implementations of rounding for concrete types to use this new interface more naturally.

@LilithHafner LilithHafner added docs This change adds or pertains to documentation maths Mathematical functions bugfix This change fixes an existing bug labels Aug 6, 2023
@oscardssmith oscardssmith added the triage This should be discussed on a triage call label Aug 6, 2023
@oscardssmith
Copy link
Member

adding triage label to this to discuss whether Inf/-Inf are valid values for the return. Imo inf can't be a valid representation of a rounded number.

@LilithHafner
Copy link
Member Author

I am inclined to allow inf for reasons I mentioned in #50778 (comment) and concur that we should discuss this at triage.

@adienes
Copy link
Contributor

adienes commented Aug 6, 2023

without knowing too much of the details here, I think it could be nice to coordinate with #50730 such that

isapprox(x, round(T, x); atol=1)

is always true (or throws an error)

@LilithHafner
Copy link
Member Author

That would be nice, though hypercomplex numbers can be arbitrarily far from the nearest lattice point.

julia> x = Complex(1.9, 1.9)
1.9 + 1.9im

julia> isapprox(x, round(x, RoundDown), atol=1)
false

julia> isapprox(x, round(x, RoundDown), atol=1.3)
true

With greater than 4 dimensions it is possible to get this behavior with RoundNearest as well.

julia> using Octanians

julia> x = Octonion(fill(1.5, 8)...)
OctonionF64(1.5, 1.5, 1.5, 1.5, 1.5, 1.5, 1.5, 1.5)

julia> Base.round(x::Octonion, r::RoundingMode) = Octonion(
           round(x.s, r),
           round(x.v1, r),
           round(x.v2, r),
           round(x.v3, r),
           round(x.v4, r),
           round(x.v5, r),
           round(x.v6, r),
           round(x.v7, r))

julia> y = round(x)
OctonionF64(2.0, 2.0, 2.0, 2.0, 2.0, 2.0, 2.0, 2.0)

julia> isapprox(x, y, atol=1)
false

julia> isapprox(x, y, atol=1.5)
true

@adienes
Copy link
Contributor

adienes commented Aug 6, 2023

well, maybe only guarantee that holds for <:Real then 😅, I guess there will always be niche number types for edge cases

@nsajko
Copy link
Contributor

nsajko commented Aug 6, 2023

@test round(Float32, 1e60) == Inf32

This is wrong because infinity isn't an integer. The Julia documentation for round already says that the return value is integral, so this conflicts with the documented behavior. Also note that unsafe_trunc is already available and part of the public interface.

The more interesting question is what should be done when the user provides an infinite value as input. FWIW, C/C++ return infinity (with unchanged sign): https://en.cppreference.com/w/cpp/numeric/math/round https://en.cppreference.com/w/cpp/numeric/math/rint https://en.cppreference.com/w/cpp/numeric/math/nearbyint

@Seelengrab
Copy link
Contributor

Seelengrab commented Aug 7, 2023

Related #42060

@vchuravy
Copy link
Member

vchuravy commented Aug 7, 2023

Could we split the bugfix out so that we can backport it?

@Seelengrab
Copy link
Contributor

Seelengrab commented Aug 7, 2023

For the record, I don't think this PR should implement #42060, for the reasons given in the discussion there. I only linked it precisely because these methods/implementations are not sound in general.

@LilithHafner
Copy link
Member Author

LilithHafner commented Aug 7, 2023

Could we split the bugfix out so that we can backport it?

The bugfix is implementing a missing method by widening type signatures and adding default fallbacks, so no, this would have to be backported manually. However, the bug can and should be fixed regardless of what we decide about this PR.

@LilithHafner LilithHafner added the speculative Whether the change will be implemented is speculative label Aug 7, 2023
@Seelengrab
Copy link
Contributor

The default fallbacks to fix are only related to #50778, i.e. the methods for Base.IEEEFloat though, no? That should be cleanly implementable without widening the existing type signatures.

@mikmoore
Copy link
Contributor

mikmoore commented Aug 7, 2023

Nice!

My obnoxious corner-case with

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

is to complain that I would naively expect round(Float32, 2^30+1, RoundUp) to produce nextfloat(2f0^30) == 2f0^30+128f0, but with this chain of logic it would round(2^30+1, RoundUp) == 2^30+1 (i.e, a no-op), then convert(Float32, 2^30+1) == 2f0^30. But perhaps this really should be the domain of convert and not round at all, so perhaps this shouldn't be considered here...

@imciner2
Copy link
Contributor

imciner2 commented Aug 7, 2023

adding triage label to this to discuss whether Inf/-Inf are valid values for the return. Imo inf can't be a valid representation of a rounded number.

Wouldn't this be dependent on the rounding mode and type being rounded to? For Float types, IEEE semantics for the rounding could dictate when/if Inf/-Inf is returned based on Sec 4.3 of the standard (and Sec 7.4 for the overflow operation for the inexact semantics). For instance, with these semantics it is impossible for RoundToZero to ever return an Inf/-Inf from the function, but RoundUp can return Inf only when the given value is greater than the largest representable number of the format (and never return a -Inf).

@nsajko
Copy link
Contributor

nsajko commented Aug 7, 2023

@imciner2 there are two different meanings of round at play here. The parts of the standard you refer to are concerned with rounding to some value in the floating-point format, while this PR deals with rounding to integer values.

The standard also covers that topic, in 5.9, "Details of operations to round a floating-point datum to integral value". Excerpt:

The rounding is analogous to that specified in Clause 4, but the rounding chooses only from among those
floating-point numbers of integral values in the format. These operations convert zero operands to zero
results of the same sign, and infinite operands to infinite results of the same sign.

@LilithHafner
Copy link
Member Author

In Julia master (but not in any released version) we already have

julia> round(Float32, 1e60)
Inf32

julia> round(Float32, floatmax(Float32)-1.0)
3.4028235f38

The method

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

was added in #45598 without serious consideration about this behavior. It is not too late to revert or revise this definition.

@Seelengrab
Copy link
Contributor

Considering the discussion in #42060, yes that should probably be reverted & limited to Irrational.

@gbaraldi
Copy link
Member

Triage disscussed this and while there are some annoyances to how this works, round documents that it's error throwing behaviour is similar to convert, and if convert doesn't error then neither should round.

@Seelengrab
Copy link
Contributor

Seelengrab commented Aug 17, 2023

Was there discussion about how Irrational & the corresponding revert is affected? The revert is important because there hasn't been a decision in #42060 after all.

@gbaraldi
Copy link
Member

No discussion was made on rounding Irrationals unfortunately.
What was discussed is that the round(Float64,1.2) method is perfectly fine. And that due to how round is documented it shouldn't error where convert doesn't. So since convert(Float32,1e60) doesn't error, neither should round, specially because the convert says that if it's representable it will do the conversion and apparently returning Inf32 means it's representable (That's the IEEE semantic) because we just call a native instruction.

@nsajko
Copy link
Contributor

nsajko commented Aug 17, 2023

What do you mean by "That's the IEEE semantic"? As far as I understand, IEEE specifies that the result should be chosen among the integers (thus among finite values) except if the input is not finite, as mentioned in my message above ("the rounding chooses only from among those floating-point numbers of integral values in the format")?

@LilithHafner
Copy link
Member Author

LilithHafner commented Aug 30, 2023

Just saw #29939. This already fixes half of that issue by accident, but should also fix the other half and add tests.

#29939 is somewhat unrelated, and this is already big enough / touches so many issues that I don't want to add another.

@LilithHafner
Copy link
Member Author

And also just found #51113 when working on fixing #51063. This PR already fixed #51113 by accident when I found it. (optimistically, better wholistic design => less bug surface area => fixes bus I didn't even know about)

Still need to add tests for the issues this fixed by accident.

@LilithHafner
Copy link
Member Author

IIUC triage has approved the semantics, but this still needs code review before merging.

As for breaking out the bugfixes for backporting, that is possible, however,
a) the bugfixes change incorrect answers to errors, which is not something I'm super eager to backport.
b) it's not trivial to separate the bugfixes out and I don't feel personally motivated to do so. None of the bugs were catastrophic.

@LilithHafner LilithHafner changed the title Refactor rounding (including bugfix) and document API Refactor rounding (including bugfixes) and document API Aug 30, 2023
base/float.jl Outdated
round(x::IEEEFloat, r::RoundingMode{:Down}) = floor_llvm(x)
round(x::IEEEFloat, r::RoundingMode{:Up}) = ceil_llvm(x)
round(x::IEEEFloat, r::RoundingMode{:Nearest}) = rint_llvm(x)
round(::Type{Bool}, x::AbstractFloat, ::RoundingMode{:ToZero}) =
Copy link
Member

Choose a reason for hiding this comment

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

It seems like these definitions are valid for all Integers (using typemin/typemax) is there a reson for special casing these?

Copy link
Member Author

Choose a reason for hiding this comment

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

tehe I underestimated the generic fallbacks. We don't need these methods at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

deleted.

@oscardssmith
Copy link
Member

Overall this change looks really good. It's always nice to simplify the dispatch hierarchy and come up with the right interface. We probably want a pkgeval to make sure no number packages were defining bad rounding methods that are method ambiguities after this change.

@LilithHafner
Copy link
Member Author

@nanosoldier runtests()

@Seelengrab
Copy link
Contributor

Can we get #50835 merged before this one, so as not to accidentally introduce breakage there & here?

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@LilithHafner
Copy link
Member Author

LilithHafner commented Aug 31, 2023

DoubleFloats.jl fails because they don't define round(::DoubleFloat, RoundDown). round(T, x, RoundDown) used to be special cased to go straight to trunc (unlike other rounding modes wich round first) but with this PR, the generic round(T, x, RoundDown) only works if round(x, RoundDown) is defined. This is technically a breakage, but idk. I think it's okay because it's due to a package bug that is literally a typo with a trivial fix that is already merged.

The other failures look unrelated.

@Seelengrab, I'd rather do t he refactor first and more functionality changes later so that we don't have to keep working with the old system.

@LilithHafner LilithHafner merged commit e0f5247 into master Aug 31, 2023
1 check passed
@LilithHafner LilithHafner deleted the lh/document-refactor-rounding branch August 31, 2023 21:40
@Seelengrab
Copy link
Contributor

Seelengrab commented Sep 1, 2023

Why was this PR still marked as closing #50812, when it was clear that this change was not clear cut? The revert I mentioned was explicitly to prevent new functionality from being merged. Merging the revert first would have likely enabled this PR to go ahead, without affecting Complex at all..

Also, considering the revert was opened two days after this PR, there was ample time to merge it first, before merging the greater refactor. Now we need an actual new PR to re-break the feature this one added :/

@Seelengrab
Copy link
Contributor

Seelengrab commented Sep 1, 2023

I've opened the PR for disallowing the complex part, but it would have been nice if that was part of this PR (which explicitly sought ought not to include new functionality, but alas..)

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 docs This change adds or pertains to documentation maths Mathematical functions
Projects
None yet