Closed st-- closed 2 years ago
One thing I notice is that the approx_lml function isn't tested, which you point to in the other PR. Should this at least be checked?
Yeah, we should have that too... maybe later? for now I just wanted to take out the non-EP changes from #64.
Re. my comment about separating out the API from the ability to provide good approximate inference, should we also consider including applying Zygote to
approx_lml
as part of this test suite somewhere? (I'm not committed to requiring this though).
Testing gradients should probably be there, too ,but also separately (e.g. current EP doesn't have approx_lml implemented, and gettings its gradients right would be another headache on top)
Happy to leave these suggestions for another PR. Please ping me again when you want me to take another look -- I think it's pretty close to being good to go.
@willtebbutt ping
Merging #117 (553550f) into master (6a5877f) will decrease coverage by
0.00%
. The diff coverage is94.11%
.
@@ Coverage Diff @@
## master #117 +/- ##
==========================================
- Coverage 94.13% 94.13% -0.01%
==========================================
Files 4 5 +1
Lines 290 324 +34
==========================================
+ Hits 273 305 +32
- Misses 17 19 +2
Impacted Files | Coverage Δ | |
---|---|---|
src/TestUtils.jl | 94.11% <94.11%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 6a5877f...553550f. Read the comment docs.
To make it easier to test various approximations (similar to AbstractGPs.TestUtils)