NNlib.jl icon indicating copy to clipboard operation
NNlib.jl copied to clipboard

Use existing implementations in StatsFuns

Open devmotion opened this issue 6 years ago • 6 comments

This PR removes the implementations of the softplus and the logistic function from this package and uses the implementations in StatsFuns instead. It goes back to https://github.com/FluxML/NNlib.jl/pull/98#issuecomment-476617160.

devmotion avatar May 24 '19 12:05 devmotion

Codecov Report

Merging #118 into master will decrease coverage by 0.42%. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
- Coverage   79.15%   78.72%   -0.43%     
==========================================
  Files          24       24              
  Lines         753      752       -1     
==========================================
- Hits          596      592       -4     
- Misses        157      160       +3
Impacted Files Coverage Δ
src/NNlib.jl 100% <ø> (ø) :arrow_up:
src/activation.jl 75% <0%> (-19.12%) :arrow_down:

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 1584f86...e46137d. Read the comment docs.

codecov-io avatar May 24 '19 13:05 codecov-io

Test errors are unrelated and exist on master as well.

devmotion avatar Jun 12 '20 09:06 devmotion

why StatFuns's functions allow for array input?

CarloLucibello avatar Jun 12 '20 09:06 CarloLucibello

They don't, but I didn't want to commit type piracy and hence removed the error message overloads for arrays for the functions implemented in StatsFuns.

devmotion avatar Jun 12 '20 09:06 devmotion

this shouldn't have any impact on zygote or gpu support, right?

CarloLucibello avatar Jun 12 '20 09:06 CarloLucibello

It shouldn't. I just tested it on a GPU, and the sigmoid function seems to be fine. The softplus function errored due to the use of log1p ("... called log1p(x::Float32) in Base.Math, maybe you intended to call log1p(x::Float32) in CUDA"). Zygote seems to work fine.

There's actually an adjoint defined for logistic in https://github.com/FluxML/Zygote.jl/blob/master/src/lib/statsfuns.jl (I guess if this PR would be merge into NNlib, then StatsFuns could become a direct dependency of Zygote as well). I'm a bit surprised that softplus errors on my GPU since it seems https://github.com/JuliaGPU/CuArrays.jl/blob/6ebe463d4a2ad991d5b95388af12ee57f6cc91f9/src/nnlib.jl#L4 even defines a special implementation of softplus for GPUs.

devmotion avatar Jun 12 '20 12:06 devmotion