Skip to content

Remove VariableOrderAccumulator and subset and merge on accs #1005

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

Merged
merged 5 commits into from
Aug 7, 2025

Conversation

mhauru
Copy link
Member

@mhauru mhauru commented Aug 1, 2025

These are no longer needed in Turing.jl.

To be merged after #901. Merging that will also clean up the diff of this. Please don't try to review before that.

Copy link
Contributor

github-actions bot commented Aug 1, 2025

Benchmark Report for Commit 1d93529

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 |                  8.3 |                 1.5 |
|           Smorgasbord |       201 | forwarddiff |             typed |  false |                622.9 |                44.2 |
|           Smorgasbord |       201 | forwarddiff | simple_namedtuple |   true |                297.7 |                73.5 |
|           Smorgasbord |       201 | forwarddiff |           untyped |   true |               1166.3 |                28.5 |
|           Smorgasbord |       201 | forwarddiff |       simple_dict |   true |               6381.5 |                28.7 |
|           Smorgasbord |       201 | reversediff |             typed |   true |                992.5 |                41.0 |
|           Smorgasbord |       201 |    mooncake |             typed |   true |                962.0 |                 4.3 |
|    Loop univariate 1k |      1000 |    mooncake |             typed |   true |               5487.4 |                 4.0 |
|       Multivariate 1k |      1000 |    mooncake |             typed |   true |                952.2 |                 9.1 |
|   Loop univariate 10k |     10000 |    mooncake |             typed |   true |              62501.9 |                 3.5 |
|      Multivariate 10k |     10000 |    mooncake |             typed |   true |               8137.0 |                10.0 |
|               Dynamic |        10 |    mooncake |             typed |   true |                126.3 |                12.3 |
|              Submodel |         1 |    mooncake |             typed |   true |                 12.3 |                 5.4 |
|                   LDA |        12 | reversediff |             typed |   true |               1268.0 |                 3.3 |

Copy link
Contributor

github-actions bot commented Aug 1, 2025

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

Copy link

codecov bot commented Aug 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.16%. Comparing base (f0ac109) to head (1d93529).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1005      +/-   ##
==========================================
+ Coverage   81.91%   82.16%   +0.24%     
==========================================
  Files          38       38              
  Lines        4025     3935      -90     
==========================================
- Hits         3297     3233      -64     
+ Misses        728      702      -26     

☔ 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.

@coveralls
Copy link

coveralls commented Aug 1, 2025

Pull Request Test Coverage Report for Build 16680558902

Details

  • 621 of 739 (84.03%) changed or added relevant lines in 27 files are covered.
  • 62 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.8%) to 82.369%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/utils.jl 8 9 88.89%
src/context_implementations.jl 31 33 93.94%
src/experimental.jl 0 2 0.0%
src/pointwise_logdensities.jl 49 51 96.08%
src/model.jl 33 36 91.67%
ext/DynamicPPLJETExt.jl 0 4 0.0%
src/accumulators.jl 28 32 87.5%
src/threadsafe.jl 36 40 90.0%
src/values_as_in_model.jl 20 24 83.33%
src/default_accumulators.jl 37 42 88.1%
Files with Coverage Reduction New Missed Lines %
src/context_implementations.jl 1 93.33%
src/debug_utils.jl 1 88.56%
src/test_utils/contexts.jl 1 88.89%
src/contexts.jl 2 74.45%
src/logdensityfunction.jl 3 57.14%
src/utils.jl 5 71.28%
src/distribution_wrappers.jl 6 28.57%
src/simple_varinfo.jl 7 70.86%
src/abstract_varinfo.jl 9 76.76%
src/threadsafe.jl 9 65.25%
Totals Coverage Status
Change from base Build 16174747412: -0.8%
Covered Lines: 3233
Relevant Lines: 3925

💛 - Coveralls

Copy link
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

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

As @yebai requested #901 (comment) I'll add some extra elaboration in the changelog about why we no longer need num_produce / order / VariableOrderAccumulator. Leaving this comment here as a reminder to myself to do this next week.

(I'm happy to do this since the corresponding Turing PR was mine.)

@penelopeysm penelopeysm self-assigned this Aug 1, 2025
@mhauru mhauru mentioned this pull request Aug 4, 2025
22 tasks
@penelopeysm penelopeysm removed their assignment Aug 4, 2025
@penelopeysm
Copy link
Member

num_produce / order / pMCMC section in changelog has been expanded.

@mhauru mhauru force-pushed the mhauru/remove-vaoacc branch from 84af272 to 9b2bee0 Compare August 7, 2025 10:00
@mhauru mhauru marked this pull request as ready for review August 7, 2025 10:01
@mhauru mhauru requested a review from penelopeysm August 7, 2025 10:01
Copy link
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

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

There's a set_retained_vns_del! in src/threadsafe.jl that should also be removed, but otherwise very happy

@penelopeysm
Copy link
Member

Probably that method never needed to exist because in Turing's particle MCMC code threadsafe evaluation is always disabled:

https://github.com/TuringLang/Turing.jl/blob/d75e6f269b18800e366b383b5059ed08e10ad7a7/src/mcmc/particle_mcmc.jl#L386-L390

I actually wonder whether threadsafe execution would work now with the changes we have... my guess is not but idk.

@mhauru mhauru requested a review from penelopeysm August 7, 2025 11:30
@mhauru mhauru enabled auto-merge August 7, 2025 11:37
@mhauru mhauru added this pull request to the merge queue Aug 7, 2025
Merged via the queue into main with commit 1ed8cc8 Aug 7, 2025
21 of 22 checks passed
@mhauru mhauru deleted the mhauru/remove-vaoacc branch August 7, 2025 12:26
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.

3 participants