Add custom overloads for rem(x,y,::RoundingMode) for FixedDecimals

[NHDaly]

However, the simple overload does not seem to work, since it is ambiguous with rem(x,y,::RoundingMode{Up}), for example, so for rem, we're manually overloading all RoundingModes.

[NHDaly]

@TotalVerb I originally added the straightforward method, below, but it gave a MethodAmbiguity error:

julia> Base.rem(x::T, y::T, r::RoundingMode) where {T <: FD} = reinterpret(T, rem(x.i, y.i, r))

julia> rem(x, x, RoundUp)
ERROR: MethodError: rem(::FixedDecimal{Int64,2}, ::FixedDecimal{Int64,2}, ::RoundingMode{:Up}) is ambiguous. Candidates:
  rem(x::T, y::T, r::RoundingMode) where T<:FixedDecimal in Main at REPL[20]:1
  rem(x, y, ::RoundingMode{:Up}) in Base at div.jl:69
Possible fix, define
  rem(::T, ::T, ::RoundingMode{:Up}) where T<:FixedDecimal

So i implemented all the more specific methods instead.

This actually leads me to wonder whether we should be doing the div() methods the same way...

---

And in fact, the generic definition of rem provided in Base, rem(x, y, ::RoundingMode{:Up}) = mod(x,-y) means that we actually already support the 3-arg rem out of the box without doing anything more in this package... I now remember that i knew this already, which is why i didn't implement it last time -- i wanted to ask someone about this.

Those defs are here: https://github.com/JuliaLang/julia/blob/fbc2c0aec19a7c415011c1abd4f05622ac9c41e7/base/div.jl#L67-L69

I'm wondering if we're not supposed to be overloading three-argument div and rem for generic ::RoundingModes? The API isn't super clear or well-documented.

[coveralls]

Coverage increased (+0.02%) to 97.531% when pulling b585127a660778d5f81febad6da0fb9b8123f2bc on nhd-rem-rounding-mode into 317c82b1cf90870ef0a004d167f7ffd231e1a731 on nhd-explicit-base-extending.

[coveralls]

Coverage increased (+0.02%) to 97.531% when pulling c660eb55fe39caf4153648c7ff613c2b0010e2f4 on nhd-rem-rounding-mode into 317c82b1cf90870ef0a004d167f7ffd231e1a731 on nhd-explicit-base-extending.

[NHDaly]

Additionally, one other concern:

The default 3-arg Base.rem implementation provided above doesn't call promote like the 2-arg versions do, which means that our newly added methods aren't being triggered for non-FixedDecimal types. I could probably add the overloads to call promote, but this might be worth filing with julia itself?

julia> @which rem(FD2(0.5), FD2(2), RoundToZero)
rem(x::T, y::T, r::RoundingMode{:ToZero}) where T<:FixedDecimal in FixedPointDecimals at /Users/daly/julia-dev/FixedPointDecimals/src/FixedPointDecimals.jl:302

julia> @which rem(FD2(0.5), 2, RoundToZero)
rem(x, y, ::RoundingMode{:ToZero}) in Base at div.jl:67

[TotalVerb]

This seems to be an unfortunate inconsistency within Julia itself (where the div 3-arg is being made primitive, with fld, etc. deferring to it, while the rem 3-arg is currently non-primitive and calls rem/mod). If the # TODO resolves in Base, then we will need to implement something like this PR, but until then I suppose it is not needed.

[NHDaly]

If the # TODO resolves in Base, then we will need to implement something like this PR, but until then I suppose it is not needed.

Okay yeah, makes sense. Then i'm going to put this PR on hold until such time as it's resolved in Base?

[TotalVerb]

That sounds reasonable to me. Thanks for your hard work!

On Sun., Apr. 19, 2020, 12:10 Nathan Daly, [email protected] wrote:

If the # TODO resolves in Base, then we will need to implement something like this PR, but until then I suppose it is not needed.

Okay yeah, makes sense. Then i'm going to put this PR on hold until such time as it's resolved in Base?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JuliaMath/FixedPointDecimals.jl/pull/53#issuecomment-616172112, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADCOMBXYG6UR3PNOBURUGWDRNMO7TANCNFSM4KOV7M3Q .