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

Test type-stability of functions #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

giordano
Copy link
Member

Use @inferred to test type-stability of functions.

  • besselj and bessely are unstable in Julia 0.6 and 0.7.
  • digamma, trigamma, invdigamma, polygamma, eta, zeta are unstable in Julia 0.7 only.

Type-instability of digamma was already reported in issue #42.

@giordano
Copy link
Member Author

giordano commented Aug 12, 2017

I believe the instabilities in besselj and bessely are caused by the conditionals

if typemin(Cint) <= nu <= typemax(Cint)

for which there is no else clause, so the return type is probably undetermined (actually, it should be an error).

The tests pass in Julia 0.6 with the following patch:

diff --git a/src/bessel.jl b/src/bessel.jl
index 958ac37..4f52082 100644
--- a/src/bessel.jl
+++ b/src/bessel.jl
@@ -388,6 +388,8 @@ function besselj(nu::Real, x::AbstractFloat)
     if isinteger(nu)
         if typemin(Cint) <= nu <= typemax(Cint)
             return besselj(Cint(nu), x)
+        else
+            error()
         end
     elseif x < 0
         throw(DomainError(x, "`x` must be nonnegative."))
@@ -443,8 +445,12 @@ Bessel function of the second kind of order `nu`, ``Y_\\nu(x)``.
 function bessely(nu::Real, x::AbstractFloat)
     if x < 0
         throw(DomainError(x, "`x` must be nonnegative."))
-    elseif isinteger(nu) && typemin(Cint) <= nu <= typemax(Cint)
-        return bessely(Cint(nu), x)
+    elseif isinteger(nu)
+        if typemin(Cint) <= nu <= typemax(Cint)
+            return bessely(Cint(nu), x)
+        else
+            error()
+        end
     end
     real(bessely(float(nu), complex(x)))
 end

Well, in place of error() there should be a more meaningful error (AmosException?)

@musm
Copy link
Contributor

musm commented Aug 12, 2017

Excellent catch @giordano

@musm
Copy link
Contributor

musm commented Aug 12, 2017

Hmm actually I think the problem is a little different. The problem is not with the conditionals but rather

bessely(nu::Real, z::Complex) in SpecialFunctions at C:\Users\Mus\.julia\v0.7\SpecialFunctions\src\bessel.jl:470

always return a Float64, which throws off type stability of bessely(nu::Real, x::AbstractFloat) and this is due to the promotion Tf = promote_type(float(typeof(nu)),float(typeof(real(z))))

@giordano
Copy link
Member Author

giordano commented Aug 12, 2017

Uhm, not sure I understand what you mean:

julia> besselj(-3f0, complex(3f0))
-0.30906272f0 - 3.7849267f-17im

julia> bessely(-3f0, complex(3f0))
0.5385416f0 + 0.0f0im

are correctly Complex{Float32}.

Maybe the issue is how those methods are called? float(nu) looks a bit suspicious: when the conditional is false but nu::Int then float(nu) is always Float64, whatever the type of x

@musm
Copy link
Contributor

musm commented Aug 12, 2017

Consider

julia>  x = 2f0
2.0f0

julia> nu = 2.0
2.0

julia>  bessely(nu,x)
-0.6174081f0

This calls bessely(Cint(nu), x) which has the correct return type (within the isinteger(nu) && typemin(Cint) <= nu <= typemax(Cint)) branch. However

julia>     real(bessely(float(nu), complex(x)))
-0.6174081041906828

which is called on line 449 and this causes the type instability

@giordano
Copy link
Member Author

So we're saying the same thing 🙂

@musm
Copy link
Contributor

musm commented Aug 12, 2017

Basically, but I don't think the patch you propose actually fixes the type instability; may need to stick oftype(x, real(bessely(float(nu), complex(x)))) on line 449, which fixes it at least for Float32 inputs, but maybe there is a cleaner fix.

@musm
Copy link
Contributor

musm commented Aug 14, 2017

Anyways I think the discussion got sidetracked, since these are besides the point of this PR.

@cossio
Copy link
Contributor

cossio commented May 14, 2020

bump

@ViralBShah
Copy link
Member

@giordano Should we get this merged? If so, would it be possible for you to rebase?

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.

4 participants