-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Make sparse operations less dependent on inference #19611
Conversation
(&)(A::SparseMatrixCSC, B::SparseMatrixCSC) = (_checksameshape(A, B); broadcast(&, A, B)) | ||
(|)(A::SparseMatrixCSC, B::SparseMatrixCSC) = (_checksameshape(A, B); broadcast(|, A, B)) | ||
xor(A::SparseMatrixCSC, B::SparseMatrixCSC) = (_checksameshape(A, B); broadcast(xor, A, B)) | ||
(+)(A::SparseMatrixCSC, B::SparseMatrixCSC) = broadcast(+, A, B) |
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.
why remove the size check?
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.
It is also performed within the broadcast
method.
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.
But shouldn't broadcast allow for 1-by-n .+
n-by-1 ?
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.
Shut! I missed these where binary arithmetic operations. I'll change it back.
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.
we should probably have a test that that throws an error with +
but works with .+
, if we don't already
8dab405
to
2a6a83d
Compare
I might have something potentially better in the works. Give me a few hours to see whether it works out... |
See #19623 for a potential alternative. The downside of converting to dense and back that I see is that something like |
Closing in favor of #19623 |
@@ -1828,3 +1828,8 @@ let | |||
@test_throws DimensionMismatch broadcast(+, A, B, speye(N)) | |||
@test_throws DimensionMismatch broadcast!(+, X, A, B, speye(N)) | |||
end | |||
|
|||
let A = sparse(Real[1 1]) | |||
@test (A + A)::SparseMatrixCSC{Int,Int} == [2 2] #19595 |
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.
the test will be good to port over
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.
Would you say we should verify the return type to have eltype Int
? Presently
julia> Real[1 1] + Real[1 1]
1×2 Array{Real,2}:
2 2
so maybe Real
would be an acceptable return type, too?
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.
Personally, I'd prefer the dense case to return an Array{Int,2}
, but we'd need a revision of the behavior of arithmetic operation on dense arrays. So, checking for eltype(A + A) === Int
here seems better to me.
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.
See #19669.
This should address #19595 and also fix the examples in #19561, although it might still be worth to have something like #19589.
This falls back, when the return type cannot be inferred, to treating the
SparseMatrixCSC
s asArray
s and then callssparse
over the result. Granted, this is not the most efficient fallback, but since it applies to type-unstable functions, it shouldn't be much of a problem. We could eventually improve the performance if it seems necessary.