Remove greek-letter keyword arguments #2139
Conversation
Codecov ReportBase: 86.48% // Head: 83.86% // Decreases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #2139 +/- ##
==========================================
- Coverage 86.48% 83.86% -2.62%
==========================================
Files 19 19
Lines 1480 1500 +20
==========================================
- Hits 1280 1258 -22
- Misses 200 242 +42
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -340,15 +342,17 @@ end | |||
| function BatchNorm(chs::Int, λ=identity; | |||
| initβ=zeros32, initγ=ones32, | |||
There was a problem hiding this comment.
This PR does not remove initβ, initγ. IMO they should be replaced with a method where you pass in a Scale layer. But perhaps better part of an overhaul of norm layers.
Getting rid of greek-letter field names may also be a good idea. The norm layers are the worst offenders.
There was a problem hiding this comment.
We can tackle this as part of a bigger rework of norm layer internals to save on some churn.
| @@ -88,8 +89,8 @@ y_dis[1,:], y_dis[2,:] = y_dis[2,:], y_dis[1,:] | |||
| @test crossentropy(ŷ, y_smoothed) ≈ lossvalue_smoothed | |||
| @test crossentropy(ylp, label_smoothing(yl, 2sf)) ≈ -sum(yls.*log.(ylp)) | |||
| @test crossentropy(ylp, yl) ≈ -sum(yl.*log.(ylp)) | |||
| @test iszero(crossentropy(y_same, ya, ϵ=0)) | |||
| @test iszero(crossentropy(ya, ya, ϵ=0)) | |||
| @test iszero(crossentropy(y_same, ya, ϵ=0)) # ε is deprecated | |||
There was a problem hiding this comment.
What's our stance on using @test_deprecated? I'm surprised there appear to be zero usages of it in Flux, perhaps this repo proceeds it?
|
have there been actual complaints about non-ASCII characters? |
|
I thought this was a fairly standard complaint. Sometime a misguided one, when people assume that e.g. |
mcabbott
force-pushed
the
no_greek_kw
branch
from
7816ac7 to
d91f0e3
Compare
src/losses/utils.jl
Outdated
| # Greek-letter keywords deprecated in Flux 0.13 | ||
| # Arguments (old => new, :function, "β" => "beta") | ||
| function _greek_ascii_depwarn(βbeta::Pair, func = :loss, names = "" => "") | ||
| Base.depwarn("""loss function $func no longer accepts greek-letter keyword $(names.first) | ||
| please use ascii $(names.second) instead""", func) | ||
| βbeta.first | ||
| end | ||
| _greek_ascii_depwarn(βbeta::Pair{Nothing}, _...) = βbeta.second |
There was a problem hiding this comment.
Could this live outside of the Losses module?
There was a problem hiding this comment.
We may want to make it non-differentiable as well.
There was a problem hiding this comment.
It could live somewhere else, but where? And is it worth it?
(Most uses are in Losses, I spotted the normalisation ones afterwards, just 3.)
There was a problem hiding this comment.
utils.jl? That's included before any layers or loss functions are defined.
| @@ -340,15 +342,17 @@ end | |||
| function BatchNorm(chs::Int, λ=identity; | |||
| initβ=zeros32, initγ=ones32, | |||
There was a problem hiding this comment.
We can tackle this as part of a bigger rework of norm layer internals to save on some churn.
The interface should be ascii. And especially should never require you to type
εnotϵor the reverse.