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

File Structure #75

Closed
willtebbutt opened this issue Nov 9, 2021 · 3 comments
Closed

File Structure #75

willtebbutt opened this issue Nov 9, 2021 · 3 comments

Comments

@willtebbutt
Copy link
Member

There are a number of file / code structure things that I think we need to address.

I'm keen to follow our KernelFunctions test style in this package, because it makes it easy to predict roughly where stuff is tested, and to avoid near-circular dependencies in testing.

Specific things I think we need to change:

  1. I can't see a particuarly good reason to have the elbo-related code separate from the sparse_variational code, and it makes testing a bit of a headache, because you wind up having to instantiate SparseVariationalApproximations more times than would be ideal.
  2. I would like the consistency tests be located with the code that they're testing ie. inside test/sparse_variational.jl, so that it's easy to see whats' going on.

Therefore I propose to

  1. add KernelFunctions test style guide to test/runtests.jl
  2. move code from src/elbo.jl into src/sparse_variational.jl, and remove src/elbo.jl
  3. move tests from test/elbo.jl into test/sparse_variational.jl and remove test/elbo.jl
  4. move tests from test/equivalences.jl inside test/sparse_variational.jl.

@rossviljoen does this seem reasonable to you? Were there particular ways to structure things in this way that I'm missing?

@rossviljoen
Copy link
Collaborator

That seems eminently reasonable! I think the only reason I put equivalences in a different file was because I thought it might be too heavy to run all the time - but that's a nonissue so it makes sense to put them all in the same place

@rossviljoen
Copy link
Collaborator

rossviljoen commented Nov 9, 2021

I'll do this in a PR along with #66

Edit: Never mind, better to do them separately

@rossviljoen
Copy link
Collaborator

Closed by #78

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

No branches or pull requests

2 participants