Skip to content
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

create ApproximateGPs.TestUtils #117

Merged
merged 17 commits into from
Mar 17, 2022
Merged

create ApproximateGPs.TestUtils #117

merged 17 commits into from
Mar 17, 2022

Conversation

st--
Copy link
Member

@st-- st-- commented Mar 15, 2022

To make it easier to test various approximations (similar to AbstractGPs.TestUtils)

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

Thanks for making some headway on this -- it will be good to have this formalised a bit more. Hopefully it will let us move towards a better understanding of scope / limitations of this particular package.

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?

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

src/TestUtils.jl Outdated Show resolved Hide resolved
src/TestUtils.jl Show resolved Hide resolved
@st--
Copy link
Member Author

st-- commented Mar 15, 2022

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)

@willtebbutt
Copy link
Member

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.

@st--
Copy link
Member Author

st-- commented Mar 16, 2022

@willtebbutt ping

@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #117 (553550f) into master (6a5877f) will decrease coverage by 0.00%.
The diff coverage is 94.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.

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

LGTM subject to formatting suggestion

didn't think; this won't work

This reverts commit 37a541e.
st-- and others added 3 commits March 16, 2022 17:07
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@st-- st-- merged commit fb3f410 into master Mar 17, 2022
@st-- st-- deleted the st/testutils branch March 17, 2022 13:07
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.

2 participants