Skip to content

Adds dispatch rules for column vector times transpose(row vector) #555

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

Merged
merged 1 commit into from
Nov 30, 2018

Conversation

zygmuntszpak
Copy link
Contributor

Fixes issue #537

@coveralls
Copy link

coveralls commented Nov 29, 2018

Coverage Status

Coverage increased (+0.06%) to 66.231% when pulling 1ebd02b on zygmuntszpak:dispatch_rules into 53a19c3 on JuliaArrays:master.

Copy link
Member

@andyferris andyferris left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution - we should probably make sure Transpose is also covered as well. :)

@@ -10,16 +10,18 @@ import LinearAlgebra: BlasFloat, matprod, mul!
@inline *(A::StaticVector, B::StaticMatrix) = *(reshape(A, Size(Size(A)[1], 1)), B)
@inline *(A::StaticVector, B::Transpose{<:Any, <:StaticVector}) = _mul(Size(A), Size(B), A, B)
@inline *(A::StaticVector, B::Adjoint{<:Any, <:StaticVector}) = _mul(Size(A), Size(B), A, B)
@inline *(A::StaticMatrix, B::Adjoint{<:Any, <:StaticVector}) = *(reshape(A, Size(Size(A)[1],)), B)
Copy link
Member

Choose a reason for hiding this comment

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

  • Wouldn't _mul(Size(A), Size(B), A, B) work?
  • Need the same thing for Transpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I change that line to _mul(Size(A), Size(B), A, B) I get

ERROR: MethodError: no method matching _mul(::Size{(4, 1)}, ::Size{(1, 4)}, ::SArray{Tuple{4,1},Float64,2,4}, ::LinearAlgebra.Adjoint{Float64,SArray{Tuple{4},Float64,1,4}})
Closest candidates are:
  _mul(::Size{sa}, ::Size{sb}, ::StaticArray{Tuple{#s175,#s174},Ta,2} where #s174 where #s175, ::StaticArray{Tuple{#s173},Tb,1} where #s173) where {sa, sb, Ta, Tb} at /home/zygmunt/.julia/dev/StaticArrays/src/matrix_multiply.jl:45
  _mul(::Size{sa}, ::Size{sb}, ::StaticArray{Tuple{#s173},Ta,1} where #s173, ::Union{Adjoint{Tb,#s171} where #s171<:(StaticArray{Tuple{N},T,1} where T where N), Transpose{Tb,#s172} where #s172<:(StaticArray{Tuple{N},T,1} where T where N)}) where {sa, sb, Ta, Tb} at /home/zygmunt/.julia/dev/StaticArrays/src/matrix_multiply.jl:65
  _mul(::Size{sa}, ::Size{sb}, ::StaticArray{Tuple{#s175,#s174},Ta,2} where #s174 where #s175, ::StaticArray{Tuple{#s173,#s172},Tb,2} where #s172 where #s173) where {sa, sb, Ta, Tb} at /home/zygmunt/.julia/dev/StaticArrays/src/matrix_multiply.jl:77
  ...
Stacktrace:
 [1] *(::SArray{Tuple{4,1},Float64,2,4}, ::LinearAlgebra.Adjoint{Float64,SArray{Tuple{4},Float64,1,4}}) at /home/zygmunt/.julia/dev/StaticArrays/src/matrix_multiply.jl:13
 [2] top-level scope at none:0

Copy link
Member

@Evizero Evizero Nov 29, 2018

Choose a reason for hiding this comment

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

what happens if size(A) is (4,2), does reshape fail then? is a DimensionMismatch thrown? I wonder if it wouldn't be cleaner to dispatch on the fact the dimensions 2 of A is 1 (similar to the example here #537 (comment) ). In that case a matrix A thats not a column vector would still go to the fallback method which would ultimately throw a DimensionMismatch.

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 does throw a DimensionMismatch but your solution is definitely neater. I have revised the pull-request as per your suggestion and have included a dispatch rule for transpose as well.

@Evizero
Copy link
Member

Evizero commented Nov 30, 2018

I don't know if it is covered by other tests already, but it would be nice to test explicitly if a DimensionMismatch is thrown in case the matrix is not a column vector

@zygmuntszpak
Copy link
Contributor Author

I don't know if it is covered by other tests already, but it would be nice to test explicitly if a DimensionMismatch is thrown in case the matrix is not a column vector

Done.

@codecov-io
Copy link

Codecov Report

Merging #555 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
+ Coverage   66.17%   66.23%   +0.05%     
==========================================
  Files          38       38              
  Lines        2903     2908       +5     
==========================================
+ Hits         1921     1926       +5     
  Misses        982      982
Impacted Files Coverage Δ
src/matrix_multiply.jl 48.73% <100%> (+0.52%) ⬆️
src/SVector.jl 95.83% <0%> (+0.08%) ⬆️
src/MVector.jl 95.55% <0%> (+0.1%) ⬆️
src/SMatrix.jl 82.35% <0%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53a19c3...1ebd02b. Read the comment docs.

@andyferris andyferris merged commit 03b2ce8 into JuliaArrays:master Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants