Skip to content

Conversation

@timholy
Copy link
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 bug Indicates an unexpected problem or unintended behavior backport pending 0.4 labels Dec 6, 2015
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 linear algebra Linear algebra label Dec 8, 2015
@tkelman tkelman deleted the teh/matmul_types branch December 8, 2015 18:16
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
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
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

bug Indicates an unexpected problem or unintended behavior linear algebra Linear algebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants