Nemocas / Nemo.jl

Julia bindings for the FLINT number theory C library
http://nemocas.github.io/Nemo.jl/
Other
189 stars 58 forks source link

Three-arg arithmetic for Arb types instead changes precision #1925

Open lgoettgens opened 1 day ago

lgoettgens commented 1 day ago
julia> RR = RealField()
Real field

julia> x = RR(1)/3
[0.3333333333333333333 +/- 4.24e-20]

julia> y = RR(2)/3
[0.6666666666666666666 +/- 8.48e-20]

julia> x+y
[1.00000000000000000 +/- 1.36e-19]

julia> x+y+2
[+/- 1.01]

the result in the last line is x+y with a precision of 2, instead of the expected x+y+2 with precision(Balls).

Having prec as a third positional argument is not documented anywhere and makes things like https://github.com/Nemocas/Nemo.jl/issues/1812 unnecessarily complicated as prec has to be passed around all the time. I thus propose to remove this argument instead of making it a positional one. I'll probably already do this as part of https://github.com/Nemocas/Nemo.jl/pull/1924.

thofma commented 1 day ago

Ah, the pointless lowering to +(...) strikes again. Should probably be a keyword argument instead of being removed.

lgoettgens commented 1 day ago

Another indicator to remove it: in composite types like RealPoly and RealMat, this always uses precision(Balls) for all arithmetic, and has no local way to insert it.

A workaround to get the same is set_precision(Balls, new_prec) do <arith here> end as this changes the value of precision(Balls) for this local scope.

thofma commented 1 day ago

I still want to do sin(R(2), prec = 1000) and not go via set_precision(...). It is the same reason we allow RR(2, precision = 200).

lgoettgens commented 1 day ago

Ok. then I'll go ahead and change all positional arguments prec that have a default of precision(Balls) to be a kwarg named precision instead (name change to be consistent with RR(2; precision = 200)). Fine with that?

thofma commented 1 day ago

perfect, thanks

I wonder if we need to deprecate this properly? Or would the argument against it be that it was not documented?

lgoettgens commented 1 day ago

I really wanna get rid of the wrong behavior as in the initial message, so at least for the arithmetic this should go completely. For things like sqrt or sin I have no preference (apart from doing proper deprecations is a lot of work)

lgoettgens commented 1 day ago

another question: for functions like add!, do we want to convert it to a kwarg as well? I am asking because in a similar case in https://github.com/Nemocas/Nemo.jl/pull/1910#discussion_r1816678305 the vote was for a positional argument for set! for additional data that flint needs.

thofma commented 1 day ago

yes, I agree that for the arithmetic functions this is a bugfix. I personally don't mind if you switch to precision = in a breaking release.

I think set! is not supposed to be user-facing right? I would prefer to have it as a positional argument there.