-
-
Notifications
You must be signed in to change notification settings - Fork 125
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
Widen tanh_fast
signature
#375
Conversation
CI fails on unrelated things which worked yesterday?
|
Getting the same issues locally on a completely separate project (getting Metalhead pre-training running). |
The only thing I can see that has changed in this timeframe is ChainRules.jl/ChainRulesCore.jl but those changes seem completely orthogonal to the error. |
Thanks, that's useful. If just pushed an upper bound to exclude ChainRules v1.18.0, today's version, just to see? Except that I don't know the syntax it seems... |
e112404
to
2334492
Compare
This reverts commit 56afc0e.
This probably shouldn't wait too long, as the current state breaks Flux's tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both this and FluxML/Flux.jl#1830 are okay with me, though I think this is safer to merge quickly and get tests up again.
As noted here #371 (comment) defining as a fallback only
tanh_fast(::Real)
causes problems for Flux right now. Andtanh
of course makes sense for complex numbers.But really, all the other activation functions only make sense for real numbers, since they often compare
x>0
etc. In the interests of sanity and sensible guard-rails, they should probably all have signature::Real
not::Any
. But can't be changed just yet.