-
Notifications
You must be signed in to change notification settings - Fork 19
Split AD tests into workflows #190
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
Conversation
|
AdvancedVI.jl documentation for PR #190 is available at: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Benchmark Results
| Benchmark suite | Current: 5928201 | Previous: e801817 | Ratio |
|---|---|---|---|
normal/RepGradELBO + STL/meanfield/Zygote |
3869106797 ns |
4047146731.5 ns |
0.96 |
normal/RepGradELBO + STL/meanfield/ReverseDiff |
1128292103 ns |
1118748915 ns |
1.01 |
normal/RepGradELBO + STL/meanfield/Mooncake |
1190705399 ns |
1241121652 ns |
0.96 |
normal/RepGradELBO + STL/fullrank/Zygote |
3955271108 ns |
4002369352.5 ns |
0.99 |
normal/RepGradELBO + STL/fullrank/ReverseDiff |
1676757225 ns |
1619611799 ns |
1.04 |
normal/RepGradELBO + STL/fullrank/Mooncake |
1281361888 ns |
1285127945.5 ns |
1.00 |
normal/RepGradELBO/meanfield/Zygote |
2767657958 ns |
2834638479.5 ns |
0.98 |
normal/RepGradELBO/meanfield/ReverseDiff |
788761749 ns |
788049327 ns |
1.00 |
normal/RepGradELBO/meanfield/Mooncake |
1062465846 ns |
1093464215 ns |
0.97 |
normal/RepGradELBO/fullrank/Zygote |
2785921421.5 ns |
2873772655.5 ns |
0.97 |
normal/RepGradELBO/fullrank/ReverseDiff |
972573810 ns |
956640380.5 ns |
1.02 |
normal/RepGradELBO/fullrank/Mooncake |
1118695995 ns |
1161037456 ns |
0.96 |
normal + bijector/RepGradELBO + STL/meanfield/Zygote |
5484603213 ns |
5705749184 ns |
0.96 |
normal + bijector/RepGradELBO + STL/meanfield/ReverseDiff |
2380715424 ns |
2472318329 ns |
0.96 |
normal + bijector/RepGradELBO + STL/meanfield/Mooncake |
4098330520.5 ns |
4249849133.5 ns |
0.96 |
normal + bijector/RepGradELBO + STL/fullrank/Zygote |
5615396878 ns |
5732121704 ns |
0.98 |
normal + bijector/RepGradELBO + STL/fullrank/ReverseDiff |
3047981874.5 ns |
3070250822 ns |
0.99 |
normal + bijector/RepGradELBO + STL/fullrank/Mooncake |
4210618967 ns |
4377541897 ns |
0.96 |
normal + bijector/RepGradELBO/meanfield/Zygote |
4309224236 ns |
4461833085.5 ns |
0.97 |
normal + bijector/RepGradELBO/meanfield/ReverseDiff |
2030629168 ns |
2088736319 ns |
0.97 |
normal + bijector/RepGradELBO/meanfield/Mooncake |
3930307061 ns |
4122678327.5 ns |
0.95 |
normal + bijector/RepGradELBO/fullrank/Zygote |
4353748197 ns |
4571360982 ns |
0.95 |
normal + bijector/RepGradELBO/fullrank/ReverseDiff |
2295184083 ns |
2345239376 ns |
0.98 |
normal + bijector/RepGradELBO/fullrank/Mooncake |
4071153970.5 ns |
4217237241 ns |
0.97 |
This comment was automatically generated by workflow using github-action-benchmark.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
=======================================
Coverage ? 88.98%
=======================================
Files ? 18
Lines ? 445
Branches ? 0
=======================================
Hits ? 396
Misses ? 49
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
test/runtests.jl
Outdated
| if GROUP == "All" || GROUP == "General" || GROUP == "AD" | ||
| if GROUP == "AD" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this setup feels a bit complex. Could you just have two different tests: those which use AD and those which don't? In other words:
- turn 'General' into 'AD' and maybe just rename it ReverseDiff.yml
- group the
familiesandparamspacesgdtests together
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree. I actually made things simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should go back to ReTest since it seems to be a little better maintained than before. The test filtering feature there is really useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite agnostic towards frameworks like ReTest. If you think it's useful I wouldn't be opposed to it. I've been trying to use https://github.com/theogf/TestPicker.jl recently which (I think) does similar things too with test filtering
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good :)
This PR separates the AD backend integration tests into their own workflow. This has the following benefit:
All AD integration tests invoke
.github/workflows/AutoDiffIntegration.yml, which implements GitHub's reusable workflows feature.