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

gpu fix #12

Merged
merged 5 commits into from
Jan 5, 2020
Merged

gpu fix #12

merged 5 commits into from
Jan 5, 2020

Conversation

vitskvara
Copy link
Collaborator

Trying to solve #11.

@@ -65,7 +65,7 @@ mean_var(p::CMeanGaussian, z::AbstractArray) = (mean(p, z), variance(p, z))
function Flux.functor(p::CMeanGaussian{V,S,M}) where {V,S,M}
fs = fieldnames(typeof(p))
nt = (; (name=>getfield(p, name) for name in fs)...)
nt, y -> CMeanGaussian{V,S,M}(y...)
nt, y -> CMeanGaussian{V,S,typeof(gpu(p.mapping))}(y...)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This only fixes the mapping conversion, however the σ field is still not converted.

Copy link
Collaborator Author

@vitskvara vitskvara Dec 18, 2019

Choose a reason for hiding this comment

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

Also, this is probably going to break if one has CuArrays loaded, the model is on cpu and functor is not called in the gpu conversion process - what are the places where functor is called?

@codecov-io
Copy link

codecov-io commented Dec 18, 2019

Codecov Report

Merging #12 into master will decrease coverage by 0.87%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
- Coverage    92.3%   91.42%   -0.88%     
==========================================
  Files           8        8              
  Lines         104      105       +1     
==========================================
  Hits           96       96              
- Misses          8        9       +1
Impacted Files Coverage Δ
src/cmeanvar_gaussian.jl 100% <100%> (ø) ⬆️
src/cmean_gaussian.jl 83.33% <33.33%> (-3.63%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a674b1...749600b. Read the comment docs.

@nmheim
Copy link
Member

nmheim commented Dec 18, 2019

functor is used by e.g. trainable and gpu. it recursively tries to adapt all Arrays to CuArrays. As far as I can see it constructs the type signature (with the cpu arrays in the mapping) before moving the mapping to the gpu... I am not sure if this is easily solvable with the type signature we have right now...

@nmheim
Copy link
Member

nmheim commented Dec 18, 2019

I cannot test on GPU atm, but does this work?

function Flux.functor(p::CMeanGaussian{V}) where V
    fs = fieldnames(typeof(p))
    nt = (; (name=>getfield(p, name) for name in fs)...)
    nt, y -> CMeanGaussian{V}(y...)
end

@nmheim
Copy link
Member

nmheim commented Dec 19, 2019

putting a gpu call in functor is not a good idea, because gpu itsself calls functor (through fmap and fmap1). have you tried if the snippet I posted above works on a GPU?

@vitskvara
Copy link
Collaborator Author

It helps with CMeanVarGaussian, but not with CMeanGaussian.

@vitskvara
Copy link
Collaborator Author

Now it fails with

julia> p  = CMeanGaussian{DiagVar}(mapping, var) |> gpu
ERROR: MethodError: no method matching CMeanGaussian{DiagVar,M,S} where S<:AbstractArray where M(::Dense{typeof(identity),CuArray{Float32,2,Nothing},CuArray{Float32,1,Nothing}}, ::CuArray{Float32,1,Nothing}, ::Int64, ::Dict{Symbol,Bool})
Closest candidates are:
  CMeanGaussian{DiagVar,M,S} where S<:AbstractArray where M(::M, ::Any, ::Int64) where {V, M} at /home/vit/.julia/dev/ConditionalDists/src/cmean_gaussian.jl:40
  CMeanGaussian{DiagVar,M,S} where S<:AbstractArray where M(::Any, ::Any) at /home/vit/.julia/dev/ConditionalDists/src/cmean_gaussian.jl:46
Stacktrace:
 [1] #14 at /home/vit/.julia/dev/ConditionalDists/src/cmean_gaussian.jl:68 [inlined]
 [2] fmap1(::Function, ::CMeanGaussian{DiagVar,Dense{typeof(identity),Array{Float32,2},Array{Float32,1}},Array{Float32,1}}) at /home/vit/.julia/packages/Flux/oObnA/src/functor.jl:32
 [3] #fmap#41(::IdDict{Any,Any}, ::typeof(fmap), ::typeof(cu), ::CMeanGaussian{DiagVar,Dense{typeof(identity),Array{Float32,2},Array{Float32,1}},Array{Float32,1}}) at /home/vit/.julia/packages/Flux/oObnA/src/functor.jl:37
 [4] fmap at /home/vit/.julia/packages/Flux/oObnA/src/functor.jl:36 [inlined]

@vitskvara
Copy link
Collaborator Author

vitskvara commented Dec 20, 2019

I am really confused with this. When debugging, it seems that the overloaded functor is called twice before some default one is called which tries to construct the Dense layer but with the original CMeanGaussian arguments, which of course does not make any sense.

@nmheim
Copy link
Member

nmheim commented Jan 5, 2020

gpu tried to call CMeanGaussian{V}(m,s,xlength,d) at some point, but only CMeanGaussian{V,M,S}(m,s,xlength,d) (with the types M and S) was defined.

@nmheim nmheim merged commit d7ca4fb into master Jan 5, 2020
@nmheim nmheim deleted the gpu-fix branch January 5, 2020 14:28
@nmheim nmheim mentioned this pull request Jan 9, 2020
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.42%. Comparing base (4a674b1) to head (749600b).
Report is 46 commits behind head on master.

Files with missing lines Patch % Lines
src/cmean_gaussian.jl 33.33% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
- Coverage   92.30%   91.42%   -0.88%     
==========================================
  Files           8        8              
  Lines         104      105       +1     
==========================================
  Hits           96       96              
- Misses          8        9       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it 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.

4 participants