Link varinfo by default in AD testing utilities; make test suite run on linked varinfos #890

penelopeysm · 2025-04-15T13:38:45Z

This is a followup from yesterday's meeting.

I think this is a breaking change, so merging into minor release...

github-actions · 2025-04-15T13:48:21Z

Benchmark Report for Commit `976885c`

Computer Information

Julia Version 1.11.5
Commit 760b2e5b739 (2025-04-14 06:53 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 |                  9.5 |                 1.6 |
|           Smorgasbord |       201 | forwarddiff |             typed |  false |                756.4 |                34.2 |
|           Smorgasbord |       201 | forwarddiff | simple_namedtuple |   true |                319.8 |                65.0 |
|           Smorgasbord |       201 | forwarddiff |           untyped |   true |               1221.8 |                27.3 |
|           Smorgasbord |       201 | forwarddiff |       simple_dict |   true |               3264.9 |                23.8 |
|           Smorgasbord |       201 | reversediff |             typed |   true |               1439.4 |                29.9 |
|           Smorgasbord |       201 |    mooncake |             typed |   true |                935.5 |                 5.3 |
|    Loop univariate 1k |      1000 |    mooncake |             typed |   true |               5428.5 |                 4.1 |
|       Multivariate 1k |      1000 |    mooncake |             typed |   true |               1004.6 |                 8.9 |
|   Loop univariate 10k |     10000 |    mooncake |             typed |   true |              61134.8 |                 3.7 |
|      Multivariate 10k |     10000 |    mooncake |             typed |   true |               8897.3 |                 9.7 |
|               Dynamic |        10 |    mooncake |             typed |   true |                135.6 |                12.2 |
|              Submodel |         1 |    mooncake |             typed |   true |                 27.2 |                 6.9 |
|                   LDA |        12 | reversediff |             typed |   true |                482.1 |                 5.8 |

codecov · 2025-04-15T14:10:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.76%. Comparing base (5ba3530) to head (976885c).
Report is 1 commits behind head on breaking.

Additional details and impacted files

@@            Coverage Diff            @@
##           breaking     #890   +/-   ##
=========================================
  Coverage     84.75%   84.76%           
=========================================
  Files            35       35           
  Lines          3877     3879    +2     
=========================================
+ Hits           3286     3288    +2     
  Misses          591      591

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

sunxd3

LGTM!

test/ad.jl

penelopeysm · 2025-04-15T16:15:21Z

I've actually been thinking about the interface a bit again, I thought that just using @test would be fine, but it's very hard to (1) capture the result if the test throws an error (this is useful for e.g. checking how wrong something was or whether it contained NaN results) and also (2) determine what kind of error was thrown...

Closes #891

penelopeysm · 2025-04-16T00:21:06Z

@sunxd3 Sorry to mess with you! Decided to change a few more things in the PR. It's mostly motivated by ADTests, because over there I need to run the AD and then catch the error to tell what went wrong (e.g. is it NaN or just outright wrong). The DPPL tests now use linked varinfo too. I think it's all looking better now.

sunxd3

In general, all make sense.

sunxd3 · 2025-04-16T09:13:24Z

src/test_utils/ad.jl

+   It will also perform _linking_, that is, the parameters in the VarInfo will
+   be transformed to unconstrained Euclidean space if they aren't already in
+   that space. Note that the act of linking may change the length of the
+   parameters. To disable linking, set `linked=false`.


I think this paragraph should be removed from this version?
And instead say something like "by default, we'll use linked (explaining what "link" means) varinfo..."

Oh, this is my bad. I messed with the interface a few times and forgot to update this.

Changed now!

sunxd3 · 2025-04-16T09:15:19Z

src/test_utils/ad.jl

+    value_atol::Real=1e-6,
+    grad_atol::Real=1e-6,


might as well be Float(even Float64)? my opinion is not strong here, just a mention.
Also could value_atol and grad_atol have different types?

Actually, yes, I think some of these Reals should be AbstractFloats? I think f64 might be a bit too restrictive, I guess it's pretty much always going to be f64 in regular usage, but since this is an exported interface I figured it should be generic.

src/test_utils/ad.jl

sunxd3

Thanks!

* Release 0.36 * AbstractPPL 0.11 + change prefixing behaviour (#830) * AbstractPPL 0.11; change prefixing behaviour * Use DynamicPPL.prefix rather than overloading * Remove VarInfo(VarInfo, params) (#870) * Unify `{untyped,typed}_{vector_,}varinfo` constructor functions (#879) * Unify {Untyped,Typed}{Vector,}VarInfo constructors * Update invocations * NTVarInfo * Fix tests * More fixes * Fixes * Fixes * Fixes * Use lowercase functions, don't deprecate VarInfo * Rewrite VarInfo docstring * Fix methods * Fix methods (really) * Link varinfo by default in AD testing utilities; make test suite run on linked varinfos (#890) * Link VarInfo by default * Tweak interface * Fix tests * Fix interface so that callers can inspect results * Document * Fix tests * Fix changelog * Test linked varinfos Closes #891 * Fix docstring + use AbstractFloat * Fix `condition` and `fix` in submodels (#892) * Fix conditioning in submodels * Simplify contextual_isassumption * Add documentation * Fix some tests * Add tests; fix a bunch of nested submodel issues * Fix fix as well * Fix doctests * Add unit tests for new functions * Add changelog entry * Update changelog Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> * Finish docs * Add a test for conditioning submodel via arguments * Clean new tests up a bit * Fix for VarNames with non-identity lenses * Apply suggestions from code review Co-authored-by: Markus Hauru <markus@mhauru.org> * Apply suggestions from code review * Make PrefixContext contain a varname rather than symbol (#896) --------- Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> Co-authored-by: Markus Hauru <markus@mhauru.org> --------- Co-authored-by: Markus Hauru <mhauru@turing.ac.uk> Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> Co-authored-by: Markus Hauru <markus@mhauru.org>

* Release 0.36 * AbstractPPL 0.11 + change prefixing behaviour (#830) * AbstractPPL 0.11; change prefixing behaviour * Use DynamicPPL.prefix rather than overloading * Remove VarInfo(VarInfo, params) (#870) * Unify `{untyped,typed}_{vector_,}varinfo` constructor functions (#879) * Unify {Untyped,Typed}{Vector,}VarInfo constructors * Update invocations * NTVarInfo * Fix tests * More fixes * Fixes * Fixes * Fixes * Use lowercase functions, don't deprecate VarInfo * Rewrite VarInfo docstring * Fix methods * Fix methods (really) * Draft of accumulators * Fix some variable names * Fix pointwise_logdensities, gut tilde_observe, remove resetlogp!! * Map rather than broadcast Co-authored-by: Tor Erlend Fjelde <tor.erlend95@gmail.com> * Start documenting accumulators * Use Val{symbols} instead of AccTypes to index * More documentation for accumulators * Link varinfo by default in AD testing utilities; make test suite run on linked varinfos (#890) * Link VarInfo by default * Tweak interface * Fix tests * Fix interface so that callers can inspect results * Document * Fix tests * Fix changelog * Test linked varinfos Closes #891 * Fix docstring + use AbstractFloat * Fix resetlogp!! and type stability for accumulators * Fix type rigidity of LogProbs and NumProduce * Fix uses of getlogp and other assorted issues * setaccs!! nicer interface and logdensity function fixes * Revert back to calling the macro @addlogprob! * Remove a dead test * Clarify a comment * Implement split/combine for PointwiseLogdensityAccumulator * Switch ThreadSafeVarInfo.accs_by_thread to be a tuple * Fix `condition` and `fix` in submodels (#892) * Fix conditioning in submodels * Simplify contextual_isassumption * Add documentation * Fix some tests * Add tests; fix a bunch of nested submodel issues * Fix fix as well * Fix doctests * Add unit tests for new functions * Add changelog entry * Update changelog Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> * Finish docs * Add a test for conditioning submodel via arguments * Clean new tests up a bit * Fix for VarNames with non-identity lenses * Apply suggestions from code review Co-authored-by: Markus Hauru <markus@mhauru.org> * Apply suggestions from code review * Make PrefixContext contain a varname rather than symbol (#896) --------- Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com> Co-authored-by: Markus Hauru <markus@mhauru.org> * Revert ThreadSafeVarInfo back to Vectors and fix some AD type casting in (Simple)VarInfo * Improve accumulator docs * Add test/accumulators.jl * Docs fixes * Various small fixes * Make DynamicTransformation not use accumulators other than LogPrior * Fix variable order and name of map_accumulator!! * Typo fixing * Small improvement to ThreadSafeVarInfo * Fix demo_dot_assume_observe_submodel prefixing * Typo fixing * Miscellaneous small fixes * HISTORY entry and more miscellanea * Add more tests for accumulators * Improve accumulators docstrings * Fix a typo * Expand HISTORY entry * Add accumulators to API docs * Remove unexported functions from API docs * Add NamedTuple methods for get/set/acclogp * Fix setlogp!! with single scalar to error * Export AbstractAccumulator, fix a docs typo * Apply suggestions from code review Co-authored-by: Penelope Yong <penelopeysm@gmail.com> * Rename LogPrior -> LogPriorAccumulator, and Likelihood and NumProduce * Type bound log prob accumulators with T<:Real * Add @addlogprior! and @addloglikelihood! * Apply suggestions from code review Co-authored-by: Penelope Yong <penelopeysm@gmail.com> * Move default accumulators to default_accumulators.jl * Fix some tests * Introduce default_accumulators() * Go back to only having @addlogprob! * Fix tilde_observe!! prefixing * Fix default_accumulators internal type * Make unflatten more type stable, and add a test for it * Always print all benchmark results * Move NumProduce VI functions to abstract_varinfo.jl --------- Co-authored-by: Penelope Yong <penelopeysm@gmail.com> Co-authored-by: Tor Erlend Fjelde <tor.erlend95@gmail.com> Co-authored-by: Hong Ge <3279477+yebai@users.noreply.github.com>

Link VarInfo by default

c5574ae

penelopeysm changed the base branch from main to breaking April 15, 2025 13:38

github-actions bot assigned penelopeysm Apr 15, 2025

penelopeysm requested a review from sunxd3 April 15, 2025 13:40

Tweak interface

6d163fd

Fix tests

f17300a

sunxd3 approved these changes Apr 15, 2025

View reviewed changes

test/ad.jl Outdated Show resolved Hide resolved

penelopeysm mentioned this pull request Apr 15, 2025

Test linked varinfos in DynamicPPL AD test suite #891

Closed

penelopeysm added 2 commits April 15, 2025 23:04

Fix interface so that callers can inspect results

c6c9595

Document

41c32a0

penelopeysm force-pushed the py/link-adtest branch from 84aa53c to 41c32a0 Compare April 15, 2025 22:04

penelopeysm added 2 commits April 15, 2025 23:23

Fix tests

3e6b2db

Fix changelog

51a26b2

penelopeysm force-pushed the py/link-adtest branch 2 times, most recently from 77faa53 to e37e6bb Compare April 15, 2025 23:16

penelopeysm changed the title ~~Link varinfo by default in AD testing utilities~~ Link varinfo by default in AD testing utilities; make test suite run on linked varinfos Apr 15, 2025

penelopeysm force-pushed the py/link-adtest branch 2 times, most recently from 1f4ec6f to dc04166 Compare April 15, 2025 23:24

Test linked varinfos

9bf0593

Closes #891

penelopeysm force-pushed the py/link-adtest branch from dc04166 to 9bf0593 Compare April 15, 2025 23:54

penelopeysm requested a review from sunxd3 April 16, 2025 00:19

sunxd3 requested changes Apr 16, 2025

View reviewed changes

Fix docstring + use AbstractFloat

976885c

penelopeysm requested a review from sunxd3 April 16, 2025 12:10

sunxd3 approved these changes Apr 16, 2025

View reviewed changes

penelopeysm merged commit be27636 into breaking Apr 16, 2025
17 of 18 checks passed

penelopeysm deleted the py/link-adtest branch April 16, 2025 12:28

This was referenced Apr 16, 2025

Release 0.36 #829

Merged

Should NaN's display as errors? TuringLang/ADTests#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Link varinfo by default in AD testing utilities; make test suite run on linked varinfos #890

Link varinfo by default in AD testing utilities; make test suite run on linked varinfos #890

Uh oh!

penelopeysm commented Apr 15, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 15, 2025 •

edited

Loading

Uh oh!

codecov bot commented Apr 15, 2025 •

edited

Loading

Uh oh!

sunxd3 left a comment

Uh oh!

Uh oh!

penelopeysm commented Apr 15, 2025

Uh oh!

penelopeysm commented Apr 16, 2025 •

edited

Loading

Uh oh!

sunxd3 left a comment

Uh oh!

sunxd3 Apr 16, 2025

Uh oh!

penelopeysm Apr 16, 2025

Uh oh!

penelopeysm Apr 16, 2025

Uh oh!

sunxd3 Apr 16, 2025

Uh oh!

penelopeysm Apr 16, 2025 •

edited

Loading

Uh oh!

Uh oh!

sunxd3 left a comment

Uh oh!

Uh oh!

Uh oh!

Link varinfo by default in AD testing utilities; make test suite run on linked varinfos #890

Link varinfo by default in AD testing utilities; make test suite run on linked varinfos #890

Uh oh!

Conversation

penelopeysm commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark Report for Commit 976885c

Computer Information

Benchmark Results

Uh oh!

codecov bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sunxd3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

penelopeysm commented Apr 15, 2025

Uh oh!

penelopeysm commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunxd3 left a comment

Choose a reason for hiding this comment

Uh oh!

sunxd3 Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

penelopeysm Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

penelopeysm Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

sunxd3 Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

penelopeysm Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sunxd3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

penelopeysm commented Apr 15, 2025 •

edited

Loading

github-actions bot commented Apr 15, 2025 •

edited

Loading

Benchmark Report for Commit `976885c`

codecov bot commented Apr 15, 2025 •

edited

Loading

penelopeysm commented Apr 16, 2025 •

edited

Loading

penelopeysm Apr 16, 2025 •

edited

Loading