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

Avoid materializing I, simplify _eyelike with CUDA and make it differentiable #1254

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DhairyaLGandhi
Copy link
Member

No description provided.

Copy link
Member

@ToucheSir ToucheSir left a comment

Choose a reason for hiding this comment

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

Do you have a link or other reference to what inspired this PR? The changes make sense to me, but given the only place _eyelike is called is not AD-safe in the first place, I'm confused as to the need for making only it differentiable.

@@ -272,7 +272,8 @@ end

end

@adjoint (::Type{T})(xs::Array) where {T <: CUDA.CuArray} =
_eyelike(y::CUDA.CuVector{T}) where T = CUDA.CuArray(I(length(y)))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_eyelike(y::CUDA.CuVector{T}) where T = CUDA.CuArray(I(length(y)))
_eyelike(y::CUDA.CuVector) = CUDA.CuArray(I(length(y)))

To avoid the unbound type param.

@@ -272,7 +272,8 @@ end

end

@adjoint (::Type{T})(xs::Array) where {T <: CUDA.CuArray} =
_eyelike(y::CUDA.CuVector{T}) where T = CUDA.CuArray(I(length(y)))
@adjoint (::Type{T})(xs::AbstractArray) where {T <: CUDA.CuArray} =
Copy link
Member

Choose a reason for hiding this comment

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

IIUC the intent of the PR, would we not want a dedicated path UniformScaling? Taking <:AbstractArray presumes we can convert back to that subtype, but the implementation can only convert back to Array at present. Adapt.jl may be able to help us here.

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.

2 participants