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

More special, eigen, and bitarray tests #12121

Merged
merged 4 commits into from
Jul 14, 2015
Merged

More special, eigen, and bitarray tests #12121

merged 4 commits into from
Jul 14, 2015

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Jul 11, 2015

Got error throwing to work for eigmin and eigmax. Added tests for a bunch of untested branches in log and airy and erfinv.

@kshyatt kshyatt added test This change adds or pertains to unit tests linear algebra Linear algebra maths Mathematical functions labels Jul 11, 2015
@@ -90,11 +90,17 @@ end
#Computes maximum and minimum eigenvalue
function eigmax(A::Union{Number, StridedMatrix}; permute::Bool=true, scale::Bool=true)
v = eigvals(A, permute = permute, scale = scale)
iseltype(v,Complex) ? error("DomainError: complex eigenvalues cannot be ordered") : maximum(v)
if iseltype(v,Complex)
throw(DomainError())
Copy link
Contributor

Choose a reason for hiding this comment

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

should this still have a message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It throws an error with the message.

Copy link
Contributor

Choose a reason for hiding this comment

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

but a generic domain error, as opposed to the specific "complex eigenvalues cannot be ordered" information right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused. If you put the string between the parens it will freak out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, bummer, DomainError doesn't have an extra message field like I thought it would. (So

throw(DomainError("x must be positive semidefinite"))
is probably untested.) Throwing a more specific exception type is good, but so is providing additional info to the user. Looks like DomainErrors get specially handled here

julia/base/replutil.jl

Lines 96 to 113 in 71f08f3

function showerror(io::IO, ex::DomainError, bt; backtrace=true)
print(io, "DomainError:")
for b in bt
code = ccall(:jl_lookup_code_address, Any, (Ptr{Void}, Cint), b-1, true)
if length(code) == 5 && !code[4] # code[4] == fromC
if code[1] in (:log, :log2, :log10, :sqrt) # TODO add :besselj, :besseli, :bessely, :besselk
println(io,"\n$(code[1]) will only return a complex result if called with a complex argument.")
print(io, "try $(code[1]) (complex(x))")
elseif (code[1] == :^ && code[2] == symbol("intfuncs.jl")) || code[1] == :power_by_squaring
println(io, "\nCannot raise an integer x to a negative power -n. Make x a float by adding")
print(io, "a zero decimal (e.g. 2.0^-n instead of 2^-n), or write 1/x^n, float(x)^-n, or (x//1)^-n")
end
break
end
end
backtrace && show_backtrace(io, bt)
nothing
end

Copy link
Contributor

Choose a reason for hiding this comment

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

The untested cholesky method sure (probably better to use PosDefException for that anyway), though the loss of information in this error (and the nonstandard indentation on this line) are unfortunate, and would be easy to forget to revisit. Maybe a TODO comment?

Copy link
Member

Choose a reason for hiding this comment

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

I think it can make an exception much more costly if contains a string (or any other pointer to a Julia object). These costs are probably okay for expensive linear algebra functions but not in scalar math functions such as sqrt where DomainError is used.

Copy link
Member

Choose a reason for hiding this comment

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

We could have DomainError{T<:Union(None,ByteString)} where DomainError() would be a DomainError{None}. I don't think that would be any more expensive when there's no string to be thrown. Would need some changes on the codegen side, though...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the right approach is just to fix the performance issue. Ref #11508

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure removing the GC frame would be enough. It would eliminate the additional cost for a DomainError with a string parameter in code that doesn't throw a DomainError, but you'd still have additional overhead if you actually throw a DomainError, which might matter.

A = bitrand(10)
B = falses(10)
fA = bitunpack(A)
fB = bitunpack(B)
Copy link
Member

Choose a reason for hiding this comment

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

B and fB seem to be unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'll remove and rebase. Thanks for catching this.

@kshyatt
Copy link
Contributor Author

kshyatt commented Jul 14, 2015

Anyone know what the deal is with that OSX failure? Does this look ok?

@tkelman
Copy link
Contributor

tkelman commented Jul 14, 2015

#12127. Still need to decide what to do about https://github.com/JuliaLang/julia/pull/12121/files#r34417008 - at a bare minimum, fix the indentation and put in a TODO comment to put the informational error message back

@kshyatt
Copy link
Contributor Author

kshyatt commented Jul 14, 2015

The TODO is already there. I'll fix my indenting.

@tkelman, is everything ok with you?

@tkelman
Copy link
Contributor

tkelman commented Jul 14, 2015

Still a bit disappointed about losing the complex eigenvalues cannot be ordered information in the error, but I guess it's alright for now.

@kshyatt
Copy link
Contributor Author

kshyatt commented Jul 14, 2015

@tkelman my impression was that we (someone?) need to make a decision about DomainError that's kind out of out-of-scope for this PR. Would you agree? I can open an issue for that and mention this PR so putting the info back in doesn't fall off the radar.

@tkelman
Copy link
Contributor

tkelman commented Jul 14, 2015

ok

kshyatt added a commit that referenced this pull request Jul 14, 2015
More special, eigen, and bitarray tests
@kshyatt kshyatt merged commit 478461a into master Jul 14, 2015
@kshyatt kshyatt deleted the ksh/mathtests branch July 14, 2015 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra maths Mathematical functions test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants