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

Fix thread-safe in rounding #615

Merged
merged 14 commits into from
Jan 21, 2024
Merged

Conversation

OlivierHnt
Copy link
Member

@OlivierHnt OlivierHnt commented Jan 15, 2024

This PR fixes the rounding mechanism to be thread-safe. It removes CRlibm.jl as a dependency, and directly calls methods from CRlibm_jll, or from MPFR as suggested by @Joel-Dahne.

This PR also fixes rounding for intervals with rational bounds by returning a float interval for non exact operations.

To do: we should probably also fix our quadrant method for trigonometric functions which is not thread-safe due to JuliaLang/julia#52862

@Joel-Dahne
Copy link
Contributor

At a quick look this looks good!

One question though, the CRlib methods are defined for :slow rounding. Should they not be defined for :tight? Currently there are no methods defined using :tight that I can see.

Regarding the comparison with irrationals. It's not only not thread-safe, but also not always correct. The implementation of rounding to float for irrationals in base doesn't guarantee faithful rounding. I recently noticed this and though I would open an issue, but haven't gotten to it yet. Here is an example, defining the completely useless irrational number $1 - \pi^{-300}$

julia> Base.@irrational qq (1 - big(π)^-300)

julia> interval(qq, qq)
[1.0, 1.0]_com

Clearly the lower bound for the interval should be less than 1. The issue is that Float64(::Irrational, ::RoundingMode) converts the irrational to a BigFloat (with 256 bits and round to nearest) and then rounds it to Float64 with the given Rounding mode. In the above example $1 - \pi^{-300}$ is then close enough to 1 to be rounded to 1 in the first step. If one takes the exponent to be say -100 the issue goes away. Note that we also for example have

julia> qq < 1
false

which is clearly not correct.

@OlivierHnt
Copy link
Member Author

Thx for taking a look. In these situations, :tight defaults to :slow indeed; from my understanding, methods for :tight rounding should not have to call BigFloat but for now we have no alternatives.

Oh what a pity that comparisons to irrationals don't work 😞. So we need to rework our quadrant method differently (already relying on rem2pi felt a little shady).

@Joel-Dahne
Copy link
Contributor

It will work in practice for all irrationals defined in Base at least (except for not being thread-safe). The only issue is for irrationals that are extremely close to floating point numbers, so that 256 bits of precision is not enough to correctly round them.

@Joel-Dahne
Copy link
Contributor

Thx for taking a look. In these situations, :tight defaults to :slow indeed; from my understanding, methods for :tight rounding should not have to call BigFloat but for now we have no alternatives.

But they call CRlibm, so are not using BigFloat? Or am I missing something?

@OlivierHnt
Copy link
Member Author

OlivierHnt commented Jan 15, 2024

Oh I see the lines you are referring too. Yes, that probably should be :tight indeed 🙂.

@codecov-commenter
Copy link

codecov-commenter commented Jan 15, 2024

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (1d744c3) 0.00% compared to head (016e4ff) 83.64%.

Files Patch % Lines
src/intervals/arithmetic/trigonometric.jl 81.91% 17 Missing ⚠️
src/intervals/arithmetic/hyperbolic.jl 50.00% 8 Missing ⚠️
src/intervals/rounding.jl 96.34% 3 Missing ⚠️
src/intervals/arithmetic/power.jl 75.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #615       +/-   ##
===========================================
+ Coverage        0   83.64%   +83.64%     
===========================================
  Files           0       25       +25     
  Lines           0     2140     +2140     
===========================================
+ Hits            0     1790     +1790     
- Misses          0      350      +350     

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

test/interval_tests/complex.jl Outdated Show resolved Hide resolved
src/intervals/rounding.jl Show resolved Hide resolved
@OlivierHnt
Copy link
Member Author

This PR does not rework the quadrant method defined used for trigonometric functions which is not thread-safe.
It should probably be a separate PR anyways.

Fixes #612.

@OlivierHnt OlivierHnt linked an issue Jan 21, 2024 that may be closed by this pull request
@OlivierHnt OlivierHnt merged commit 4113027 into JuliaIntervals:master Jan 21, 2024
16 checks passed
@OlivierHnt OlivierHnt deleted the rounding branch January 21, 2024 13:16
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.

Non-thread safe use of setrounding
5 participants