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 <[email protected]> * 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 <[email protected]> * Apply suggestions from code review * Make PrefixContext contain a varname rather than symbol (#896) --------- Co-authored-by: Hong Ge <[email protected]> Co-authored-by: Markus Hauru <[email protected]> --------- Co-authored-by: Markus Hauru <[email protected]> Co-authored-by: Hong Ge <[email protected]> Co-authored-by: Markus Hauru <[email protected]>

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

penelopeysm commented Apr 15, 2025 •

edited

Loading

github-actions bot commented Apr 15, 2025 •

edited

Loading

codecov bot commented Apr 15, 2025 •

edited

Loading

sunxd3 left a comment

penelopeysm commented Apr 15, 2025

penelopeysm commented Apr 16, 2025 •

edited

Loading

sunxd3 left a comment

sunxd3 Apr 16, 2025

penelopeysm Apr 16, 2025

penelopeysm Apr 16, 2025

sunxd3 Apr 16, 2025

penelopeysm Apr 16, 2025 •

edited

Loading

sunxd3 left a comment

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

Conversation

penelopeysm commented Apr 15, 2025 • edited Loading

github-actions bot commented Apr 15, 2025 • edited Loading

Benchmark Report for Commit 976885c

Computer Information

Benchmark Results

codecov bot commented Apr 15, 2025 • edited Loading

Codecov Report

sunxd3 left a comment

Choose a reason for hiding this comment

penelopeysm commented Apr 15, 2025

penelopeysm commented Apr 16, 2025 • edited Loading

sunxd3 left a comment

Choose a reason for hiding this comment

sunxd3 Apr 16, 2025

Choose a reason for hiding this comment

penelopeysm Apr 16, 2025

Choose a reason for hiding this comment

penelopeysm Apr 16, 2025

Choose a reason for hiding this comment

sunxd3 Apr 16, 2025

Choose a reason for hiding this comment

penelopeysm Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

sunxd3 left a comment

Choose a reason for hiding this comment

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