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
use FiniteDifferences.jl for gradient checks #464
base: master
Are you sure you want to change the base?
Conversation
This is good to go now. |
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.
ngradients
is babies first finite differencing function.
Haha, very fair. Fun fact about finite differences though (which I bet FDM doesn't do either) is that you technically should deepcopy
the function the first time you call it (e.g. (deepcopy(f)(x+ϵ) - f(x))/ϵ
). If you don't you can get perturbation-confusion-like bugs from numerical diff.
Anyway, don't fully understand the FDM usage but as long as tests pass I think it's pretty certain that this is more solid than the current setup.
|
||
function default_fdm(f::F) where F | ||
# Attempt to choose a way of finite differencing that will avoid escaping the domain | ||
lower, upper = realdomainrange(F) |
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.
Using realdomainrange
here makes me a bit uneasy... if you're implementing an adjoint it seems like it'd be hard to know that you need to do this (if you do?) and how to do it right. Not any kind of deal-breaker but maybe there's some way to avoid it.
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.
You don't need to do it.
realdomainrange
falls back to (-Inf, Inf)
and actually in basically every test bar a few critical ones, that is already being hit because the tests use anon functions
I actually just moved this code up from below where it was defined for those few critical ones already
Can you open an issue about this in FiniteDIfferences.jl? |
Thought you'd like that one. Classic example is the one that's currently screwing with FD and FD2: D(1) do x
f(y) = (x = x*y)
D(f, 1)
D(f, 1)
end # => 0 Correct answer is one (both of our forward-mode packages say 2). I suspect this is absolutely not worth worrying about in a finite differences package, but can open an issue if it seems useful. |
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.
the rest seems fine to me, it basically stays the same as the what current ngradient
has in Zygote, but using FiniteDifferences, but I'm not sure if we are gonna just overwrite this PR later when #235 is completely done.
That's fine even if we do. |
Problem is FiniteDifferences is just kind of slow.
|
Ah, so the way to really spead up FiniteDifferences.jl is to turn off |
No longer timing out. |
Its really annoying worrying if your gradient check failed because you did something slightly less accurate in a custom adjoint,
or if it was because
ngradients
is babies first finite differencing function.(Ok baby's 3rd it is central fdm after all)
This PR changes it over to use FiniteDifferences.jl
One probably cool switch to FiniteDiff.jl instead, but I couldn't work it out in the 2 minutes i spent looking at the README.
This is substantially more accuate than before.
for
neogradient
being the new code andngradient
being the old.demo
Results: