fix hypot, return Wirtinger appropriately
#55
Conversation
|
Bump. Can we get this merged? |
src/rules/base.jl
Outdated
|
|
||
| function frule(::typeof(hypot), x::Real...) | ||
| Ω = hypot(x...) | ||
| return Ω, Rule((Δ...) -> sum(Δ .* x) * inv(Ω)) |
There was a problem hiding this comment.
| return Ω, Rule((Δ...) -> sum(Δ .* x) * inv(Ω)) | |
| return Ω, Rule((Δ...) -> sum(Δ .* x) / Ω) |
There was a problem hiding this comment.
The problem is that Zero() / x and One() / x doesn't get overdubbed, so this throws errors if all Δs are Zero().
There was a problem hiding this comment.
Is that because only multiplications and additions are overdubbed?
src/rules/base.jl
Outdated
| function frule(::typeof(hypot), x...) | ||
| Ω = hypot(x...) | ||
| return Ω, WirtingerRule( | ||
| Rule((Δ...) -> sum(Δ .* conj.(x) * inv(2Ω))), |
There was a problem hiding this comment.
This should be moved flush with the opening parenthesis with the second line aligned below it.
return Ω, WirtingerRule(Rule((Δ...) -> sum(Δ .* conj.(x) * inv(2Ω))),
Rule((Δ...) -> sum(Δ .* x) * inv(2Ω)))Again in this case, why multiply by inv instead of just dividing?
There was a problem hiding this comment.
See my comment above. It might make sense to overload / for Zero() and One() though.
There was a problem hiding this comment.
Yeah I think that would probably make sense.
There was a problem hiding this comment.
Would it be ok if I left it for now? I'm not too experienced with Cassette and there would be some edge cases to think about, so I think this deserves its own PR.
There was a problem hiding this comment.
I had a go at it. Could maybe be done more elegantly, but works for now.
| @@ -1,10 +1,27 @@ | |||
| _isapprox(x, y; kwargs...) = isapprox(x, y; kwargs...) | |||
There was a problem hiding this comment.
Same comments as in the other PR.
There was a problem hiding this comment.
I'll delete it once #64 is merged
Turns out the derivative for
hypotwas wrong, even in the real case. Now supports an arbitrary number of real or complex arguments. Also in the process discovered that therrulefor*was wrong for complex arguments.Still needs tests.