more generic ZScoreTransform, UnitRangeTranform to support CuArrays #622

sdewaele · 2020-11-29T21:47:06Z

Adresses #619.

Make transformations work for CuArray vectors. They currently only work CuArray matrices.
Find an appropriate home for methods currently defined in "test/cuda.jl" during testing.

This code passes the tests and is ready for merge from my perspective.

…or CuArrays

…ependency using Requires

wildart · 2020-12-01T01:10:02Z

src/StatsBase.jl

+function __init__()
+    @require CUDA="052768ef-5323-5732-b1bb-66c8b64840ba" begin
+        using .CUDA
+        function _compute_extrema(X::Union{CuMatrix{<:Real},Adjoint{T,CuMatrix{T}} where T<:Real})


Do you use this function somewhere? If not, whole __init__ can be removed.

It is used here in the fit(::Type{UnitRangeTransform}, ...) function. The existing implementation does not work (or, is very slow) for CuArrays because the array is indexed.

Sorry, but I don't think using Requires is OK for a package like StatsBase, as it will increase load times of many packages. We should find better abstractions so that StatsBase doesn't have to be aware of CUDA.

Couldn't _compute_extrema be replaced with extrema.(eachcol(X))? Would that be enough to use optimized CUDA methods?

I was not too happy with using Requires either. I have removed it now. I also replaced _compute_extrema by extrema. The caveat that extrema has not yet been implemented for CUDA arrays, but it is reasonable to expect that it will become available in the future; it is mentioned in JuliaGPU/CUDA.jl#265 (comment)

src/transformations.jl

wildart · 2020-12-01T02:03:46Z

src/transformations.jl

-    m, s = mean_and_std(X)
-    return ZScoreTransform(1, dims, (center ? [m] : zeros(T, 0)),
-                                    (scale ? [s] : zeros(T, 0)))
+    return fit(ZScoreTransform, reshape(X, :, 1);dims=dims, center=center, scale=scale)


The vector structure of the input input will be lost, and transform will return a 1-column matrix instead of a vector.

Thanks for your review. I will fix this shortly. The issue is that when I stay closer to the original code, I got an inference error. I'll look for an alternate solution to work around that.

It looks like transform still works as expected, i.e. it returns a Vector.

julia> X = rand(3) 3-element Vector{Float64}: 0.0220367 0.019156 0.572394 julia> t = fit(ZScoreTransform, X) ZScoreTransform{Float64, Vector{Float64}}(1, 1, [0.204529], [0.318584]) julia> transform(t, X) 3-element Vector{Float64}: -0.572823 -0.581866 1.15469 julia> Xt = transform(t, X) 3-element Vector{Float64}: -0.572823 -0.581866 1.15469 julia> reconstruct(t, Xt) 3-element Vector{Float64}: 0.0220367 0.019156 0.572394

This is achieved through the transform method that is specialized for vectors:

…able

test/transformations.jl

…testing

wildart

Minor comment on allocation, otherwise looks good.

wildart · 2020-12-04T21:13:30Z

src/transformations.jl

-    end
+    dims ∈ [1,2] || throw(DomainError(dims, "fit only accept dims to be 1 or 2."))
+    tmin_tmax = extrema(X; dims=dims)
+    tmin, tmax = [vec(getindex.(tmin_tmax, i)) for i in 1:2]


You can form a generator instead of allocating an array.

tmin, tmax = (vec(getindex.(tmin_tmax, i)) for i in 1:2)

Done. Thanks for the suggestion.

nalimilan · 2020-12-06T11:01:35Z

src/transformations.jl

-    for i = 1:l
-        @inbounds tmax[i] = 1 / (tmax[i] - tmin[i])
-    end
+    dims ∈ [1,2] || throw(DomainError(dims, "fit only accept dims to be 1 or 2."))


Suggested change

dims ∈ [1,2] || throw(DomainError(dims, "fit only accept dims to be 1 or 2."))

dims ∈ (1, 2)] || throw(DomainError(dims, "fit only accept dims to be 1 or 2."))

src/transformations.jl

nalimilan · 2020-12-06T11:08:48Z

src/moments.jl

    m, s
 end

-


No need to change this.

I restored the additional line.

nalimilan · 2020-12-06T11:10:34Z

src/transformations.jl

-    end
+    dims ∈ [1,2] || throw(DomainError(dims, "fit only accept dims to be 1 or 2."))
+    tmin_tmax = extrema(X; dims=dims)
+    tmin, tmax = (vec(getindex.(tmin_tmax, i)) for i in 1:2)


Ah so it's too bad that this forces copying the result to extract the min and max. Do you think CUDA supports eachslice, or would it be efficient to iterate over rows/columns and call extrema on them? Then we could store directly the result in the right format.

I have been experimenting a little bit, but have not found a good solution. This prevents the copying, but does not work with CuArrays because of the element-wise setindex:

l = size(X, dims==1 ? 2 : 1) tmin, tmax = (similar(X, l) for _ in 1:2) for (i, x) in enumerate(eachslice(X; dims=dims)) tmin[i], tmax[i] = extrema(x) # per-element assignment does not work with CUDA end

Then there is zip + ... but I think the performance of that is even worse because of the additional penalty of ...:

tmin, tmax = collect.(zip((extrema(x) for x in eachslice(X; dims=dims))...))

Also, now the outputs no longer end up in a CuArray which will mean the transform! will fail on CuArray.

An option is to not pursue support for UnitRangeTransform in this PR. Anyway, extrema has not yet been implemented in CUDA.jl. I can reintroduce _compute_extrema which can then later be implemented for CuArrays.

Ah, too bad. Maybe better not implement this until CUDA supports it. Could you ask for advice there about the best way to achieve this without copying?

I asked the question on discourse, see here.

Might also be worth posting a link on #gpu on Slack.

In the latest commit I added a new _compute_extrema that eliminates the allocation. With that, it should now be as efficient as the original implementation for regular arrays. Also, this allow for a temporary solution for CuArrays by implementing a specialized _compute_extrema for CuArrays:

function StatsBase._compute_extrema(X::CuArray; dims) tmin = vec(minimum(X; dims=dims)) tmax = vec(maximum(X; dims=dims)) return tmin, tmax end

This is not optimal, but it works. Also, given the compute power of a GPU, I assume it will still give a significant performance improvement.

Cool. Though you'll need Compat 2.2 to use eachslice on Julia 1.0.

test/transformations.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

…chslice (requires 1.1)

sdewaele · 2020-12-17T20:42:22Z

I suggest to merge the PR now. It will have the following benefits:

ZScoreTransform has been made more generic and works for CuArray
UnitRangeTransform has been made more generic in preparation for using it for CuArray.

ZScoreTransform is anyway more important in my opinion; at least it is probably the more frequently used normalization in machine learning.

After merging this PR request, users can now define _compute_extrema for CuArrays as described to make UnitRangeTransform work. This is not possible with the current definition of UnitRangeTransform because it is not generic enough.

P.S. The CI states that "Some checks were not successful". I don't think this has anything to do with this PR; it has failed on downloading the registry. Probably re-running the test will eliminate the error.

nalimilan

Fine with me. Just a few stylistic comments.

src/transformations.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

nalimilan · 2020-12-19T21:09:28Z

Thanks!

sdewaele · 2020-12-19T22:06:00Z

You are welcome. Thanks for your thorough review @nalimilan and @wildart.

more generic ZScoreTransform, UnitRangeTranform; method definitions f…

60c3d25

…or CuArrays

This was referenced Nov 29, 2020

StatsBase.transform fails on CuArray JuliaGPU/CUDA.jl#426

Closed

Implement Statistics.varm/stdm instead of Statistics._var JuliaGPU/CUDA.jl#583

Merged

fit(ZScoreTransform, ...) works for vectors; added CUDA as optional d…

8b81017

…ependency using Requires

wildart reviewed Dec 1, 2020

View reviewed changes

sdewaele added 2 commits December 1, 2020 21:00

space added; removed commented

72dd628

removed CUDA as test dependency; GPU tests only done if CUDA is avail…

c36a267

…able

nalimilan reviewed Dec 4, 2020

View reviewed changes

test/transformations.jl Outdated Show resolved Hide resolved

replaced _compute_extrema by extrema; removed Requires; removed CUDA …

63ee063

…testing

wildart approved these changes Dec 4, 2020

View reviewed changes

replaced array comprehension by a generator to reduce allocations

a263546

nalimilan reviewed Dec 6, 2020

View reviewed changes

sdewaele and others added 5 commits December 6, 2020 11:46

Update src/transformations.jl

668a92d

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

added dims ∈ (1,2); reverted whitespace change in src/moments.jl

df832b6

restored isequal in test; whitespace addition in src/transformations.jl

8fe6975

added _compute_extrema to reduce memory allocation

bf1c475

_compute_extrema now also compatible with Julia 1.0 after removing ea…

8ad7b9e

…chslice (requires 1.1)

nalimilan reviewed Dec 18, 2020

View reviewed changes

src/transformations.jl Outdated Show resolved Hide resolved

src/transformations.jl Outdated Show resolved Hide resolved

src/transformations.jl Outdated Show resolved Hide resolved

sdewaele and others added 3 commits December 19, 2020 11:24

Update src/transformations.jl

c2231f1

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/transformations.jl

00608a9

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

Update src/transformations.jl

bee6d06

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

nalimilan merged commit 83ccc37 into JuliaStats:master Dec 19, 2020

sdewaele mentioned this pull request Dec 19, 2020

ZScoreTransform and UnitRangeTransform do not support CuArray #619

Closed

	dims ∈ [1,2] \|\| throw(DomainError(dims, "fit only accept dims to be 1 or 2."))
	dims ∈ (1, 2)] \|\| throw(DomainError(dims, "fit only accept dims to be 1 or 2."))

more generic ZScoreTransform, UnitRangeTranform to support CuArrays #622

more generic ZScoreTransform, UnitRangeTranform to support CuArrays #622

Uh oh!

Conversation

sdewaele commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wildart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdewaele Dec 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdewaele Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sdewaele commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nalimilan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nalimilan commented Dec 19, 2020

Uh oh!

sdewaele commented Dec 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sdewaele commented Nov 29, 2020 •

edited

Loading

sdewaele Dec 6, 2020 •

edited

Loading

sdewaele Dec 11, 2020 •

edited

Loading

sdewaele commented Dec 17, 2020 •

edited

Loading