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

Redefines value_and_pullback_function to return a value and a pullback function #35

Closed
wants to merge 5 commits into from

Conversation

sethaxen
Copy link
Member

@sethaxen sethaxen commented Jan 22, 2022

Fixes part of #4. It's a breaking change, but the version number is given a -DEV suffix, since we should also handle #34 before releasing. (EDIT: Ref #36)

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2022

Codecov Report

Merging #35 (fae5c75) into master (979fa04) will decrease coverage by 1.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
- Coverage   80.05%   79.04%   -1.01%     
==========================================
  Files           2        2              
  Lines         396      377      -19     
==========================================
- Hits          317      298      -19     
  Misses         79       79              
Impacted Files Coverage Δ
src/AbstractDifferentiation.jl 76.99% <100.00%> (-1.23%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 979fa04...fae5c75. Read the comment docs.

@sethaxen
Copy link
Member Author

@mohamed82008 in #36 (comment)

This PR needs more thinking. I think when using the Jacobian as a primitive, deriving the value_and_pullback function won't be possible. Only the value and pullback_function is. So it might make sense to define the value_and_pullback_function/value_and_pushforward_function to throw when the Jacobian is used as a primitive. That is users shouldn't expect to use these 2 functions for such a backend or at least the methods won't be define automatically.

I'm not sure I understand what you mean. value_and_pushforward_function is not touched in this PR. And the tests already include examples where Jacobian is used as a primitive and value_and_pushforward_function is tested. e.g. this works with this PR:

julia> using ForwardDiff, AbstractDifferentiation

julia> struct ForwardDiffBackend1 <: AD.AbstractForwardMode end

julia> AD.primal_value(::ForwardDiffBackend1, ::Any, f, xs) = ForwardDiff.value.(f(xs...))

julia> AD.@primitive function jacobian(ab::ForwardDiffBackend1, f, xs)
           if xs isa Number
               return (ForwardDiff.derivative(f, xs),)
           elseif xs isa AbstractArray
               out = f(xs)
               if out isa Number
                   return (adjoint(ForwardDiff.gradient(f, xs)),)
               else
                   return (ForwardDiff.jacobian(f, xs),)
               end
           elseif xs isa Tuple
               error(typeof(xs))      
           else
               error(typeof(xs)) 
           end
       end

julia> ba = ForwardDiffBackend1();

julia> val, back = AD.value_and_pullback_function(ba, identity, randn(10))
([0.05771077577524417, -0.6046897384683757, -0.43893636554383, 1.008345527744184, -1.3448119234793194, 0.0541982394477022, 0.2593213719269091, 1.6426658474014317, 0.651265342373578, -0.795563384561261], AbstractDifferentiation.var"#21#23"{ForwardDiffBackend1, typeof(identity), Tuple{Vector{Float64}}}(ForwardDiffBackend1(), identity, ([0.05771077577524417, -0.6046897384683757, -0.43893636554383, 1.008345527744184, -1.3448119234793194, 0.0541982394477022, 0.2593213719269091, 1.6426658474014317, 0.651265342373578, -0.795563384561261],)))

julia> back(randn(10))
([-1.2729554614899294, 0.1378713003865544, 0.2690893343964529, 0.9206297829092358, -1.3913076264330144, 0.4862468894828594, -1.1728445800434935, 0.6753224021990761, 0.14583421961218776, 0.732158895744132],)

@devmotion
Copy link
Member

This change would be very helpful for other PRs such as #93: #93 (comment)

@gdalle
Copy link
Member

gdalle commented Aug 5, 2023

@devmotion should we close this one?

@devmotion
Copy link
Member

Probably. We can always re-open if it is preferred over #93.

@devmotion devmotion closed this Aug 5, 2023
gdalle added a commit that referenced this pull request Sep 20, 2023
* Define value_and_pullback_function as returning value

* Update definition of Jacobian

* Update tests

* Update API definition

* Increment minor version number

* Make value_and_oullback_function the primitive

* Define pullback_function in terms of value_and_pullback_function

* Define value_and_pullback_function in tests

* Increment version number

* Use value_and_pb_function in CRC and Tracker extensions

* Fix bug in Tracker backend

* More updates

* Fix end

* Handle `nothing`

* Fix test failures

* Use named functions

---------

Co-authored-by: Seth Axen <seth.axen@gmail.com>
Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
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

Successfully merging this pull request may close these issues.

None yet

4 participants