Skip to content

New Zygote context in every call to AD.pullback_function#77

Merged
devmotion merged 2 commits intomasterfrom
dw/zygote_context
Mar 3, 2023
Merged

New Zygote context in every call to AD.pullback_function#77
devmotion merged 2 commits intomasterfrom
dw/zygote_context

Conversation

@devmotion
Copy link
Member

Alternative to #76 that I had in mind when writing #76 (review).

In contrast to #76, with this PR ZygoteBackend would still be a ReverseRuleConfigBackend, and hence the behaviour of ZygoteBackend and ReverseRuleConfigBackend(Zygote.ZygoteRuleConfig()) would not differ.

Fixes #69. Closes #70. Closes #76.

@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Patch coverage: 75.00% and project coverage change: -0.12 ⚠️

Comparison is base (19ce815) 84.36% compared to head (848ee38) 84.25%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   84.36%   84.25%   -0.12%     
==========================================
  Files           8        8              
  Lines         467      470       +3     
==========================================
+ Hits          394      396       +2     
- Misses         73       74       +1     
Impacted Files Coverage Δ
src/backends.jl 83.33% <0.00%> (-16.67%) ⬇️
ext/AbstractDifferentiationChainRulesCoreExt.jl 100.00% <100.00%> (ø)
ext/AbstractDifferentiationZygoteExt.jl 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@devmotion
Copy link
Member Author

I made the fix more modular in 848ee38, to avoid code duplication and make it easy to apply similar fixes for other AD backends if needed.

@ToucheSir
Copy link

ToucheSir commented Mar 3, 2023

Well, that eliminates my only substantive point in #76 (comment). As the adage goes, the fastest way to get something done is to do it wrong and get someone to correct it for you 😆

@devmotion
Copy link
Member Author

That is, you approve this PR? 🙂

Copy link

@ToucheSir ToucheSir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, though I'm not sure how much weight it carries here :)

@devmotion
Copy link
Member Author

I trust your Zygote expertise 🙂

@devmotion devmotion merged commit 3218c08 into master Mar 3, 2023
@devmotion devmotion deleted the dw/zygote_context branch March 3, 2023 20:54
marius311 added a commit to marius311/AbstractDifferentiation.jl that referenced this pull request Apr 7, 2023
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.

Zygote context cache incorrectly(?) persists between AD calls

2 participants