Skip to content

InitContext, part 5 - Remove SamplingContext, SampleFrom{Prior,Uniform}, {tilde_,}assume #985

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

Draft
wants to merge 4 commits into
base: py/actually-use-init
Choose a base branch
from

Conversation

penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Jul 10, 2025

Part 1: Adding hasvalue and getvalue to AbstractPPL
Part 2: Removing hasvalue and getvalue from DynamicPPL
Part 3: Introducing InitContext and init!!
Part 4: Using ParamsInit to implement predict, returned, and initialize_values

This is part 5/N of #967.

This PR removes everything that is no longer needed. SamplingContext, SampleFromPrior, SampleFromUniform, now have direct one-to-one replacements (albeit with slightly different behaviour since they now always overwrite variables in the varinfo).

It also removes assume and tilde_assume.

Prior to this PR we had two different kinds of assume, one with a sampler and one without. Now we only have the one without, so we can just move that definition into tilde_assume!!(::DefaultContext, ...).

Finally, tilde_assume has been subsumed into tilde_assume!! as we can just dispatch on the type of right. (Previously this wasn't possible because there was a lot of stuff about is_rhs_model, etc. etc. which was removed in #960.)

Closes #859
Closes #955

@penelopeysm penelopeysm changed the base branch from main to py/actually-use-init July 10, 2025 17:00
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch 2 times, most recently from c32d112 to b7221cc Compare July 10, 2025 17:16
@penelopeysm penelopeysm changed the title InitContext, part 5 - Remove SamplingContext, SampleFromPrior, SampleFromUniform, and associated code InitContext, part 5 - Remove SamplingContext, SampleFrom{Prior,Uniform}, {tilde_,}assume Jul 10, 2025
This was referenced Jul 10, 2025
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch from b7221cc to 3835d01 Compare July 10, 2025 17:24
@penelopeysm penelopeysm force-pushed the py/actually-use-init branch from d55d378 to a392451 Compare July 10, 2025 17:33
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch from 3835d01 to 713034f Compare July 10, 2025 17:42
@penelopeysm penelopeysm force-pushed the py/actually-use-init branch from a392451 to 12d93e5 Compare July 10, 2025 17:44
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch from 713034f to 45a97ee Compare July 10, 2025 17:45
@penelopeysm penelopeysm force-pushed the py/actually-use-init branch from 12d93e5 to 7a8e7e3 Compare July 10, 2025 17:47
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch from 45a97ee to e817d9c Compare July 10, 2025 17:48
@penelopeysm penelopeysm force-pushed the py/actually-use-init branch from 7e38bbe to 1d8bceb Compare July 19, 2025 22:37
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch from 92772b5 to 1ffc409 Compare July 19, 2025 22:39
@penelopeysm penelopeysm force-pushed the py/actually-use-init branch from 1d8bceb to 2edcd10 Compare July 20, 2025 00:59
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch 2 times, most recently from 1957b06 to 4fc60dc Compare July 20, 2025 17:47
Copy link
Contributor

github-actions bot commented Jul 20, 2025

Benchmark Report for Commit 7aae613

Computer Information

Julia Version 1.11.6
Commit 9615af0f269 (2025-07-09 12:58 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 4 × AMD EPYC 7763 64-Core Processor
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, znver3)
Threads: 1 default, 0 interactive, 1 GC (on 4 virtual cores)

Benchmark Results

|                 Model | Dimension |  AD Backend |      VarInfo Type | Linked | Eval Time / Ref Time | AD Time / Eval Time |
|-----------------------|-----------|-------------|-------------------|--------|----------------------|---------------------|
| Simple assume observe |         1 | forwarddiff |             typed |  false |                 13.6 |                 1.4 |
|           Smorgasbord |       201 | forwarddiff |             typed |  false |                772.3 |                37.5 |
|           Smorgasbord |       201 | forwarddiff | simple_namedtuple |   true |                422.1 |                57.3 |
|           Smorgasbord |       201 | forwarddiff |           untyped |   true |               1106.4 |                32.9 |
|           Smorgasbord |       201 | forwarddiff |       simple_dict |   true |               7728.7 |                26.6 |
|           Smorgasbord |       201 | reversediff |             typed |   true |               1148.7 |                36.4 |
|           Smorgasbord |       201 |    mooncake |             typed |   true |               1119.6 |                12.4 |
|    Loop univariate 1k |      1000 |    mooncake |             typed |   true |               6996.7 |                52.1 |
|       Multivariate 1k |      1000 |    mooncake |             typed |   true |               1015.3 |                 8.9 |
|   Loop univariate 10k |     10000 |    mooncake |             typed |   true |              83355.2 |                47.9 |
|      Multivariate 10k |     10000 |    mooncake |             typed |   true |               8411.9 |                10.0 |
|               Dynamic |        10 |    mooncake |             typed |   true |                140.8 |                23.9 |
|              Submodel |         1 |    mooncake |             typed |   true |                 18.1 |                26.6 |
|                   LDA |        12 | reversediff |             typed |   true |               1212.8 |                 1.9 |

Copy link

codecov bot commented Jul 20, 2025

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (py/actually-use-init@4c96020). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/context_implementations.jl 86.66% 2 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                   @@
##             py/actually-use-init     #985   +/-   ##
=======================================================
  Coverage                        ?   79.89%           
=======================================================
  Files                           ?       39           
  Lines                           ?     3914           
  Branches                        ?        0           
=======================================================
  Hits                            ?     3127           
  Misses                          ?      787           
  Partials                        ?        0           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

DynamicPPL.jl documentation for PR #985 is available at:
https://TuringLang.github.io/DynamicPPL.jl/previews/PR985/

@penelopeysm penelopeysm force-pushed the py/actually-use-init branch 2 times, most recently from 3a16f9c to 4c96020 Compare July 26, 2025 18:47
@penelopeysm penelopeysm force-pushed the py/remove-samplingcontext branch from 3951d1d to 79a5f3c Compare July 26, 2025 19:20
test/lkj.jl Outdated
Copy link
Member Author

@penelopeysm penelopeysm Jul 26, 2025

Choose a reason for hiding this comment

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

It's maybe a bit opinionated to delete this file. I personally don't see the point of it. It's essentially testing that rand(::LKJ) or rand(::LKJCholesky) works (which is a Distributions problem) and/or testing that LKJ / LKJCholesky linking works correctly (which is a Bijectors problem).

The direct replacement for these tests would be something like this:

for init_strategy in [PriorInit(), UniformInit()]
    lkjchol_samples = [DynamicPPL.init(Random.default_rng, @varname(x), LKJCholesky(...), init_strategy) for _ in 1:1000]
    @test isapprox(mean(lkjchol_samples), expected_mean)
end

However, I don't quite see why we should perform this test for LKJ/LKJCholesky and not for any other distributions. DynamicPPL.init only uses Bijectors functionality, and the only reason why we would test LKJ specifically is if we were concerned about Bijectors' reliability on LKJ. But that should be tested in Bijectors, not here.

@@ -77,48 +77,6 @@ using DynamicPPL.TestUtils.AD: run_ad, WithExpectedResult, NoTest
end
end

@testset "Turing#2151: ReverseDiff compilation & eltype(vi, spl)" begin
Copy link
Member Author

@penelopeysm penelopeysm Jul 26, 2025

Choose a reason for hiding this comment

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

I couldn't find a way to reproduce this test without SamplingContext and the whole tilde-pipeline machinery that this PR removes, so I think that this is no longer relevant.

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.

1 participant