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

Add fix and unfix #488

Merged
merged 21 commits into from
Jul 13, 2023
Merged

Add fix and unfix #488

merged 21 commits into from
Jul 13, 2023

Conversation

torfjelde
Copy link
Member

We currently have condition and decondition which does what you'd expect.

But sometimes you want to fix variables without also making them be considered as an observation (which condition does imply). Use-cases are plenty, but for example:

  • Interventions / causal inference. IIUC fix can also be considered a do since it effectively cuts off the incoming edges to the variable we fix.
  • In Turing.predict it would be nice to just determine which variables are considered observations and which aren't, and then simply sample the observations rather than requiring the user to manually construct the model without the observations conditioned. The reason why we can't currently do that now is because it's very much within the realms of possibility that a user might do model | (some_parameter=10, ), run inference on this model, and then run predict. In such a scenario you'd end up also sampling some_parameter, which is unlikely to be the user's intention. If the user instead did fix(model, some_parameter=10), then we could correctly determine the observations. Related: predict() from a Prior-sampled chain returns no predictions Turing.jl#2012 (comment)

I don't think this PR is too controversial with the exception of introducing an additional branch in the generated tilde-statements 😕 IMO this case is worth it + it's a very simple branch.

src/compiler.jl Outdated Show resolved Hide resolved
test/contexts.jl Outdated Show resolved Hide resolved
test/contexts.jl Outdated Show resolved Hide resolved
test/contexts.jl Outdated Show resolved Hide resolved
test/contexts.jl Outdated Show resolved Hide resolved
test/contexts.jl Outdated Show resolved Hide resolved
get_conditioned_value and has_conditioned_value
src/contexts.jl Outdated Show resolved Hide resolved
src/contexts.jl Outdated Show resolved Hide resolved
src/contexts.jl Outdated Show resolved Hide resolved
src/model.jl Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Pull Request Test Coverage Report for Build 5298956606

  • 85 of 112 (75.89%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 75.815%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/compiler.jl 17 18 94.44%
src/contexts.jl 63 89 70.79%
Files with Coverage Reduction New Missed Lines %
src/contexts.jl 1 77.27%
Totals Coverage Status
Change from base Build 5278850283: 0.1%
Covered Lines: 1953
Relevant Lines: 2576

💛 - Coveralls

@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Patch coverage: 75.89% and project coverage change: +0.04 🎉

Comparison is base (e6dd4ef) 76.40% compared to head (4d28cbf) 76.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   76.40%   76.45%   +0.04%     
==========================================
  Files          21       21              
  Lines        2522     2612      +90     
==========================================
+ Hits         1927     1997      +70     
- Misses        595      615      +20     
Impacted Files Coverage Δ
src/DynamicPPL.jl 100.00% <ø> (ø)
src/contexts.jl 77.27% <70.78%> (-6.58%) ⬇️
src/compiler.jl 92.92% <94.44%> (+0.07%) ⬆️
src/model.jl 86.36% <100.00%> (-0.23%) ⬇️

... and 1 file with indirect coverage changes

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

@torfjelde
Copy link
Member Author

I don't think there's anything holding this PR back? We've agreed that fix and unfix should go to APPL at some point, in addition to deprecating decondition in favour of uncondition, which is also a APPL issue (and it has been raised accordingly).

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

A few smaller comments regarding the docstrings 🙂

I'm a bit worried that GH points out quite a few new lines in the PR that are not covered by tests?

IMO a good follow-up PR would be to unify and simplify all these tree-based property extractions. In particular, the _nested functions seem very similar logic-wise, so I wonder if their implementation could be unified by some more generic internal tree walk?

docs/src/api.md Outdated Show resolved Hide resolved
src/model.jl Outdated Show resolved Hide resolved
src/model.jl Outdated Show resolved Hide resolved
src/model.jl Outdated Show resolved Hide resolved
src/model.jl Outdated Show resolved Hide resolved
src/model.jl Outdated Show resolved Hide resolved
torfjelde and others added 2 commits July 7, 2023 17:10
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
docs/src/api.md Outdated Show resolved Hide resolved
docs/src/api.md Outdated Show resolved Hide resolved
docs/src/api.md Outdated Show resolved Hide resolved
docs/src/api.md Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@yebai yebai enabled auto-merge July 13, 2023 19:12
@yebai yebai added this pull request to the merge queue Jul 13, 2023
Merged via the queue into master with commit 7d312cd Jul 13, 2023
12 of 14 checks passed
@yebai yebai deleted the torfjelde/fixed branch July 13, 2023 19:35
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

3 participants