Skip to content

Conversation

@barucden
Copy link
Collaborator

@barucden barucden commented Nov 6, 2024

The PR replaces round and normalize.

Fixes #27:

julia> o2_sum = dec"123.456789"
123.456789

julia> o_c = dec"987.654321"
987.654321

julia> o2_sum - o_c
-864.197532

julia> round(o2_sum - o_c, sigdigits=6)
-864.198

Fixes #36 (the confusing argument will be gone)
Fixes #39 (the confusing argument will be gone)
Fixes #50:

julia> round(Int, Decimal(3.5))
4

julia> round(Decimal(3.5), RoundNearest)
4

@codecov
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (85560c3) to head (d6c9071).
Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master       #88      +/-   ##
===========================================
+ Coverage   99.69%   100.00%   +0.30%     
===========================================
  Files          11        11              
  Lines         330       327       -3     
===========================================
- Hits          329       327       -2     
+ Misses          1         0       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@barucden barucden changed the title Normal Normalization and rounding Nov 7, 2024
@barucden barucden force-pushed the normal branch 2 times, most recently from 5b8bbb0 to fa1e39e Compare November 7, 2024 21:26
The PR replaces `round` and `normalize`.

Fixes JuliaMath#27
Fixes JuliaMath#36
Fixes JuliaMath#39
Fixes JuliaMath#50
@barucden barucden marked this pull request as ready for review November 7, 2024 21:46
@barucden
Copy link
Collaborator Author

barucden commented Nov 8, 2024

@ViralBShah @oscardssmith I would appreciate your input here. My intention is to support the interface

round([T,] x, [r::RoundingMode])
round(x, [r::RoundingMode]; digits::Integer=0)
round(x, [r::RoundingMode]; sigdigits::Integer)

Defining Base.round(x::Decimal, r::RoundingMode) is not enough because round(Decimal(0.123456), digits=2) would not guarantee that the output actually has only two decimal places.

It is also not enough to define only one method

Base.round(x::Decimal, r::RoundingMode; digits, sigdigits)

because then, e.g., round(Decimal(...), RoundNearestTiesAway) is ambigous since Base defines round(::AbstractFloat, ::RoundingMode{:NearestTiesAway}) and Decimal <: AbstractFloat.

So I am generating 6 methods for 6 rounding modes:

for r in (RoundingMode{:Up},
          RoundingMode{:Down},
          RoundingMode{:FromZero},
          RoundingMode{:Nearest},
          RoundingMode{:NearestTiesAway},
          RoundingMode{:ToZero})
    @eval function Base.round(x::Decimal, r::$r;
                     digits::Union{Nothing, Integer}=nothing,
                     sigdigits::Union{Nothing, Integer}=nothing)
        return _round(x, r, digits, sigdigits)
    end
end

It passes tests but I am not sure how good/bad this is. I asked on Discourse, but haven't received a definite answer so far.

@ViralBShah
Copy link
Member

I don't have input here, but I would suggest looking at DecFP.jl (in case they address it there).

@barucden
Copy link
Collaborator Author

barucden commented Nov 8, 2024

I looked at DecFP.jl and tried to copy their approach, but it did not work for Decimals.

DecFP only defines methods of the form round(x, rounding_mode) for several rounding modes, but then round(x, digits=n) works too. Somehow, the rounding logic (https://github.com/JuliaLang/julia/blob/a005c07914040995784914cf0b2ccaf8af52ccc2/base/floatfuncs.jl#L50) works nicely with DecFP.

I implemented the same methods for Decimals, but when I tried round(Decimal(0.123456), digits=1) I was getting results like 0.10000...04 (it's not a real result, I did not record the numbers; this is just to paint a picture).

@barucden barucden merged commit 92225ac into JuliaMath:master Nov 11, 2024
7 checks passed
@barucden barucden deleted the normal branch November 11, 2024 07:36
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.

rounding failures Unclear option normal in round function Rounded option in the normalize function significant digits rounding

2 participants