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

Use existing implementations in StatsFuns #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devmotion
Copy link
Contributor

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 #98 (comment).

@codecov-io
Copy link

codecov-io commented May 24, 2019

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% <ø> (ø) ⬆️
src/activation.jl 75% <0%> (-19.12%) ⬇️

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.

@devmotion
Copy link
Contributor Author

Test errors are unrelated and exist on master as well.

@CarloLucibello
Copy link
Member

why StatFuns's functions allow for array input?

@devmotion
Copy link
Contributor Author

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.

@CarloLucibello
Copy link
Member

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

@devmotion
Copy link
Contributor Author

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.

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.

3 participants