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

Normalization and rounding #88

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Normalization and rounding #88

merged 1 commit into from
Nov 11, 2024

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

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
2 participants