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

matmul: don't assume the existence of type-conversions #14293

Merged
merged 1 commit into from
Dec 8, 2015

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Dec 6, 2015

I've been playing around with number types whose product is a real number but for which I cannot define a conversion to Reals. This fixes a bug in our algorithm for matrix multiplication.

julia> immutable RootInt
           i::Int
       end

julia> import Base: *

julia> (*)(x::RootInt, y::RootInt) = x.i*y.i
* (generic function with 143 methods)

julia> a = [RootInt(3)]
1-element Array{RootInt,1}:
 RootInt(3)

julia> C = [0]
1-element Array{Int64,1}:
 0

julia> A_mul_Bt!(C, a, a)
ERROR: MethodError: `convert` has no method matching convert(::Type{Int64}, ::RootInt)
This may have arisen from a call to the constructor Int64(...),
since type constructors fall back to convert methods.
Closest candidates are:
  call{T}(::Type{T}, ::Any)
  convert(::Type{Int64}, ::Int8)
  convert(::Type{Int64}, ::UInt8)
  ...
 in copy_transpose! at abstractarray.jl:383
 in copy_transpose! at linalg/matmul.jl:355
 in generic_matmatmul! at linalg/matmul.jl:465
 in A_mul_Bt! at linalg/matmul.jl:168
 in eval at ./boot.jl:263

@timholy timholy added kind:bug Indicates an unexpected problem or unintended behavior backport pending 0.4 labels Dec 6, 2015
(*)(x::RootInt, y::RootInt) = x.i*y.i
promote_op(::Base.MulFun, ::Type{RootInt}, ::Type{RootInt}) = Int

a = [RootInt(3)]
Copy link
Contributor

Choose a reason for hiding this comment

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

put these in a let or give them issue-number-specific names

nevermind guess it isn't necessary here, the same file is already using these names above

jakebolewski added a commit that referenced this pull request Dec 8, 2015
matmul: don't assume the existence of type-conversions
@jakebolewski jakebolewski merged commit f5f3b57 into master Dec 8, 2015
@kshyatt kshyatt added the domain:linear algebra Linear algebra label Dec 8, 2015
@tkelman tkelman deleted the teh/matmul_types branch December 8, 2015 18:16
A_mul_Bt!(C, a, a)
@test C[1] == 9
a = [RootInt(2),RootInt(10)]
@test a*a' == [4 20; 20 100]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how this passed without defining zero(::Type{RootInt}) ?

oh right this also depends on #13803

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

If you want to backport this, we could add that method (for use by the test).

Copy link
Contributor

Choose a reason for hiding this comment

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

without the promote_op changes I don't think this will work?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:linear algebra Linear algebra kind:bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants