Skip to content

Change type annotation and similar to zero #30

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 5 commits into from
Aug 11, 2022

Conversation

frankschae
Copy link
Member

This seems to solve #29 of ReverseDiffVJP().

@frankschae
Copy link
Member Author

ReverseDiff's similar is in
https://github.com/JuliaDiff/ReverseDiff.jl/blob/4a50321ab03eb59271d83863d420bdc73ac0f869/src/tracked.jl#L386
Can that be further overloaded to solve this?

julia> s
(2, 2)

julia> u
2×2 ReverseDiff.TrackedArray{Float64, Float64, 2, Matrix{Float64}, Matrix{Float64}}:
 TrackedReal<73Q>(4.275532424545035, 0.0, LZl, 1, Les)  TrackedReal<27L>(1.4210350814697517, 0.0, LZl, 3, Les)
 TrackedReal<Gra>(3.986400929377556, 0.0, LZl, 2, Les)  TrackedReal<Af6>(2.732510481205003, 0.0, LZl, 4, Les)

julia> similar(u, s)
2×2 Matrix{ReverseDiff.TrackedReal{Float64, Float64, ReverseDiff.TrackedArray{Float64, Float64, 2, Matrix{Float64}, Matrix{Float64}}}}:
 #undef  #undef
 #undef  #undef
 
julia> zero(u)
2×2 ReverseDiff.TrackedArray{Float64, Float64, 2, Matrix{Float64}, Matrix{Float64}}:
 TrackedReal<8lT>(0.0, 0.0, C0z, 1, I6e)  TrackedReal<ICq>(0.0, 0.0, C0z, 3, I6e)
 TrackedReal<2o6>(0.0, 0.0, C0z, 2, I6e)  TrackedReal<1jB>(0.0, 0.0, C0z, 4, I6e)

How about?

function Base.getindex(b::LazyBufferCache, u::T) where {T <: AbstractArray}
    s = b.sizemap(size(u)) # required buffer size
    buf = get!(b.bufs, (T, s)) do
        similar(u, s) # buffer to allocate if it was not found in b.bufs
    end::T  # declare type since b.bufs dictionary is untyped
    return buf
end

function Base.getindex(b::LazyBufferCache, u::ReverseDiff.TrackedArray)
    s = b.sizemap(size(u)) # required buffer size
    buf = get!(b.bufs, (T, s)) do
        # declare type since b.bufs dictionary is untyped
        zero(u)::T # buffer to allocate if it was not found in b.bufs
    end
    return buf
end

@ChrisRackauckas
Copy link
Member

I think so. @mohamed82008 do you think overloading that similar to return a TrackedArray would be breaking? Seems reasonable enough to me.

@mohamed82008
Copy link

mohamed82008 commented Aug 7, 2022

It might be breaking since code that worked before might not work. Especially similar is usually used for in-place operations so a tracked array is useless here. Also there is JuliaDiff/ReverseDiff.jl#66.

@frankschae
Copy link
Member Author

I couldn't make the TrackedArray version work yet. From a quick chat with @mohamed82008 I guess the additional ReverseDiff dispatch is the best alternative?

Defining a similar_type function, like https://github.com/JuliaArrays/StaticArrays.jl/blob/0feac146bb71ff48ca0a8465739f6b442395292c/src/abstractarray.jl#L59, overloading it in all downstream packages of ReverseDiff and using that here seems more error-prone (+ actually it sounds like that could not be used to differentiate in-place functions in first place)?

@ChrisRackauckas
Copy link
Member

It looks like this broke CUDA support?

@frankschae
Copy link
Member Author

strange -- I don't see how it can break it. Should I try a rebase?

@ChrisRackauckas
Copy link
Member

LoadError: ArgumentError: cannot take the CPU address of a CUDA.CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}
--
  | Stacktrace:
  | [1] unsafe_convert(#unused#::Type{Ptr{Float32}}, x::CUDA.CuArray{Float32, 2, CUDA.Mem.DeviceBuffer})
  | @ CUDA ~/.cache/julia-buildkite-plugin/depots/76057d1d-9065-4525-8f99-c59b2e38dd89/packages/CUDA/DfvRa/src/array.jl:319
  | [2] getrf!(A::CUDA.CuArray{Float32, 2, CUDA.Mem.DeviceBuffer})
  | @ LinearAlgebra.LAPACK ~/.cache/julia-buildkite-plugin/julia_installs/bin/linux/x64/1.7/julia-1.7-latest-linux-x86_64/share/julia/stdlib/v1.7/LinearAlgebra/src/lapack.jl:575
  | [3] lu!(A::CUDA.CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}, ::LinearAlgebra.RowMaximum; check::Bool)
  | @ LinearAlgebra ~/.cache/julia-buildkite-plugin/julia_installs/bin/linux/x64/1.7/julia-1.7-latest-linux-x86_64/share/julia/stdlib/v1.7/LinearAlgebra/src/lu.jl:81
  | [4] lu(A::CUDA.CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}, pivot::LinearAlgebra.RowMaximum; check::Bool)
  | @ LinearAlgebra ~/.cache/julia-buildkite-plugin/julia_installs/bin/linux/x64/1.7/julia-1.7-latest-linux-x86_64/share/julia/stdlib/v1.7/LinearAlgebra/src/lu.jl:279
  | [5] lu(A::CUDA.CuArray{Float32, 2, CUDA.Mem.DeviceBuffer}, pivot::LinearAlgebra.RowMaximum) (repeats 2 times)
  | @ LinearAlgebra ~/.cache/julia-buildkite-plugin/julia_installs/bin/linux/x64/1.7/julia-1.7-latest-linux-x86_64/share/julia/stdlib/v1.7/LinearAlgebra/src/lu.jl:278
  | [6] lu_instance(A::CUDA.CuArray{Float32, 2, CUDA.Mem.DeviceBuffer})
  | @ ArrayInterfaceGPUArrays ~/.cache/julia-buildkite-plugin/depots/76057d1d-9065-4525-8f99-c59b2e38dd89/packages/ArrayInterfaceGPUArrays/lwGfo/src/ArrayInterfaceGPUArrays.jl:23

https://github.com/JuliaGPU/CUDA.jl/blob/master/lib/cusolver/linalg.jl#L314-L316

@maleadt how come LU factorization is only for v1.8 and above?

@ChrisRackauckas ChrisRackauckas merged commit ca505eb into SciML:master Aug 11, 2022
@ChrisRackauckas
Copy link
Member

The last time this CI ran the default was QR factorization for GPU. It changed to LU factorization for GPU. This failed because of the version bound. But that's unrelated to this repo so I'll merge.

@maleadt
Copy link

maleadt commented Aug 11, 2022

@maleadt how come LU factorization is only for v1.8 and above?

I only implemented it after we got the generalized factorizations in 1.8, and didn't bother porting it to older versions (which is a hassle, because it requires CUDA-specific factorization objects, i.e. CuLU, etc).

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.

4 participants