Fix issue 508

[GiggleLiu]

fix #508 by setting a looser threshold.

[jlbosse]

What stops us from doing something like atol=eps(x)? Is it the fact that eps might not be defined for x? If yes, how about defining

function safe_real(x)
    # like what we had previously
end

function safe_real(x::Union{AbstractFloat,Complex{<:AbstractFloat}})
    # like in this PR
end

[GiggleLiu]

Using eps might be too strict.

julia> eps(0.0)
5.0e-324

But you are right, eps may not be defined for e.g. symbolic types. Maybe implementing two interfaces is better.

[GiggleLiu]

Oops, defining a function on AbstractFloat causes Dual number input run into the other function. Got an error in the AD tests.

[Roger-luo]

I don't like this change, this is too heurestic, we should really just let user decide what precision they want. And only guarantee on throw or nothrow behaviour.

[jlbosse]

In what case would the proposed behaviour not be desirable? Grepping through the code safe_real(x::T) is currently only used in situations where we would expect x to be real up to precision eps(T) since it is the output of sum large-ish calculation that would be exactly real if we had no floating point error.

Its behaviour is i) return its input if x is real, ii) return the real part of x if the imaginary part is zero up to eps(T) if eps(T) is defined and iii) return the real part of x if the imaginary part is exactly zero if eps(T) is not defined. To me this is (with the exception of maybe iii) ) exactly the behaviour I would find sensible. And iii) would fail loudly if we get something with tiny, but non-zero imaginary part.

[GiggleLiu]

The only issue is, the eps, which is defined by the gap between two floating point numbers, might be a bit too strict. What about using sqrt(eps(T)) or 10 * eps(T)?
I am fine to make it a keyword argument so that users can tune it when the program errors unexpectedly.

[jlbosse]

I personally would be happy with either option, I guess do whichever is more standard when doing numerical computations?

[Roger-luo]

The problem is we cannot know how large this eps means fine. This is exactly why you have rtol and atol in isapprox to control such with a good default if you want an approximate behaviour. What I'm proposing is either:

a) forward these two options in expect;

expect(...; atol=..., rtol=...)

b) if user turns on nothrow=True, fallback to the unsafe real conversion and let users decide what they want to do with the result

expect(...; nothrow=True) # this is explicitly saying the calculation will not throw and may not be safe

@jlbosse for #508 , it is exactly the case of an unexpected throw when someone expects it to go through by just ignoring the imaginary part despite of how large it is.

[GiggleLiu]

Ok, I will assume users know what they are doing. I will throw a warning for large floating point errors to let user know they might want to use sandwich function instead. Then expect will never throw error.

@Roger-luo I think the interface that you proposed is too complicated. When the inputs are non-floating point types, it does not make sense to set eps.

[Roger-luo]

I will throw a warning for large floating point errors to let user know they might want to use sandwich function instead. Then expect will never throw error.

that sounds good to me.