Allow only real input for activation functions #98
Conversation
|
I like the idea but would prefer not to annotate each definition like this. How about |
|
I agree, as soon as I had written this initial implementation I thought that would have been easier 😄 I guess, it might also simplify overloading existing methods - e.g., I was wondering whether it would make sense to have a common implementation of |
Only Conway's law. And I think there's some advantage in exposing I think a StatsFuns dependency would be ok, especially since Rmath is going away eventually. We probably want all of it to work well with the GPU and AD ecosystem anyway. |
|
Note that we want to keep the manifest (it's useful for development/testing, especially if we need to use unreleased functionality temporarily). If it needs an update feel free to do that. |
Codecov Report
@@ Coverage Diff @@
## master #98 +/- ##
===========================================
+ Coverage 84.94% 96.61% +11.67%
===========================================
Files 17 17
Lines 611 532 -79
===========================================
- Hits 519 514 -5
+ Misses 92 18 -74
Continue to review full report at Codecov.
|
This reverts commit 62f1929.
|
Ah I see, sorry for that! Cleaning the requirements lead to test errors on Travis, so I thought the easiest solution would be to remove the Manifest all together. I updated it. |
src/activation.jl
Outdated
| # Provide an informative error message if activation functions are called with an array | ||
| for f in (:σ, :σ_stable, :logσ, :relu, :leakyrelu, :elu, :gelu, :swish, :selu, :softsign, :softplus) | ||
| @eval $(f)(x::AbstractArray, args...) = | ||
| error("Use explicit invocations such as `", $(string(f)), ".(x)` to apply activation functions to tensors!") |
There was a problem hiding this comment.
| error("Use explicit invocations such as `", $(string(f)), ".(x)` to apply activation functions to tensors!") | |
| error("Use broadcasting (`", $f, ".(x)`) to apply activation functions to arrays.") |
I think $f will print the same but worth checking quickly.
There was a problem hiding this comment.
Actually in my initial implementation I used $f, but I added string since I was not satisfied with the output. With just $f I get, e.g.,
ERROR: Use broadcasting (
NNlib.σ.(x)) to apply activation functions to arrays.
whereas with $(string(f))
ERROR: Use broadcasting (
σ.(x)) to apply activation functions to arrays.
is printed.
There was a problem hiding this comment.
I see, a bit surprising that they behave differently, but that makes sense.
|
Unfortunately the changes to the manifest etc are conflicting now, but that should be easy to fix. I suggested a tweak to the error message but otherwise this is good to go! |
|
Perfect, thank you! |
This PR implements what was discussed in #85. A simple macro ensures that activation functions are defined for real inputs only and calling them with an array results in a (hopefully informative) error message.