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

Internals of BigFloat getting changed in 1.12 #485

Closed
KristofferC opened this issue Feb 10, 2025 · 13 comments
Closed

Internals of BigFloat getting changed in 1.12 #485

KristofferC opened this issue Feb 10, 2025 · 13 comments

Comments

@KristofferC
Copy link
Member

See JuliaLang/julia#55906.

Now causes

Error During Test at /home/pkgeval/.julia/packages/SpecialFunctions/Zijv9/test/gamma_inc.jl:231
  Test threw exception
  Expression: gamma(a, x) ≈ gamma(big(a), big(x))
  not compatible with mpfr
  Stacktrace:
    [1] error(s::String)
      @ Base ./error.jl:44
    [2] unsafe_convert(::Type{Ref{BigFloat}}, x::Base.RefValue{BigFloat})
      @ Base.MPFR ./mpfr.jl:212
    [3] _gamma_big(a::BigInt, x::BigFloat)
      @ SpecialFunctions ~/.julia/packages/SpecialFunctions/Zijv9/src/gamma_inc.jl:1159
    [4] _gamma
      @ ~/.julia/packages/SpecialFunctions/Zijv9/src/gamma_inc.jl:1147 [inlined]
    [5] gamma(a::BigInt, x::BigFloat)
      @ SpecialFunctions ~/.julia/packages/SpecialFunctions/Zijv9/src/gamma_inc.jl:1120
    [6] macro expansion
      @ /opt/julia/share/julia/stdlib/v1.12/Test/src/Test.jl:676 [inlined]
    [7] (::var"#14#15")()
      @ Main ~/.julia/packages/SpecialFunctions/Zijv9/test/gamma_inc.jl:231
    [8] setprecision(f::var"#14#15", ::Type{BigFloat}, prec::Int64; base::Int64)
      @ Base.MPFR ./mpfr.jl:1180
    [9] setprecision(f::Function, ::Type{BigFloat}, prec::Int64)
      @ Base.MPFR ./mpfr.jl:1179
   [10] top-level scope
      @ ~/.julia/packages/SpecialFunctions/Zijv9/test/gamma_inc.jl:229
   [11] macro expansion
      @ /opt/julia/share/julia/stdlib/v1.12/Test/src/Test.jl:1771 [inlined]
   [12] macro expansion
      @ ~/.julia/packages/SpecialFunctions/Zijv9/test/gamma_inc.jl:229 [inlined]
   [13] include(mapexpr::Function, mod::Module, _path::String)
      @ Base ./Base.jl:308
   [14] macro expansion
      @ ~/.julia/packages/SpecialFunctions/Zijv9/test/runtests.jl:45 [inlined]
   [15] macro expansion
      @ /opt/julia/share/julia/stdlib/v1.12/Test/src/Test.jl:1771 [inlined]
@inkydragon
Copy link
Member

Simple MWE:

julia> using SpecialFunctions
Precompiling SpecialFunctions finished.
  1 dependency successfully precompiled in 2 seconds. 17 already precompiled.

julia> gamma(big(1.), 0)
1.0

julia> gamma(big(1), 0)
ERROR: not compatible with mpfr
Stacktrace:

@stevengj
Copy link
Member

stevengj commented Feb 11, 2025

The current implmentation is:

using Base.MPFR: MPFRRoundingMode, ROUNDING_MODE

function _gamma(x::BigFloat)
    isnan(x) && return x
    z = BigFloat()
    ccall((:mpfr_gamma, :libmpfr), Int32, (Ref{BigFloat}, Ref{BigFloat}, Int32), z, x, ROUNDING_MODE[])
    isnan(z) && throw(DomainError(x, "NaN result for non-NaN input."))
    return z
end

The current master still passes things as Ref{BigFloat}. It seems like the rounding mode should be passed as:

 ccall((:mpfr_gamma, :libmpfr), Int32, (Ref{BigFloat}, Ref{BigFloat}, MPFRRoundingMode), z, x, Base.Rounding.rounding_raw(BigFloat))

but this shouldn't change anything.

We should also change :libmpfr to Base.MPFR.libmpfr,

What else should change?

@giordano
Copy link
Member

I think you're looking at the wrong method? The failing one is

function _gamma_big(a::Real,x::Real)
if x < 0
# MPFR returns NaN in this case
if isinteger(a) && a > 0
return invoke(_gamma, Tuple{Number,Number}, a, BigFloat(x))
else
throw(DomainError((a, x), "gamma will only return a complex result if called with a complex argument"))
end
end
z = BigFloat()
ccall((:mpfr_gamma_inc, :libmpfr), Int32, (Ref{BigFloat}, Ref{BigFloat}, Ref{BigFloat}, Int32), z, a, x, ROUNDING_MODE[])
return z
end
for the incomplete gamma function: #485 (comment)

@giordano
Copy link
Member

I believe the issue is that x::Int is passed in the ccall as x::Ref{BigFloat} when x >= 0. Julia v1.11:

julia> ccall((:mpfr_gamma_inc, :libmpfr), Int32, (Ref{BigFloat}, Ref{BigFloat}, Ref{BigFloat}, Int32), BigFloat(0.0), BigFloat(1.0), 1.0, Base.MPFR.ROUNDING_MODE[])
-1

julia nightly:

julia> ccall((:mpfr_gamma_inc, :libmpfr), Int32, (Ref{BigFloat}, Ref{BigFloat}, Ref{BigFloat}, Int32), BigFloat(0.0), BigFloat(1.0), 1.0, Base.MPFR.ROUNDING_MODE[])
ERROR: not compatible with mpfr
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:44
 [2] unsafe_convert(::Type{Ref{BigFloat}}, x::Base.RefValue{BigFloat})
   @ Base.MPFR ./mpfr.jl:215
 [3] top-level scope
   @ ./REPL[6]:1

@stevengj
Copy link
Member

stevengj commented Feb 11, 2025

Shouldn't Base.cconvert take care of that? Base.cconvert(Ref{BigFloat}, 1.0) works on 1.11

@giordano
Copy link
Member

giordano commented Feb 11, 2025

Based on the stacktrace we're hitting this unsafe_convert method, introduced by the PR linked above. The Base.cconvert is the argument to Bsae.unsafe_convert

@giordano
Copy link
Member

Basically, 1.11:

julia> Base.unsafe_convert(Ref{BigFloat}, Base.cconvert(Ref{BigFloat}, 1.0))
Ptr{BigFloat} @0x00007fd8cd79c460

master:

julia> Base.unsafe_convert(Ref{BigFloat}, Base.cconvert(Ref{BigFloat}, 1.0))
ERROR: not compatible with mpfr
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:44
 [2] unsafe_convert(::Type{Ref{BigFloat}}, x::Base.RefValue{BigFloat})
   @ Base.MPFR ./mpfr.jl:215
 [3] top-level scope
   @ REPL[7]:1

@stevengj
Copy link
Member

Why can't cconvert and/or unsafe_convert be updated to support this?

@giordano
Copy link
Member

Since this was specifically introduced in the PR which reworked BigFloat this looks completely deliberate.

@stevengj
Copy link
Member

stevengj commented Feb 11, 2025

In particular, couldn't we just have:

Base.cconvert(::Type{Ref{BigFloat}}, x::Number) = convert(BigFloat, x).d

?

This mirrors the behavior in earlier versions, where cconvert creates a BigFloat object.

@giordano
Copy link
Member

This mirrors the behavior in earlier versions, where cconvert creates a BigFloat object.

It still does.

@stevengj
Copy link
Member

stevengj commented Feb 11, 2025

The problem is that cconvert(Ref{BigFloat}, x) calls convert(Ref{BigFloat}, x) (via the default fallback method), and that is no longer the correct conversion in the context of a ccall.

Doing this right is the whole job of cconvert, so this seems like a bug in cconvert on master: JuliaLang/julia#57367 (which makes _gamma_big work again).

@KristofferC
Copy link
Member Author

Fixed in JuliaLang/julia#57367

KristofferC pushed a commit to JuliaLang/julia that referenced this issue Feb 12, 2025
#55906 changed `cconvert(Ref{BigFloat}, x::BigFloat)` to return `x.d`,
but neglected to do so for other types of `x`, where it still returns a
`Ref{BigFloat}` and hence is now returning the wrong type for `ccall`.

Not only does this break backwards compatibility
(JuliaMath/SpecialFunctions.jl#485), but it
also seems simply wrong: the *whole* job of `cconvert` is to convert
objects to the correct type for use with `ccall`. This PR does so (at
least for `Number` and `Ref{BigFloat}`).
KristofferC pushed a commit to JuliaLang/julia that referenced this issue Feb 13, 2025
#55906 changed `cconvert(Ref{BigFloat}, x::BigFloat)` to return `x.d`,
but neglected to do so for other types of `x`, where it still returns a
`Ref{BigFloat}` and hence is now returning the wrong type for `ccall`.

Not only does this break backwards compatibility
(JuliaMath/SpecialFunctions.jl#485), but it
also seems simply wrong: the *whole* job of `cconvert` is to convert
objects to the correct type for use with `ccall`. This PR does so (at
least for `Number` and `Ref{BigFloat}`).

(cherry picked from commit 6dca4f4)
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

No branches or pull requests

4 participants