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.

Sign up for GitHub

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

Jump to bottom

Use value_and_pullback_function as a primitive #36

Closed
wants to merge 4 commits into from
Closed

Use value_and_pullback_function as a primitive #36

wants to merge 4 commits into from

Conversation

sethaxen
Copy link
Member

Fixes #34. Breaking change marked with -DEV version number in case it is merged before #35.

@sethaxen sethaxen mentioned this pull request Jan 22, 2022
Closed
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2022 •
edited
Loading

Codecov Report

Merging #36 (3dcb8ce) into master (979fa04) will decrease coverage by 1.56%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
- Coverage   80.05%   78.48%   -1.57%     
==========================================
  Files           2        2              
  Lines         396      395       -1     
==========================================
- Hits          317      310       -7     
- Misses         79       85       +6
Impacted Files Coverage Δ
src/AbstractDifferentiation.jl 76.47% <81.81%> (-1.75%) ⬇️

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...3dcb8ce. Read the comment docs.

@mohamed82008
Copy link
Member

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.

@mohamed82008
Copy link
Member

mohamed82008 commented Jan 26, 2022 •
edited
Loading

Ah sorry I meant to comment on the other PR. Anyways, I suggest merging this PR and #34 and updating the README once.

@sethaxen
Copy link
Member Author

Ah sorry I meant to comment on the other PR. Anyways, I suggest merging this PR and #34 and updating the README once.

I assume you mean #35? That PR already updates the ReadMe for the redefinition of value_and_pullback_function. Currently definition of a primitive is not documented, so we need no updates in this PR.

@mohamed82008
Copy link
Member

I want to take a stab at this myself if you don't mind. I will open another PR.

@devmotion devmotion mentioned this pull request May 11, 2022
Closed
@devmotion devmotion mentioned this pull request Jul 25, 2023
Merged
@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
@devmotion @sethaxen @gdalle
Update of #36 and #35 (#93)
3c18e86 
* 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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make value_and_pullback_function a primitive instead of pullback_function
5 participants
@sethaxen @codecov-commenter @mohamed82008 @gdalle @devmotion