Skip to content
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

Forward difference gradient has the same number of function calls as central difference #132

Open
benide opened this issue Mar 14, 2022 · 1 comment

Comments

@benide
Copy link

benide commented Mar 14, 2022

Changes to gradients.jl made in fc5a08e removed any way to reduce the number of function calls made when using forward difference for gradients as compared to central difference. The finite_difference_gradient! functions assume that f(x) will be provided in the GradientCache for forward differences, but the only constructor for GradientCache forces fx = nothing.

Here are the lines returning the only cache the constructor will give you:

GradientCache{Nothing,typeof(_c1),typeof(_c2),typeof(_c3),fdtype,
returntype,inplace}(nothing,_c1,_c2,_c3)

I can think of three ways to fix it:

  1. There could be a constructor that accepts an input of fx returns the a cache with fx, although fc5a08e was trying to get rid of extra constructors.
  2. Instead of using the type Nothing for the first parameter of the returned GradientCache, use Union{Nothing,returntype} so the user can manually update it later.
  3. Or, don't bother using the cache and change the following line
    fx, c1, c2, c3 = cache.fx, cache.c1, cache.c2, cache.c3

    to _fx, c1, c2, c3 = cache.fx, cache.c1, cache.c2, cache.c3 and then before this for loop:
    if fdtype == Val(:forward)
    @inbounds for i eachindex(x)

    add something along the lines of fx = _fx == Nothing ? f(x) : _fx

2 makes the most sense to me, but I'm not sure what the performance implications are.

@ChrisRackauckas
Copy link
Member

I would prefer not to do 2. Union splitting isn't too bad for runtime performance, but can sneakily lead to more compile times. I think we need to do (1) and add fx to the cache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants