Towards single parent balls #1367

thofma · 2022-12-23T15:09:12Z

At the moment, there is a new type RealField and the corresponding elements are called arb_t (temporarily). I will rename them to ArbField and arb later.

julia> RR = RealField()
Real Field

julia> x = RR(1.0001)
[1.0000999999999999890 +/- 1.35e-20]

julia> y = RR(1.0)
1.0000000000000000000

julia> Nemo.set_precision(RealField, 4) do
         x*y
       end
[1e+0 +/- 0.126]

julia> Nemo.set_precision(RealField, 1000) do
         x*y
       end
[1.0000999999999999890 +/- 1.35e-20]

(There is also set_precision!(RealField, 1000) for permanent change of the global precision).

We will also add the layer where one can specify the precision directly, e.g., z = sin(x, 32).

Here are some questions:

Should the set_precision syntax use the an instance of RealField or RealField itself? That is, should it be set_precision(RR, 100) or set_precision(RealField, 100)? At the moment I went with the former, but I am happy to change it.
It is also the question, should it be roots(f, RealField) or roots(f, RealField()).
The word "precision" has a very specific meaning for elements of type arb, but it is quite ambiguous in certain functions. For example roots(f::fmpz_poly, R::RealField, prec::Int). Does that mean that we want the computation done with prec bits of precision? Or do we want the resulting balls to have a certain radius, e.g, radius(r) <= 2^-prec? Does anyone have a good name for the second application? We should get away from the term "precision" in this case.

TODO

codecov · 2023-01-01T14:01:22Z

Codecov Report

Merging #1367 (3d3836c) into master (5e06a3d) will decrease coverage by 0.90%.
The diff coverage is 80.43%.

@@            Coverage Diff             @@
##           master    #1367      +/-   ##
==========================================
- Coverage   89.32%   88.42%   -0.91%     
==========================================
  Files          82       88       +6     
  Lines       30748    34260    +3512     
==========================================
+ Hits        27467    30294    +2827     
- Misses       3281     3966     +685

Impacted Files	Coverage Δ
src/Nemo.jl	`28.65% <ø> (ø)`
src/arb/acb.jl	`85.23% <ø> (-0.22%)`	⬇️
src/arb/acb_poly.jl	`79.28% <ø> (-0.23%)`	⬇️
src/arb/arb_poly.jl	`81.11% <ø> (+0.38%)`	⬆️
src/arb/ArbTypes.jl	`59.62% <59.76%> (+0.04%)`	⬆️
src/arb/ComplexMat.jl	`77.30% <77.30%> (ø)`
src/arb/RealMat.jl	`80.12% <80.12%> (ø)`
src/arb/RealPoly.jl	`80.25% <80.25%> (ø)`
src/arb/ComplexPoly.jl	`80.33% <80.33%> (ø)`
src/arb/Complex.jl	`85.13% <85.13%> (ø)`
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

thofma · 2023-01-01T14:18:31Z

I added the missing types. To change the global precision one can invoke one of the following calls (they all do the same):

set_precision!(ArbField, 64)
set_precision!(ArbField(), 64)
set_precision!(AcbField, 64)
set_precision!(AcbField(), 64)
set_precision!(Balls, 64)

I added the Balls thing, since one changes precision for all ball arithmetic and I wanted this to be reflected in the syntax. We also support the set_precision!(X, 64) do bla; end syntax, where X is as above.

The only thing missing is documentation.

Any other thoughts @fieker?

fredrik-johansson · 2023-01-01T14:31:35Z

It is useful to have a default global RR with a mutable precision, but just to clarify, it is still possible to create new ArbField instances with their own precision? The latter being useful for library code.

thofma · 2023-01-01T14:58:46Z

No, the plan is to have only one ArbField instance.

In our experience, having different ArbField instances, one for each precision, makes for a rather cumbersome interface. (See oscar-system/Oscar.jl#1619 for the discussion that lead to this PR here).

What we will add is the "raw" interface, where one does add!(x::arb, y::arb, z::arb, prec::Int), that is, one supplies the precision directly, avoiding the global precision.

fingolfin · 2023-01-02T07:20:26Z

src/arb/ArbTypes.jl

+  return ARB_DEFAULT_PRECISION[]
+end
+
+function set_precision!(f, ::Type{Balls}, prec::Int)


How about calling this with_precision instead, matching withenv?

fingolfin · 2023-01-02T07:28:36Z

src/arb/ArbTypes.jl

+function set_precision!(f, ::Type{Balls}, prec::Int)
+  arb_check_precision(prec)
+  old = precision(Balls)
+  set_precision!(Balls, prec)


So this function is not thread safe (I am not sure if other parts of Nemo are thread safe, though it sure would be nice if they were :-) ).

If ARB_DEFAULT_PRECISION is only used by Julia code, I guess this could be rectified (now or in a future update) by making ARB_DEFAULT_PRECISION thread local. (E.g. by changing it to a Vector{Int}, indexed by Threads.threadid())

fingolfin · 2023-01-02T07:37:01Z

Regarding roots(f, RealField) versus roots(f, RealField()), from experience I know people will try the former and be confused if it doesn't work. So it would be nice if that at least printed a helpful error message, though at that point as a user one may wonder why it can't just "do what I want" if it clearly has guessed my intention right (a possible answer of course is: "because passing the type may not work in other situations and we don't want you to form bad habits, and instead learn how to do it right". Some users will appreciate this kind of sentiment more than others (to put it mildly).

fieker · 2023-01-02T07:59:38Z

On Sun, Jan 01, 2023 at 11:20:37PM -0800, Max Horn wrote: @fingolfin commented on this pull request. > +struct Balls +end + +# arb as in arblib +const ARB_DEFAULT_PRECISION = Ref{Int}(64) + +function set_precision!(::Type{Balls}, n::Int) + arb_check_precision(n) + ARB_DEFAULT_PRECISION[] = n +end + +function Base.precision(::Type{Balls}) + return ARB_DEFAULT_PRECISION[] +end + +function set_precision!(f, ::Type{Balls}, prec::Int) How about calling this `with_precision` instead, matching `withenv`?

Name is, essentially, inherited from julia's setprecision for BigFloat Does not mean that with_precision might not be better, but setprecision is Julia's choice

…

-- Reply to this email directly or view it on GitHub: #1367 (review) You are receiving this because you were mentioned. Message ID: ***@***.***>

fieker · 2023-01-02T08:00:56Z

On Sun, Jan 01, 2023 at 11:28:47PM -0800, Max Horn wrote: @fingolfin commented on this pull request. > +# arb as in arblib +const ARB_DEFAULT_PRECISION = Ref{Int}(64) + +function set_precision!(::Type{Balls}, n::Int) + arb_check_precision(n) + ARB_DEFAULT_PRECISION[] = n +end + +function Base.precision(::Type{Balls}) + return ARB_DEFAULT_PRECISION[] +end + +function set_precision!(f, ::Type{Balls}, prec::Int) + arb_check_precision(prec) + old = precision(Balls) + set_precision!(Balls, prec) So this function is not thread safe (I am not sure if other parts of Nemo are thread safe, though it sure would be nice if they were :-) ). If `ARB_DEFAULT_PRECISION` is only used by Julia code, I guess this could be rectified (now or in a future update) by making `ARB_DEFAULT_PRECISION` thread local. (E.g. by changing it to a `Vector{Int}`, indexed by `Threads.threadid()`)

From the Julia help: elp?> setprecision search: setprecision setprecision! set_precision! setprecision([T=BigFloat,] precision::Int; base=2) Set the precision (in bits, by default) to be used for T arithmetic. If base is specified, then the precision is the minimum required to give at least precision digits in the given base. │ Warning │ │ This function is not thread-safe. It will affect code running on │ all threads, but its behavior is undefined if called concurrently │ with computations that use the setting. │ Julia 1.8 │ │ The base keyword requires at least Julia 1.8. I think the thread safety can be (could be) fixed - eveltually Julia needs to have a sensible model for thread-local stuff

…

-- Reply to this email directly or view it on GitHub: #1367 (review) You are receiving this because you were mentioned. Message ID: ***@***.***>

thofma · 2023-01-02T09:00:29Z

So this function is not thread safe (I am not sure if other parts of Nemo are thread safe, though it sure would be nice if they were :-) ).

If ARB_DEFAULT_PRECISION is only used by Julia code, I guess this could be rectified (now or in a future update) by making ARB_DEFAULT_PRECISION thread local. (E.g. by changing it to a Vector{Int}, indexed by Threads.threadid())

Yes, the name is inherited from julia, although slightly more correct with the underscore.

Regarding thread safety, it is slightly more complicated since julia now allows to change the number of threads at runtime. It is also painful since the threads interface changed between julia versions. To have an idea how it is done nowadays, have a look at https://github.com/JuliaLang/julia/blob/master/base/pcre.jl#L34. It is also not clear how performance will be effected, but since there are locks and try/catch, I have a feeling that performance for something like x + y will suffer noticeably.

Regarding roots(f, RealField) versus roots(f, RealField()), from experience I know people will try the former and be confused if it doesn't work. So it would be nice if that at least printed a helpful error message, though at that point as a user one may wonder why it can't just "do what I want" if it clearly has guessed my intention right (a possible answer of course is: "because passing the type may not work in other situations and we don't want you to form bad habits, and instead learn how to do it right". Some users will appreciate this kind of sentiment more than others (to put it mildly).

OK. I think we will go with the helpful error message.

thofma · 2023-01-08T10:22:59Z

OK, next iteration. We keep the old ArbField/arb, AcbField/acb and call them "fixed precision ball arithmetic". The new "arbitrary precision ball arithmetic" is done using the following types:

Element	Parent
`RealElem`	`RealField`
`RealMat`	`RealMatSpace`
`RealPoly`	`RealPolyRing`
`ComplexElem`	`ComplexField`
`ComplexMat`	`ComplexMatSpace`
`ComplexPoly`	`ComplexPolyRing`

I don't want to hear any discussions about the names. If they need to be adjusted, it can be done during the "big rename" (@fieker, I could not use Real or Complex since this is already an exported type in julia Base).

This is still breaking, since before we had RealField = ArbField and ComplexField = AcbField.

For this PR I also don't care about coverage, since I just moved things around.

fieker · 2023-01-08T14:28:42Z

Sounds good.

…

On Sun, 8 Jan 2023, 11:23 Tommy Hofmann, ***@***.***> wrote: OK, next iteration. We keep the old ArbField/arb, AcbField/acb and call them "fixed precision ball arithmetic". The new "arbitrary precision ball arithmetic" is done using the following types: Parent Element RealField RealElem RealMat RealMatSpace RealPoly RealPolyRing ComplexField ComplexElem ComplexMat ComplexMatSpace ComplexPoly ComplexPolyRing I don't want to hear any discussions about the names. If they need to be adjusted, it can be done during the "big rename" ***@***.*** <https://github.com/fieker>, I could not use Real or Complex since this is already an exported type in julia Base). This is still breaking, since before we had RealField = ArbField and ComplexField = AcbField. For this PR I also don't care about coverage, since I just moved things around. — Reply to this email directly, view it on GitHub <#1367 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA36CV4FWRYRILMBWUTJKXLWRKIQ3ANCNFSM6AAAAAATH2R2NM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

fingolfin · 2023-02-22T08:31:51Z

Regarding thread safety, it is slightly more complicated since julia now allows to change the number of threads at runtime. It is also painful since the threads interface changed between julia versions. To have an idea how it is done nowadays, have a look at https://github.com/JuliaLang/julia/blob/master/base/pcre.jl#L34. It is also not clear how performance will be effected, but since there are locks and try/catch, I have a feeling that performance for something like x + y will suffer noticeably.

Note that the try/catch and lock are only in the "slow path", which allocates the TLS data the first time it is accessed in any given thread. So they are negligible. I am not saying there won't be an overhead, just that it might not be that bad; we should measure. Although, I don't see an alternative? Unless we are willing to risk crazy threading bugs?

thofma added 2 commits December 23, 2022 15:26

towards single parent balls

9ad6981

add missing types

718ecea

thofma added the breaking label Jan 1, 2023

fingolfin reviewed Jan 2, 2023

View reviewed changes

thofma added 4 commits January 7, 2023 15:07

Reorganize

eb95c81

missing files

842cb65

Documentation

7e95409

fix

3d3836c

thofma marked this pull request as ready for review January 8, 2023 10:23

thofma merged commit eefcf5b into master Feb 20, 2023

thofma deleted the th/arb branch February 20, 2023 16:48

thofma mentioned this pull request Feb 21, 2023

The big renaming #1376

Merged

4 tasks

lgoettgens mentioned this pull request Sep 18, 2023

Fix ComplexPolyRing definition #1538

Merged

fieker mentioned this pull request Nov 24, 2023

CalciumField -> ArbField ignores precision #1580

Closed

fingolfin mentioned this pull request Apr 5, 2024

Precision fixes for some code dealing with ArbField/AcbField #1707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Towards single parent balls #1367

Towards single parent balls #1367

thofma commented Dec 23, 2022 •

edited

Loading

codecov bot commented Jan 1, 2023 •

edited

Loading

thofma commented Jan 1, 2023

fredrik-johansson commented Jan 1, 2023

thofma commented Jan 1, 2023 •

edited

Loading

fingolfin Jan 2, 2023

fingolfin Jan 2, 2023

fingolfin commented Jan 2, 2023

fieker commented Jan 2, 2023 via email

fieker commented Jan 2, 2023 via email

thofma commented Jan 2, 2023 •

edited

Loading

thofma commented Jan 8, 2023 •

edited

Loading

fieker commented Jan 8, 2023 via email

fingolfin commented Feb 22, 2023

Towards single parent balls #1367

Towards single parent balls #1367

Conversation

thofma commented Dec 23, 2022 • edited Loading

TODO

codecov bot commented Jan 1, 2023 • edited Loading

Codecov Report

thofma commented Jan 1, 2023

fredrik-johansson commented Jan 1, 2023

thofma commented Jan 1, 2023 • edited Loading

fingolfin Jan 2, 2023

Choose a reason for hiding this comment

fingolfin Jan 2, 2023

Choose a reason for hiding this comment

fingolfin commented Jan 2, 2023

fieker commented Jan 2, 2023 via email

fieker commented Jan 2, 2023 via email

thofma commented Jan 2, 2023 • edited Loading

thofma commented Jan 8, 2023 • edited Loading

fieker commented Jan 8, 2023 via email

fingolfin commented Feb 22, 2023

thofma commented Dec 23, 2022 •

edited

Loading

codecov bot commented Jan 1, 2023 •

edited

Loading

thofma commented Jan 1, 2023 •

edited

Loading

thofma commented Jan 2, 2023 •

edited

Loading

thofma commented Jan 8, 2023 •

edited

Loading