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

Fix OneHotVector/Matrix performance on GPU #591

Merged
merged 5 commits into from Feb 4, 2019

Conversation

DhairyaLGandhi
Copy link
Member

This PR aims to fix performance of OneHotMatrix performance on the GPU. It additionally adds some missing functionality to their behaviour.

@KristofferC
Copy link
Contributor

Does this help with #582?

@DhairyaLGandhi
Copy link
Member Author

For that, we want to ensure onecold returns a CuArray. I'll add a commit to make it so.

@DhairyaLGandhi
Copy link
Member Author

This should help with #582

@MikeInnes
Copy link
Member

LGTM, but are there any tests we can add for this? e.g. returns CuArray on the GPU.

@testset "onecold gpu" begin
x = rand(Float32, 10, 3) |> gpu;
y = Flux.onehotbatch(1:3, 1:10) |> gpu;
@test_nowarn Flux.onecold(x) .== Flux.onecold(y)
Copy link
Contributor

Choose a reason for hiding this comment

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

@test_nowarn doesn't work anymore.

julia> using Test

julia> @test_nowarn @warn "foo"
┌ Warning: foo
└ @ Main REPL[5]:1

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Perhaps?

@testset "onecold gpu" begin
     x = zeros(Float32, 10, 3) |> gpu;
     y = Flux.onehotbatch(ones(3), 1:10) |> gpu;
     res = Flux.onecold(x) .== Flux.onecold(y)
     @test res isa CuArray
end

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems to indeed better test what we want to check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks bunches!

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.

None yet

3 participants