adding transpose!() to linear algebra #808
Conversation
|
|
||
| @testset "transpose!" begin | ||
| v = zeros(5) | ||
| M = rand(1, 5) |
There was a problem hiding this comment.
Would you mind not using rand? we had problems tracking numerical stability and bugs due to rand.
There was a problem hiding this comment.
No problem. I will make a change to that. Also, the rest of the test set uses rand. Should I also change them?
|
|
||
| function LinearAlgebra.transpose!(B::AnyTracedRMatrix, A::AnyTracedRMatrix) | ||
| LinearAlgebra.check_transpose_axes(size(B), size(A)) | ||
| set_mlir_data!(B, get_mlir_data(Ops.transpose(A, [2,1]))) |
There was a problem hiding this comment.
We should do a promote and set here. Base implements it like B[j,i] = f(A[i,j]) so its auto promoted
There was a problem hiding this comment.
noted. I'll go fix this.
There was a problem hiding this comment.
i know we do promote here but is it the correct thing? e.g. in transpose!(::TracedRArray{Int}, ::TracedRArray{Float64}), promotion of eltype will be Float64 and the result we are storing will be wrong.
or should the promotion handle it and error in that case?
There was a problem hiding this comment.
I think let's update Ops.convert to throw errors in cases where Julia would also throw an error.
Then we just do Ops.convert(..., Ops.transpose(A, [2,1]))
There was a problem hiding this comment.
I'm wrestling with the Ops.convert() for a while now. It looks like the stablehlo spec is that it performs unsafe truncation by default while Julia does not. And I could not find any Julia function that takes T1, T2 and be able to tell whether type conversion will be safe.
Julia seems to determine if the conversion is OK (like throwing InexactError or not) during run-time by explicitly converting the value to type T (i.e. 1.0 -> 1 is ok, for example).
In the case of TracedRNumber, maybe we can just do T(x) directly and let Julia throw error - if any (not sure yet how to go from mlir_dat -> Julia primitives).
In case of TracedRArray, I don't really know. Is there anything or some obvious solutions I am missing?
There was a problem hiding this comment.
Ah I see what's going on. Unfortunately stablehlo doesn't seem to have conversion mode supported (openxla/stablehlo#180). We can do the convert here and just defer to stablehlo to handle the truncation.
cc @wsmoses any thoughts here?
| @@ -397,4 +397,21 @@ function LinearAlgebra._kron!(C::AnyTracedRMatrix, A::AnyTracedRMatrix, B::AnyTr | |||
| return C | |||
| end | |||
|
|
|||
| LinearAlgebra.transpose(a::AnyTracedRArray) = error("transpose not defined for $(typeof(a)).") | |||
There was a problem hiding this comment.
is there a reason to add this? Shouldn't it fall back to the inplace versions?
There was a problem hiding this comment.
The stdlib LinearAlgebra.jl has this:
transpose(a::AbstractArray) = error("transpose not defined for $(typeof(a)). Consider using `permutedims` for higher-dimensional arrays.")
The transpose!() docstring also mentions about not supporting the in-place version.
"""
transpose!(dest,src)
Transpose array src and store the result in the preallocated array dest, which should
have a size corresponding to (size(src,2),size(src,1)). No in-place transposition is
supported and unexpected results will happen if src and dest have overlapping memory
regions.
"""
So yeah, I was just trying to mimic it exactly.
There was a problem hiding this comment.
i guess that then you can leave it unimplemented and it will call the default method which will error like is supposed to
Hello, I'm an applicant to GSoC this summer. Avik told me that LinearAlgebra.jl is good for getting up to speed.
I started with what I assume is the most simple function.
I hope I took the right approach by re-using the functions from Ops.jl
Please advise if I make any mistakes or anything I can do better.