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

sparse.^0 incorrect #77

Closed
tkelman opened this issue Feb 15, 2014 · 7 comments
Closed

sparse.^0 incorrect #77

tkelman opened this issue Feb 15, 2014 · 7 comments
Labels
bug Something isn't working

Comments

@tkelman
Copy link

tkelman commented Feb 15, 2014

Sparse matrix to elementwise power of 0 is incorrect, ref https://github.com/JuliaLang/julia/blob/c93ed16a093e33fe9e0de153120c821959641bd2/base/sparse/sparsematrix.jl#L579

In Julia:

julia> sparse(zeros(2,2)).^0
2x2 sparse matrix with 0 Float64 nonzeros:

In Matlab:

>> sparse(zeros(2,2)).^0
ans =
   (1,1)        1
   (2,1)        1
   (1,2)        1
   (2,2)        1

Separate comment, but it would be nice if I got a more descriptive error message (or no error at all) from:

julia> sparse([1 2],[1 2],[1 2])
MethodError(sparse,(
1x2 Array{Int64,2}:
 1  2,

1x2 Array{Int64,2}:
 1  2,

1x2 Array{Int64,2}:
 1  2))

Putting commas or semicolons in fixes things, but that's not particularly obvious to a new user.

@andreasnoack
Copy link
Member

Would't it be better to throw an error here instead of an explosion in memory allocation?

Regarding your second comment, people (myself included) coming from MATLAB will be annoyed by this for a period until they get used julia's vectors. This is explained under Noteworthy Differences, and I don't think it is feasible to include in error messages for all functions accepting vector arguments.

@tkelman
Copy link
Author

tkelman commented Feb 16, 2014

Would't it be better to throw an error here instead of an explosion in memory allocation?

It would be a good idea to explicitly check for a zero exponent for sparse.^scalar, maybe others too. sparse.^sparse is also currently mathematically wrong here, speye(2).^speye(2) should be a matrix of all ones, not diagonal as Julia currently outputs. There might be some use case where you care about the elementwise powers only within the same nonzero structure as the base matrix, but I can't think of one. sparse.^0 and sparse.^sparse are the kind of not-so-useful operations that you probably want to avoid, like sparse .+ scalar, but erroring instead of giving an expensive mathematically correct answer may be a bit much.

I see there's a general issue JuliaLang/julia#4744 about improving error messages. If multiple dispatch fails to find an appropriate method, it could be useful to optionally print a few "Did you mean?" almost-matches in signature. For sparse in particular, it may be reasonable to allow 2-or-higher-dimensional array inputs for specifying indices and values, converted to Vector via [:].

It's a bit surprising that using just semicolons does the same thing as using all commas in an array concatenation, vs Matlab where comma and space are equivalent in this context. Commas seem redundant for this purpose in Julia if they do the same thing as semicolons, except that they aren't allowed for the horizontal-and-vertical case.

@andreasnoack
Copy link
Member

It would be a good idea to explicitly check for a zero exponent for sparse.^scalar, maybe others too.

Is there a situation where sparse.^sparse would be useful? To me it doesn't seem like a useful function to support.

I see there's a general issue JuliaLang/julia#4744 about improving error messages. If multiple dispatch fails to find an appropriate method, it could be useful to optionally print a few "Did you mean?" almost-matches in signature.

I am not up to date with the plans for improvements on the error messages.

For sparse in particular, it may be reasonable to allow 2-or-higher-dimensional array inputs for specifying indices and values, converted to Vector via [:]

I am not sure I understand your proposal, but if it is to allow, a matrix input and the flatten that to a vector and call the vector version, then I think it is better only to have the vector version and let the user do the flattening. It will only cause confusion for MATLAB programmers in a short period.

Commas seem redundant for this purpose in Julia if they do the same thing as semicolons

I have had the same thought and I think it would make more sense if [1,1] was a Vector and [1;1] was Matrix, but I am sure the arguments for the present solution are somewhere either in the docs or the mailing list.

@tkelman
Copy link
Author

tkelman commented Feb 17, 2014

Is there a situation where sparse.^sparse would be useful? To me it doesn't seem like a useful function to support.

Probably not? .^ could easily be removed from the for loop here https://github.com/JuliaLang/julia/blob/c93ed16a093e33fe9e0de153120c821959641bd2/base/sparse/sparsematrix.jl#L449 since the assumptions baked into that implementation aren't valid for that operation on a pair of sparse matrices.

I am not up to date with the plans for improvements on the error messages.

It's partly a Julia Studio bug, the error messages in the REPL straight from julia-readline are significantly different and more useful than I was seeing in Studio. I'll try to come up with a simple example and report it on forio's tracker.

I am not sure I understand your proposal, but if it is to allow, a matrix input and the flatten that to a vector and call the vector version, then I think it is better only to have the vector version and let the user do the flattening. It will only cause confusion for MATLAB programmers in a short period.

You're right, flattening behind the API would allow more valid inputs but could be confusing in its own right. Perhaps another short sentence in the concatenation bullet, "Row matrices created by [x y z] are not valid inputs when a function expects a 1-D vector, unlike many comparable functions in Matlab," in case it's not obvious from the terse error messages and long lists of methods().

@jiahao
Copy link
Member

jiahao commented Feb 18, 2014

Thanks for catching this embarrassing error. I'll plan to remove the sparse.^sparse method and special case sparse.^0.

@tkelman
Copy link
Author

tkelman commented Feb 18, 2014

I was going to say this is good as long as none of your nonzero values are NaN, but quoting Wikipedia:

The 2008 version of the IEEE 754 standard says that pow(1,qNaN) and pow(qNaN,0) should both return 1 since they return 1 whatever else is used instead of quiet NaN.

it looks like Matlab's the non-conformant one w.r.t. NaN^0

@jiahao
Copy link
Member

jiahao commented Feb 18, 2014

Reasoning about the behaviour NaN can be surprisingly tricky, and quite a few of our numerics issues relate to standards-compliant NaN arithmetic.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants