- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 23
type parametrization for rules #209
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
Conversation
🥳 |
I'm going to merge tomorrow if there is no feedback. |
I don't have much time but I do think all of these need to remain errors: That could be achieved by making the macro produce |
struct Rule{T1, T2} | ||
eta::T1 | ||
beta::T2 | ||
Rule(eta, beta = (0.7, 0.8)) = eta < 0 ? error() : new(eta, beta) |
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.
This example is now broken, ERROR: syntax: too few type parameters specified in "new{...}"
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.
yes but new{typeof(eta),typeof(beta)}(eta, beta)
wouldn't be too verbose
sum(abs2, gradient(m -> sum(abs2, destructure(m)[1]), (x = v, y = sin, z = [4,5,6.0]))[1][1]) | ||
end[1] ≈ [8,16,24] | ||
# Zygote error in (::typeof(∂(canonicalize)))(Δ::NamedTuple{(:backing,), Tuple{NamedTuple{(:x, :y, :z) | ||
# Diffractor error in perform_optic_transform | ||
end | ||
|
||
false && @testset "using Yota" begin |
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.
This should really be a different PR...
test/interface.jl
Outdated
@test typeof(r.b1) == Float64 # int promoted to float | ||
@test typeof(r.b2) == Float64 # Float64 not converted to Float32 | ||
@test r.c == (1.0, 2.0) | ||
end |
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.
These are only tests of the happy paths. I think we should be more defensive, don't trust people to hold it right, error as soon as they try to make things which don't make sense.
end | ||
rule = Meta.isexpr(expr.args[2], :<:) ? expr.args[2].args[1] : expr.args[2] | ||
params = [Expr(:kw, nv...) for nv in zip(names,vals)] | ||
check = :eta in names ? :(eta < 0 && throw(DomainError(eta, "the learning rate cannot be negative"))) : nothing | ||
expr.args[2] = Expr(:(<:), Expr(:curly, rule, type_params...), :AbstractRule) |
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.
Current use is always @def struct Rprop <: AbstractRule
. What's this for?
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.
this adds the the type parameters
those examples are now errors |
Fix #205