-
Notifications
You must be signed in to change notification settings - Fork 40
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
Support matrix multiplication (Continue #93) #146
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #146 +/- ##
==========================================
+ Coverage 93.60% 94.23% +0.63%
==========================================
Files 2 2
Lines 250 260 +10
==========================================
+ Hits 234 245 +11
+ Misses 16 15 -1
Continue to review full report at Codecov.
|
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 additional overhead comes from #144, I think we may need a way to opt-out the overflow check.
Solving these might be possible only with an explicit dependence on LinearAlgebra. I guess that's ok?
This looks okay to me.
- New methods for * for combinations of OffsetArray and AbstractArray Unfortunately this introduces method ambiguities with Base. - As an alternative, tried defining a new promote_rule, but somehow it doesn't get invoked. New convenience constructors - Wrap any (non-offset) array as an OffsetArray with standard indices - Remove layer of indirection when constructing with explicit offsets Cosmetic edits to constructor section for improved readability
I'm starting to think that we should not solve the matrix multiplications involving In any case I'm not too sure how to approach arbitrarily nested |
I have the same feeling that such kind of support (including the ones you've added) should be done upstream: Base, LinearAlgebra or so. Supporting these operations in OffsetArrays is like Whack-a-mole. Would like to get @mcabbott involved for some advice |
Yes, would be nice if this worked. My impression is that |
I think what's missing here is a protocol in LinearAlgebra to generically deal with array with offsets. It currently only disabled such ability. The one I just made is: # LinearAlgebra
offsetted(A::AbstractArray, offsets) = A
unoffsetted(A::AbstractArray) = A, ntuple(_->0, ndims(A))
# OffsetArrays
# this isn't right because it override the function definition
offsetted(A::AbstractArray{T, N}, offsets) where {T, N} = OffsetArray{T, N, typeof(A)}(A, offsets)
unoffsetted(A::OffsetArray) = A.parent, A.offsets The name is perhaps not good, but you get the idea. For a simple experiment for 2*2 matrix: # https://github.com/JuliaLang/julia/blob/8bdf5695e25e77d53f8934d9f135858d174e30d1/stdlib/LinearAlgebra/src/matmul.jl#L914-L950
+ offsetted(A::AbstractArray, offsets) = A
+ unoffsetted(A::AbstractArray) = A, ntuple(_->0, ndims(A))
+
function matmul2x2!(C::AbstractMatrix, tA, tB, A::AbstractMatrix, B::AbstractMatrix,
_add::MulAddMul = MulAddMul())
+ A, Ao = unoffsetted(A)
+ B, Bo = unoffsetted(B)
+
+ if !(Ao[2] == Bo[1])
+ throw(ArgumentError("offset doesn't match"))
+ end
- require_one_based_indexing(C, A, B)
...
- C
+ offsetted(C, (Ao[1], Bo[2]))
end
# OffsetArrays
+ import LinearAlgebra
+ LinearAlgebra.offsetted(A::AbstractArray{T, N}, offsets) where {T, N} = OffsetArray{T, N, typeof(A)}(A, offsets)
+ LinearAlgebra.unoffsetted(A::OffsetArray) = A.parent, A.offsets and it seems work pretty well: julia> ao = ones(0:1, 0:1)
2×2 OffsetArray(::Matrix{Float64}, 0:1, 0:1) with eltype Float64 with indices 0:1×0:1:
1.0 1.0
1.0 1.0
julia> ao * ao
2×2 OffsetArray(::Matrix{Float64}, 0:1, 0:1) with eltype Float64 with indices 0:1×0:1:
2.0 2.0
2.0 2.0 I'm not very sure if this goes to the BLAS call for large size cases. Performance might be an issue. This is just some simple experiment, I haven't yet checked it. Any ideas on how to properly dispatch the |
Can't this be simpler though? And At least for ordinary |
A lot of codes in LinearAlgebra preassume it being base-1 indexes. Only stripped the offset off might not be sufficient and requires a lot of more effort to make things correct. For example, # https://github.com/JuliaLang/julia/blob/8bdf5695e25e77d53f8934d9f135858d174e30d1/stdlib/LinearAlgebra/src/matmul.jl#L927-L928
A11 = copy(A[1,1]'); A12 = copy(A[2,1]')
A21 = copy(A[1,2]'); A22 = copy(A[2,2]') |
I really don't think this should be fixed here. It just needs someone to go through LinearAlgebra and start generalizing the 1-based assumptions. I'm not saying that's a small job (there's a reason it hasn't been done yet), but it's the only reasonable path to get to where we want to be. |
Here's a sketch of what I was trying to say above: https://gist.github.com/mcabbott/03225572217660aa7694f46caaa6e15f There aren't so many calls to Is there a flaw in this plan? |
That's not bad; it's certain more generic than having each package that uses offset axes implement the same linear algebra functions over and over again. But fundamentally I don't see a need to "strip" unconventional axes; indeed, some day (perhaps when To me it seems the sensible thing to do is start going through the multiplication routines and generalize them to use |
Thanks for having a look. I guess this is an attempt to change as little as possible. It wouldn't preclude doing a better The contract for using AxisArrays # a wrapper with no remove_offset_indexing method
aa = AxisArray(rand(2,2), x='a':'b', y=10:11)
@which aa' * aa # AbstractArray, no special methods, no special axes
ao = AxisArray(zeros(0:1, 3:4), x='a':'b', y=10:11) # with indices 0:1×3:4
ao' * ao # ArgumentError: offset arrays are not supported It's a bit awkward that there's no simple way here for the method for If |
Since |
I guess I was thinking of the pointer as being the maximally unwrapped form, just the memory (& the eltype). Before you pass that along, you'd need some logic to be sure they are all in the same convention. I suppose that is why |
Well, mostly I think there hasn't been any reason to have it so far...because matrix multiplication hasn't been supported. But again, "maximally unwrapped" is a property of our current implementations, not a generic property. struct ZeroIndexedSVector{L,T} <: AbstractVector{T}
data::RawBuffer{UInt8} # RawBuffer needs to be implemented in Julia (gc management) but is coming some day...
end
Base.axes(::ZeroIndexedSVector{L}) where L = (ZeroRange(L),) # https://github.com/JuliaArrays/CustomUnitRanges.jl/blob/master/src/ZeroRange.jl does not have a wrapper you can strip---instead you have to wrap it with something else. |
OK, good example. It's a bit ugly if The other objection to this design is that, with my |
|
Yes to strides-of-the-parent, and JuliaLang/julia#30432 seems like a very neat mechanism to let this pass information about non-stridedness too. Not sure I'm volunteering to re-write |
What's the current state of this? It would be really nice if matrix multiply just worked with offset |
I'm pretty firmly of the opinion that most of the work needs to be done in LinearAlgebra and/or Base, not here. I know folks probably think the magic of OffsetArrays mostly happens here, but that's not really true (plus about a gazillion bugfix PRs submitted by lots of other people after that). |
Oh yes, I definitely agree with you! This PR in its current form looks like a worthwhile improvement, at least to me, though, but I might have missed something in the discussions. Or does this cause method ambiguities, even without trying to support |
If it doesn't break anything now, and its later removal doesn't break anything later, then we could use this as a bandaid. It's a bandaid that may slow progress on the right solution, though. So it's worth asking whether this PR is in our best interests, long-term. I won't stand in the way if folks want to merge this. |
Adding such a bandaid can easily cause unexpected performance regression for some blas operations without enough specifications. The fallback blas implementations are usually quite slow and identifying the performance regression isn't usually a trivial task; I'd rather avoid this whack-a-mole patch. |
This PR continues #93, along the lines of the strategy 2 mentioned here, ie. the axes of the two terms have to be compatible. Now these are possible:
Still WIP, doesn't play well with
Adjoint
andTranspose
yet.To-do:
oa' * ov
oa' * oa
Solving these might be possible only with an explicit dependence on LinearAlgebra. I guess that's ok?
This is actually somewhat less ambitious than the original PR, as product with arbitrary matrices is not handled here and defaults to
LinearAlgebra
.