Support rounding Irrationals

[theabhirath]

Fixes #45588.

[Liozou]

Hello and thank you for this PR! It looks fine but you should add tests as well, probably in test/numbers.jl.

Since these definitions are identical to the ones for AbstractFloat, I think an alternative solution could consist in replacing AbstractFloat with Real instead in these definitions:

julia/base/float.jl

Lines 357 to 359 in 56f1d24

	floor(::Type{T}, x::AbstractFloat) where {T<:Integer} = trunc(T,round(x, RoundDown))
	ceil(::Type{T}, x::AbstractFloat) where {T<:Integer} = trunc(T,round(x, RoundUp))
	round(::Type{T}, x::AbstractFloat) where {T<:Integer} = trunc(T,round(x, RoundNearest))

which I believe is also more in the spirit of what the docstring of round specifies:

To extend round to new numeric types, it is typically sufficient to define Base.round(x::NewType, r::RoundingMode).

But I'm not completely sure that's fine either. Tentatively pinging @simonbyrne since I think you're familiar with these, any preference?

[theabhirath]

Thank you so much for the quick response! I've added some tests now. I wasn't sure about using Real since there are existing definitions for Rational that might be in conflict, so I just stuck to duplicating the code a little bit instead😅

[Liozou]

Thank you for the tests! Looks good to me.

[Liozou]

Since Rational <: Real and given the signatures of the function, the rules of dispatch with subtyping would ensure that the Rational functions get called on Rational inputs, and the Real one for subtypes of Real with no specialized methods. But anyway, that can always be for another PR, this one looks fine as it is!

[LilithHafner]

I think we may want to extend the type signatures all the way to Number. Extended reals such as Dual from DualNumbers.jl may be rounded to Integers in this way as well. That said, this PR is already an improvement, so we can generalize to Real or Number later.

[theabhirath]

Bump on merging this if it's okay?

[inkydragon]

CI failed look unrelated.

CI failed list

• x86_64-linux-gnuassertrr-net Pkg error x1
• x86_64-linux-gnu Pkg fail x2 + error x3

But it is always helpful to re-run the CI. You can do this by clicking on "Update branch" button.

[LilithHafner]

I prefer the solution @Liozou proposed of widening the existing type signatures over the approach of adding more methods because it requires less code, leaves fewer special cases, and applies more seamlessly to user-defined types. I think it should be safe to extend these definitions to ::Real. In the case of reals that cannot be rounded to the given integer type, we will get an error like this

julia> round(UInt, -5.3)
ERROR: InexactError: trunc(UInt64, -5.0)
Stacktrace:
 [1] trunc
   @ ./float.jl:760 [inlined]
 [2] round(#unused#::Type{UInt64}, x::Float64)
   @ Base ./float.jl:359
 [3] top-level scope
   @ REPL[8]:1

Mathematically, it makes sense to ask for the next highest integer after a real, so I think we should provide generic methods to support this type of query.

[theabhirath]

Thanks for the feedback! I've done what you've suggested and extended the already existing signature to Real. I'm not sure if there's another file where these methods should be moved to, but for now they're in base/float.jl.

[theabhirath]

There's a test failure that indicates an ambiguous method because there are now two definitions. There's a Rational specific definition here that seems to be in conflict. I think the solution could be to separate out the method definitions like so:

function round(::Type{T}, x::Rational{Tr}, r::RoundingMode=RoundNearest) where {T,Tr}
    if iszero(denominator(x))
        return convert(T, copysign(unsafe_rational(one(Tr), zero(Tr)), numerator(x)))
    end
    convert(T, div(numerator(x), denominator(x), r))
end

function round(::Type{T}, x::Rational{Tr}, r::RoundingMode=RoundNearest) where {T<:Integer, Tr}
    convert(T, div(numerator(x), denominator(x), r))
end

but I'm not an expert so I'd appreciate any help I get 😅

[LilithHafner]

Now I see what you meant by

there are existing definitions for Rational that might be in conflict

You were quite right!

This sounds like a good way to resolve the ambiguity. The second method does not need the Tr type parameter anymore because it is only used in the !(T <: Integer) branch. There should not be any runtime impact from removing it.

[theabhirath]

Ah, the edge cases continue to crop up 😅 this may take time to fix completely. Most of the test failures seem to indicate that there need to be more specific methods defined (either that or there are now ambiguous methods because Real has a ton of subtypes being affected)

[LilithHafner]

The ambiguities between Real and Missing can be fixed by adding
round(::Type{T}, x::Real, r::RoundingMode=RoundNearest) where {T>:Missing} = round(nonmissingtype_checked(T), x, r)
and
($f)(::Type{T}, x::Real) where {T>:Missing} = $f(nonmissingtype_checked(T), x)
methods analogous to the existing methods at base/missing.jl:146 and base/missing.jl:158 to alleviate ambiguities with Rational and Missing.

[LilithHafner]

I think #45598 (comment) is still unresolved

[theabhirath]

I think #45598 (comment) is still unresolved

Whoops yeah, had missed that, resolved now!

[LilithHafner]

These should go in floatfuncs.jl by

julia/base/floatfuncs.jl

Lines 153 to 155 in 31d4c22

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

and also pass on keyword arguments.

I also think convert is more appropriate than trunc in general. We use trunc instead of convert for IEEEFloats because it is faster. Perhaps we have these functions dispatch to a three argument round, have three argument round fallback to convert(T, round(x, rm)), and define three argument round for floats to use trunc.

The reason we can't merge as is and make those changes later is that exporting these new generic methods is a feature and changing them would be breaking.

[theabhirath]

Hey, sorry for dropping the ball on this, I caught up with some things and completely forgot! I think I've implemented these suggestions now. The three argument round for floats already uses trunc, I believe:

julia/base/floatfuncs.jl

Lines 122 to 125 in 5e4ecf1

	function round(::Type{T}, x::AbstractFloat, r::RoundingMode) where {T<:Integer}
	r != RoundToZero && (x = round(x,r))
	trunc(T, x)
	end

so hopefully the current iteration of the code does what was intended 😅

[LilithHafner]

Rounding could probably use some refactoring, but this PR doesn't need to wait for that.

[LilithHafner]

From a past triage #50812 (comment)

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.

Which, as I interpret it, means triage agreed with the changes made in this PR. (specifically with widening the type signature to ::Real)

So thank you for your contribution, @theabhirath. I think we did the right thing here (or right-ish, nothing is really right or wrong in PL, just lots of opinions)

[theabhirath]

Cool, happy to help 😄! (and also to have my first contribution to the base Julia repo 🎉 )