-
Notifications
You must be signed in to change notification settings - Fork 117
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 issue 508 #509
Fix issue 508 #509
Conversation
lib/YaoBlocks/src/utils.jl
Outdated
@@ -315,7 +315,7 @@ Base.vec(et::EntryTable) = Vector(et) | |||
# convert a (maybe complex) number x to real number. | |||
function safe_real(x) | |||
img = imag(x) | |||
if !(iszero(img) || isapprox(x - im*img, x)) | |||
if !(iszero(img) || isapprox(x - im*img, x; atol=1e-15)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, defining a function on AbstractFloat
causes Dual
number input run into the other function. Got an error in the AD tests.
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. |
In what case would the proposed behaviour not be desirable? Grepping through the code Its behaviour is i) return its input if |
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 |
I personally would be happy with either option, I guess do whichever is more standard when doing numerical computations? |
The problem is we cannot know how large this a) forward these two options in expect(...; atol=..., rtol=...) b) if user turns on 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. |
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 @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. |
that sounds good to me. |
fix #508 by setting a looser threshold.