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

Fix #11512 (disable computation for Complex BigInt/BigFloat type) #14979

Closed
wants to merge 1 commit into from

Conversation

lvnguyen
Copy link
Contributor

@lvnguyen lvnguyen commented Feb 7, 2016

We implement an explicit type checking and disable Complex{BigInt} and Complex{BigFloat}.
Currently only do for Bessel and Airy function.

@yuyichao
Copy link
Contributor

yuyichao commented Feb 7, 2016

Clarification on the way the error is raised. It is used to avoid the GC frame introduded due to string interpolation in the error branch. Another way is to have a non-inline function to throw the error instead.

@@ -370,7 +378,14 @@ for f in ("i", "ix", "j", "jx", "k", "kx", "y", "yx")
bfn = symbol("bessel", f)
@eval begin
$bfn(nu::Real, z::Complex64) = Complex64($bfn(Float64(nu), Complex128(z)))
$bfn(nu::Real, z::Complex) = $bfn(Float64(nu), Complex128(z))
$bfn{T}(nu::Real, z::Complex{T}) = if T === BigInt
throw(ArgumentError("Bessel is currently not supported for type Complex{BigInt} (see #11512)"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the style guidelines in CONTRIBUTING.md specify 4-space indent, not tabs. It's also preferable to use function syntax once the definition requires multiple lines.

@lvnguyen lvnguyen force-pushed the ln/j11512 branch 2 times, most recently from a9d87d5 to edea98f Compare February 7, 2016 19:43
@vtjnash
Copy link
Member

vtjnash commented Feb 8, 2016

In #11512, only BigFloat is mentioned. I don't know if Complex{BigInt} is broken, since the truncating convert should throw an error if it can't be represented in the target type.

function airy{T}(k::Number, z::Complex{T})
if T === BigInt
throw(ArgumentError("Airy is currently not supported for type Complex{BigInt} (see #11512)"))
elseif T == BigFloat
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one of == or === should be used consistently here

@simonbyrne
Copy link
Contributor

I think this would be handled via dispatch: for airy we should just get rid of the generic airy(k::Number, z::Complex) signature. Also, since the first argument is always an integer, it would probably make sense to change Number to Integer.

The Bessel case is a bit more complicated, but we should be able to do this via promote.

@yuyichao
Copy link
Contributor

yuyichao commented Feb 8, 2016

How can this work with the generic signature removed? I don't think we want to define airy(::Int, ::Complex{T}) individually for T equals to Int8, UInt8, Int16, UInt16 ......

@yuyichao
Copy link
Contributor

yuyichao commented Feb 8, 2016

And I think it is better to not define a method to raise error for BigFloat since it will prevent packages from providing this function without annoy warnings.

…type)

We implement an explicit type checking and disable Complex{BigInt} and Complex{BigFloat}.
Currently only do for Bessel and Airy function.
@simonbyrne
Copy link
Contributor

How can this work with the generic signature removed? I don't think we want to define airy(::Int, ::Complex{T}) individually for T equals to Int8, UInt8, Int16, UInt16 ......

@yuyichao Why would we need to do that? We can just define

airy{T<:Integer}(k::Integer, z::Complex{T}) = airy(k, float(z))

@simonbyrne
Copy link
Contributor

Actually, we could keep it generic, with just

airy(k::Integer, z::Complex) = airy(k, float(z))

@yuyichao
Copy link
Contributor

yuyichao commented Feb 8, 2016

There's also Rational{Int} etc... and I think a sane default in base is to promote to float for all types, while rasing error for big types in base but still allow more precise implementation to be provided by packages.

@simonbyrne
Copy link
Contributor

Agreed, but we should be able to do this via dispatch, rather than have explicit branches in functions.

@simonbyrne
Copy link
Contributor

What should work is:

airy(k::Integer, z::Complex) = airy(k, float(z))
airy(k::Integer, z::Complex{BigFloat}) = throw(MethodError(airy, typeof((k,z)))

@yuyichao
Copy link
Contributor

yuyichao commented Feb 9, 2016

No, it will make it impossible to support that in a package. The branch is written in a way that is constant folded away and generate not runtime overhead.

@simonbyrne
Copy link
Contributor

How will that make it impossible to support? Just re-define Base.airy(k::Integer, z::Complex{BigFloat})

The whole point of multiple dispatch is that we don't need branches on types cluttering up our code!

@yuyichao
Copy link
Contributor

yuyichao commented Feb 9, 2016

It will give a warning.

@simonbyrne
Copy link
Contributor

Is that a huge problem? I'm willing to pay that to avoid 6 lines of completely useless boilerplate code

Alternatively, we could do it via

airy(k::Integer, z::Complex) = _airy(k, float(z))

where _airy is the "internal" function

@simonbyrne
Copy link
Contributor

For other functions which have the same problem (e.g. erfinv), we don't actually define fully-generic methods, just Integer ones.

@andreasnoack
Copy link
Member

For other functions which have the same problem (e.g. erfinv), we don't actually define fully-generic methods, just Integer ones.

That doesn't seem right to me. A more generic solution would be something like airy(k::Integer, z::Number) = airy(k, promote_type(typeof(z),Float32))(z)).

@simonbyrne
Copy link
Contributor

@andreasnoack That's the same as calling float, isn't it? The problem with that is that Complex{BigFloat} arguments get stuck in an infinite loop instead of throwing an error.

@yuyichao
Copy link
Contributor

yuyichao commented Feb 9, 2016

The whole point of multiple dispatch is that we don't need branches on types cluttering up our code!

Certain logic is still much cleaner when expressed as branches. This one can indeed use a float on the argument and only define the internal function for Complex64 and Complex128 and also promote Complex{Float16} in the way that is compatible to what we are doing now.

For promote_type, we'll want to promote it to Float64 (which is what we do now) and use the internal function _airy instead since it can be a infinite loop otherwise.

@yuyichao
Copy link
Contributor

yuyichao commented Feb 9, 2016

And if we do it this way, we should probably leave a comment about the intended behavior.

@andreasnoack
Copy link
Member

You are right. Time for more coffee. I think an explicit MethodError method for Complex{BigFloat} is the best solution.

@simonbyrne
Copy link
Contributor

Actually, a better solution might be

airy(k::Integer, z::Complex) = airy(k, float(z))
airy{T<:AbstractFloat}(k::Integer, z::Complex{T}) = throw(MethodError(airy, typeof((k,z)))

This should then (1) avoid throwing a warning if defined for BigFloat, and (2) make it easy to define new AbstractFloat types.

@simonbyrne
Copy link
Contributor

I've made the changes in #15119 instead.

@simonbyrne simonbyrne closed this Feb 26, 2016
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.

6 participants