Skip to content

Conversation

@sostock
Copy link
Contributor

@sostock sostock commented Jan 19, 2021

Fixes #34325.

@kimikage
Copy link
Contributor

The current (master) implementation is buggy. However, considering the promotion, I think the prohibition is too strict.

julia> div(3.0, UInt(2), RoundUp)
2.0

julia> rem(3.0, UInt(2), RoundUp)
3.0 # bug (#34325)
-1.0 # expected

julia> div(3, UInt8(2), RoundUp)
2

julia> rem(3, UInt8(2), RoundUp)
0x0000000000000003 # bug (#34325)
-1 # expected

Just FYI, I implemented some arithmetic operations involving saturating arithmetic in FixedPointNumbers.jl (cf. JuliaMath/FixedPointNumbers.jl#230).

From the perspective of consistency, I would like to relax the restriction.
For example, it might look something like this:

Base.rem(x, y, r::RoundingMode{:Up}) = rem(promote(x, y)..., r)
Base.rem(x::T, y::T, ::RoundingMode{:Up}) where {T} = mod(x, -y)
Base.rem(x::T, y::T, ::RoundingMode{:Up}) where {T <: Unsigned} = y == oneunit(T) ? zero(T) : error()

@sostock
Copy link
Contributor Author

sostock commented Jan 20, 2021

I agree, the current implementation is too strict. However, I think it’s better to always throw an error in Base.rem(x::T, y::T, ::RoundingMode{:Up}) where {T <: Unsigned}. It seems unfavorable to me that whether an error is thrown depends on the value of y but not on the value of x.

@sostock
Copy link
Contributor Author

sostock commented Jan 21, 2021

However, other rem methods do not use promotion. Maybe they should? Their return type currently also depends on whether a RoundingMode is passed or not:

julia> rem(Int(5), UInt(2), RoundDown)
0x0000000000000001

julia> rem(UInt(5), Int(2), RoundDown)
1

julia> rem(Int(5), UInt(2))
1

julia> rem(UInt(5), Int(2))
0x0000000000000001

@kimikage
Copy link
Contributor

Ah, good reminder of complicated remainder!
We might want to be a little more cautious about making changes.

@StefanKarpinski
Copy link
Member

Seems like fixing this would be more appropriate than disallowing it.

@ViralBShah ViralBShah added the maths Mathematical functions label Feb 24, 2022
@ViralBShah
Copy link
Member

Do we want to get this merged for now?

@adienes adienes added bugfix This change fixes an existing bug needs decision A decision on this change is needed labels May 12, 2025
@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing and removed needs decision A decision on this change is needed labels Oct 1, 2025
@oscardssmith oscardssmith added triage This should be discussed on a triage call and removed merge me PR is reviewed. Merge when all tests are passing labels Oct 1, 2025
@LilithHafner LilithHafner added needs pkgeval Tests for all registered packages should be run with this change minor change Marginal behavior change acceptable for a minor release labels Oct 9, 2025
@LilithHafner
Copy link
Member

LilithHafner commented Oct 23, 2025

Triage proposes this semantic for rem(x, y::Unsigned, RoundUp):

Return an object of type mod(x, -signed(y)) or error. If the correct answer (as determined by widening) is representable in that type, return it. Otherwise error. Don't return answers that are modularly correct because that doesn't make sense in the context of division.

This can be tested exhaustively over all combinations of values of types Int8/UInt8/UInt16/Int16.

@LilithHafner LilithHafner removed the triage This should be discussed on a triage call label Oct 23, 2025
@adienes
Copy link
Member

adienes commented Nov 9, 2025

one thing not discussed during triage is that the rounding modes RoundNearest and RoundFromZero have the exact same problem, as does divrem.

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 maths Mathematical functions minor change Marginal behavior change acceptable for a minor release needs pkgeval Tests for all registered packages should be run with this change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rem(::Unsigned, ::Unsigned, RoundUp) gives wrong results

8 participants