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

Mark Sampling context as not needing derivatives #556

Merged
merged 5 commits into from
Nov 21, 2023

Conversation

wsmoses
Copy link
Contributor

@wsmoses wsmoses commented Nov 8, 2023

No description provided.

@devmotion
Copy link
Member

Is it possible to add a simple test for this change? Generally, it might also be a good candidate for an extension.

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (c9489aa) 80.24% compared to head (b3a3757) 80.26%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/DynamicPPL.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
+ Coverage   80.24%   80.26%   +0.01%     
==========================================
  Files          25       26       +1     
  Lines        3159     3167       +8     
==========================================
+ Hits         2535     2542       +7     
- Misses        624      625       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yebai
Copy link
Member

yebai commented Nov 9, 2023

I'm happy with the changes but perhaps convert this to an extension

@wsmoses
Copy link
Contributor Author

wsmoses commented Nov 19, 2023

So the only dependency here is our interface package EnzymeCore, rather than the fully heavyweight Enzyme. Extension is fine, but this is similar to how ChainRulesCore is currently used within this package already.

@wsmoses
Copy link
Contributor Author

wsmoses commented Nov 19, 2023

@devmotion done

@devmotion
Copy link
Member

There are a few dependencies that were added before support for weak dependencies was available and should be made weak dependencies (e.g. Test #550, ChainRulesCore, and ZygoteRules).

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

Just a few comments:)

test/contexts.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@yebai
Copy link
Member

yebai commented Nov 21, 2023

Thanks @wsmoses @devmotion @torfjelde!

@devmotion devmotion enabled auto-merge November 21, 2023 14:03
@devmotion devmotion added this pull request to the merge queue Nov 21, 2023
Merged via the queue into TuringLang:master with commit 03e4ba2 Nov 21, 2023
11 of 13 checks passed
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.

4 participants