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

Context #75

Merged
merged 1 commit into from
Nov 3, 2024
Merged

Context #75

merged 1 commit into from
Nov 3, 2024

Conversation

barucden
Copy link
Collaborator

@barucden barucden commented Oct 27, 2024

This PR introduces Context, a structure that holds configuration of the decimal arithmetic.

Eventually, the global variable DIGITS should be completely removed in favor of this newly-added structure.

Requires #79

Copy link

codecov bot commented Oct 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.60%. Comparing base (f3deb0b) to head (a9e51b0).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   98.13%   99.60%   +1.47%     
==========================================
  Files          10       11       +1     
  Lines         214      255      +41     
==========================================
+ Hits          210      254      +44     
+ Misses          4        1       -3     

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

@barucden barucden force-pushed the ctxt branch 5 times, most recently from 8ab50a2 to 1d7c94e Compare October 28, 2024 17:44
@barucden barucden marked this pull request as ready for review October 28, 2024 17:56
@barucden
Copy link
Collaborator Author

@ViralBShah I wonder if it would be a better interface to have CONTEXT be a ScopedValue. The users would then do

@with CONTEXT => Context(precision=123, rounding=RoundUp) begin
   # user code
end

instead of

setprecision(Decimal, 123) do
    setrounding(Decimal, RoundUp) do
        # user code
    end
end

I think the ScopedValue approach is better. It even avoids things like

# In package A
module A
    using Decimals
    function f()
        setprecision(Decimal, 123)
    end
end

# In package B
module B
    using A
    function g()
        setprecision(Decimal, 456)
        f()
        # precision is 123, not 456
    end
end

One "downside" is that users might be used to setprecision/setrounding from Base.

@ViralBShah
Copy link
Member

ViralBShah commented Oct 29, 2024

This sounds like a better idea than global state for sure! I think doing things differently from Base that is a better way , should be encouraged. And that often paves the way for Base to do new things later. @oscardssmith Any thoughts, or anyone else we can ask.

I don't think there is much of a design decision here, since this is natural - but sometimes it is good to bring these up on discourse.

@oscardssmith
Copy link
Member

yeah this seems like a reasonable design. I think this is what BigFloat is moving to as well, right?

@barucden
Copy link
Collaborator Author

I am not sure about BigFloat but there is a very recent PR that turns global variables into ScopedValues: JuliaLang/julia#56378

@barucden
Copy link
Collaborator Author

ScopedValues require Julia 1.8 at minimum. Is it okay to lift our requirement from v1.6 to v1.8?

@oscardssmith
Copy link
Member

yeah 1.10 is new LTS, so no one would complain if you lifted to 1.8

@barucden barucden force-pushed the ctxt branch 2 times, most recently from 6482133 to 34fd7d2 Compare October 30, 2024 20:09
@barucden
Copy link
Collaborator Author

What do you think about the with_context function/macro? It's just for convenience

Macro:

using Decimals
Decimals.@with_context (Emax=384, Emin=-383, precision=9, rounding=RoundNearestTiesAway) begin
    @test -(dec"56267e-0") == dec"-56267"
end

instead of

using Decimals
using ScopedValues
@with Decimals.CONTEXT => Context(Emax=384, Emin=-383, precision=9, rounding=RoundNearestTiesAway) begin
    @test -(dec"56267e-0") == dec"-56267"
end

Function:

using Decimals
Decimals.with_context(f, Emax=384, Emin=-383, precision=9, rounding=RoundNearestTiesAway)

instead of

using Decimals
using ScopedValues
with(f, Decimals.CONTEXT => Decimals.Context(Emax=384, Emin=-383, precision=9, rounding=RoundNearestTiesAway))

This commit introduces `Context`, a structure that holds configuration
of the decimal arithmetics.

Eventually, the global variable `DIGITS` should be completely removed in
favor of this newly-added structure.
@barucden barucden merged commit 1cd6b89 into JuliaMath:master Nov 3, 2024
7 checks passed
@barucden barucden deleted the ctxt branch November 3, 2024 13:06
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.

3 participants