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

onecold does not work on CuMatrix #864

Closed
AzamatB opened this issue Sep 19, 2019 · 3 comments
Closed

onecold does not work on CuMatrix #864

AzamatB opened this issue Sep 19, 2019 · 3 comments

Comments

@AzamatB
Copy link
Contributor

AzamatB commented Sep 19, 2019

julia> xs = rand(2,3)
2×3 Array{Float64,2}:
 0.0758553  0.779633    0.218497
 0.840066   0.00234058  0.402085

julia> Flux.onecold(xs, ["a","b"])
3-element Array{String,1}:
 "b"
 "a"
 "b"

julia> xs = cu(xs)
2×3 CuArray{Float32,2}:
 0.0758553  0.779633    0.218497
 0.840066   0.00234058  0.402085

julia> Flux.onecold(xs, ["a","b"])
ERROR: Type String does not have a definite size.
Stacktrace:
 [1] sizeof at ./essentials.jl:452 [inlined]
 [2] CuArray{String,1}(::UndefInitializer, ::Tuple{Int64}) at /home/azamat/.julia/packages/CuArrays/wXQp8/src/array.jl:38
 [3] similar at /home/azamat/.julia/packages/CuArrays/wXQp8/src/array.jl:69 [inlined]
 [4] similar(::CuArray{Float32,1}, ::Type{String}, ::Tuple{Base.OneTo{Int64}}) at ./abstractarray.jl:635
 [5] #mapslices#111(::Int64, ::typeof(mapslices), ::getfield(Flux, Symbol("##25#26")){Tuple{Array{String,1}}}, ::CuArray{Float32,2}) at ./abstractarray.jl:2009
 [6] #mapslices at ./none:0 [inlined]
 [7] onecold(::CuArray{Float32,2}, ::Array{String,1}) at /home/azamat/.julia/packages/Flux/dkJUV/src/onehot.jl:121
 [8] top-level scope at none:0
AzamatB added a commit to AzamatB/ListenAttendSpell.jl that referenced this issue Sep 19, 2019
@CarloLucibello
Copy link
Member

It does work if you replace ["a","b"] with numbers. Also it may be not too slow in some cases to perform the computation on the cpu (e.g. large network, i.e. slow forward pass, and move only last output to the cpu. This is what Knet does)

@dnabanita7
Copy link
Contributor

Since onecold only accepts numbers, Can I try fixing this?

@darsnack
Copy link
Member

I don't see how this works for strings:

onecold(y::OneHotMatrix, labels...) =
  mapreduce(x -> Flux.onecold(x, labels...), |, y.data, dims = 2, init = 0)

What does | do for strings? Why do we need to reduce at all? Isn't the following sufficient?

onecold(y::OneHotMatrix, labels...) = map(x -> Flux.onecold(x, labels...), y.data)

bors bot added a commit that referenced this issue Jan 8, 2021
1448: Arbitrary dimension one-hot arrays r=DhairyaLGandhi a=darsnack

This supersedes #1447. It should address the same issues:
- fix #1445, #1229
- probably fix also #864, #556, #189

This PR introduces a new one-hot N-dimensional array type, `OneHotArray`. Like #1447, this approach avoids the pointer allocations associated with `OneHotMatrix` being an array of `OneHotVector`s. It also lifts the "height" into the type parameter to avoid unnecessary allocation. Unlike #1447, this approach does not introduce a new primitive type. Instead, a "one-hot vector" is represented with a single subtype of `Integer` that is configurable by the user. By default, the exposed API will use `UInt32`.

Fundamentally, the primitive type is necessary because wrapping a `UInt32` as a `OneHotVector` will suffer memory penalties when you create an `Array{<:OneHotVector}`. But if we begin by designing for N-dimensions, then `OneHotVector` is just the specialized 1D case (similar to how `Vector{T} = Array{T, 1}`).

## Performance

I compared against the same tests mentioned in #1447. Please suggest more if you want to.

1. #189
```jl
#master
julia> x = Flux.onehotbatch(rand(1:100, 50), 1:100);

julia> W = rand(128, 100);

julia> @Btime $W * $x;
  5.095 μs (13 allocations: 50.86 KiB)

julia> cW, cx = cu(W), cu(x);

julia> @Btime $cW * $cx;
  24.948 μs (86 allocations: 3.11 KiB)

#1447
julia> x = Flux.onehotbatch(rand(1:100, 50), 1:100);

julia> W = rand(128, 100);

julia> @Btime $W * $x;
  5.312 μs (3 allocations: 50.36 KiB)

julia> cW, cx = cu(W), cu(x);

julia> @Btime $cW * $cx;
  8.466 μs (61 allocations: 1.69 KiB)

# this PR
julia> x = Flux.onehotbatch(rand(1:100, 50), 1:100);

julia> W = rand(128, 100);

julia> @Btime $W * $x;
  4.708 μs (3 allocations: 50.56 KiB)

julia> cW, cx = cu(W), cu(x);

julia> @Btime $cW * $cx;
  8.576 μs (63 allocations: 1.73 KiB)
```

2. #556
```jl
#master
julia> valY = randn(1000, 128);

julia> @Btime Flux.onecold($valY);
  365.712 μs (1131 allocations: 38.16 KiB)

julia> @Btime Flux.onecold($(gpu(valY)));
┌ Warning: Performing scalar operations on GPU arrays: This is very slow, consider disallowing these operations with `allowscalar(false)`
└ @ GPUArrays ~/.julia/packages/GPUArrays/jhRU7/src/host/indexing.jl:43
  1.330 s (781248 allocations: 31.59 MiB)

#1447
julia> valY = randn(1000, 128);

julia> @Btime Flux.onecold($valY);
  524.767 μs (8 allocations: 4.00 KiB)

julia> @Btime Flux.onecold($(gpu(valY)));
  27.563 μs (169 allocations: 5.56 KiB)

# this PR
julia> valY = randn(1000, 128);

julia> @Btime Flux.onecold($valY);
  493.017 μs (8 allocations: 4.53 KiB)

julia> @Btime Flux.onecold($(gpu(valY)));
  26.702 μs (171 allocations: 5.61 KiB)
```

## Summary

This should basically be #1447 but simpler to maintain w/ fewer changes. Tests are passing, though I think we should add more tests for one-hot data (currently our test set seems pretty sparse). Performance matches #1447 where I have tested, but please suggest more performance tests. In theory, any performance difference between #1447 and this PR should be recoverable.

### PR Checklist

- [ ] Tests are added
- [ ] Entry in NEWS.md
- [ ] Documentation, if applicable
- [ ] Final review from @DhairyaLGandhi (for API changes).

cc @CarloLucibello @chengchingwen 

Co-authored-by: Kyle Daruwalla <daruwalla@wisc.edu>
Co-authored-by: Kyle Daruwalla <daruwalla.k.public@icloud.com>
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

No branches or pull requests

4 participants