JuliaPy / SymPy.jl

Julia interface to SymPy via PyCall
http://juliapy.github.io/SymPy.jl/
MIT License
269 stars 61 forks source link

Incorrect evaluation of ratio of Sym integers #372

Closed mzaffalon closed 4 years ago

mzaffalon commented 4 years ago

I do not know how to fix https://github.com/JuliaPy/SymPy.jl/blob/master/src/numbers.jl#L286-L287 so that the following rational is evaluated to a BigFloat:

julia> a = Sym(13)/17;
julia> typeof(N(a, 256))
Float64
mzaffalon commented 4 years ago

Maybe

p = round(Int,log2(10)*digits)
return BigFloat(N(numer(x)),p) / BigFloat(N(denom(x)), p)
jverzani commented 4 years ago

I think that entire function should do the conversion within sympy, and then call N, as with:

julia> a = Sym(13)/17;

julia> a.evalf(200) |> N
0.7647058823529411764705882352941176470588235294117647058823529411764705882352961

That would clean up that N(x,digits) code and be more inline with expectations of sympy users.

jverzani commented 4 years ago

Yeah, there was a bug for rational numbers. I think this fix works better (it essentially does your suggestion), as my suggestion was too simplistic.

mzaffalon commented 4 years ago

In [24249f21] SymPy v1.0.29, SymPy reals and rationals are computed to different amount of digits:

julia> N(Sym(13)/17, 256).prec
256
julia> N(Sym(2.5), 256).prec
850

For the two to have the same behavior, the code should either use what I propose above for rationals

p = round(Int,log2(10)*digits)
return BigFloat(N(numer(x)),p) / BigFloat(N(denom(x)), p)

or evaluate a SymPy real as BigFloat(x, digits; kwargs) as you are now doing for rationals. I prefer the current approach for real.

jverzani commented 4 years ago

Maybe this, so that number if digits matches up with digits.

  p = round(Int, log2(10)*digits)
        ex =  x.evalf(1+digits)
        out = setprecision(p) do
            convert(BigFloat, ex)
        end
        return(out)

Somehow I missed BigFloat(a, p) and used this setprecision style. Which is odd, as there appears to be a slight difference so figure I should do both the same way.

Thanks for pointing out the errors in my ways.

jverzani commented 4 years ago

Closed by #374 (second time is charm?)