-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Real valued FFTs #1657
Real valued FFTs #1657
Conversation
Note: The transpose rule is not correct yet (hence the failing tests).
just want to pin this to the issue tracker #1877 |
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.
Beautiful, thanks for doing this! I think it slipped off our radars.
Now with |
self._CheckAgainstNumpy(onp_fn, np_fn, args_maker, check_dtypes=False, | ||
tol=1e-4) | ||
self._CompileAndCheck(np_fn, args_maker, check_dtypes=True) | ||
# Test gradient for differentiable types. | ||
if dtype in inexact_dtypes: | ||
tol = 0.15 # TODO(skye): can we be more precise? | ||
jtu.check_grads(np_fn, args_maker(), order=1, atol=tol, rtol=tol) |
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.
Why get rid of order=1?
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.
order=2
already tests all derivatives up to order=2
. So the separate order=1
was just redundant.
def _irfft_with_zeroed_inputs(irfft_fun): | ||
# irfft isn't defined on the full domain of inputs, so in order to have a | ||
# well defined derivative on the whole domain of the function, we zero-out | ||
# the imaginary part of the first and possibly the last elements. |
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 logic is pretty complicated for a test... would be it be possible/simpler to limit the test inputs to irfft?
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 challenge here is that in order for the finite difference calculation to be correct, I need to apply the restriction to the perturbation as well. I don't think our check_grads
machinery is set-up to do that unless we modify the function passed into 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.
I didn't give this a super close review, but overall LGTM. Lemme know if there's a certain part you wanted me to look at in particular.
Note: The transpose rule is not correct yet (hence the failing tests).Everything should be working -- this is ready for review!