Split code into submodules for the different approximations

[st--]

New submodules:

• API: common interface
• SparseVariationalApproximationModule
• LaplaceApproximationModule

This does not change the exports of ApproximateGPs.jl, and hence users should not have to actually change their code.

NOTE: users of this package should only use the exported symbols.
Functions &c inside the submodules are liable to change, and are not part of the public interface!

---

Example implementation of #90. I'm actually happy with how it turned out, so changed it to "ready for review", let's continue the discussion until we find something we can all settle on:)

(Side note - ignore any of the code around expected_loglik, that should really move into GPLikelihoods.jl anyways.)

[st--]

I really like how this makes clear where the function is coming from.

[codecov]

Codecov Report

Merging #96 (11b186b) into master (bae0a1e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #96   +/-   ##
=======================================
  Coverage   95.78%   95.78%           
=======================================
  Files           4        4           
  Lines         285      285           
=======================================
  Hits          273      273           
  Misses         12       12           

Impacted Files	Coverage Δ
src/SparseVariationalApproximationModule.jl	96.20% <ø> (ø)
src/expected_loglik.jl	95.34% <ø> (ø)
src/utils.jl	42.85% <ø> (ø)
src/LaplaceApproximationModule.jl	98.07% <100.00%> (ø)

---

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 bae0a1e...11b186b. Read the comment docs.

[theogf]

Two main comments if we go in this direction:

• Can LaplaceApproximationModule can be shortened? I think LaplaceModule is still understandable
• Why put SparseVariationalApproximation in a sub-module? I can see the point of having each approach in a different sub-module but SVA is not an approach in itself

[st--]

• Can LaplaceApproximationModule can be shortened? I think LaplaceModule is still understandable

Yes, could do, but wouldn't work quite the same for ExpectationPropagation; it could be EPModule, but I found it cleaner to have the module name follow the name of the approximation struct. You're not

• Why put SparseVariationalApproximation in a sub-module? I can see the point of having each approach in a different sub-module but SVA is not an approach in itself

I'm not sure what you mean... ?

[theogf]

I'm not sure what you mean... ?

I don't understand the point of having SparseVariationalApproximationModule.

[st--]

I don't understand the point of having SparseVariationalApproximationModule.

For the same reason we've got LaplaceApproximationModule - to separate the different subcomponents of the ApproximateGPs package. No?

(In a sense we might just want them all to be their own packages, which the submodules would prepare us for later down the road, but right now it's probably not worth it.)

[theogf]

For the same reason we've got LaplaceApproximationModule - to separate the different subcomponents of the ApproximateGPs package. No?

Yeah but that's not the argument you raised initially. It was more about conflicting naming between methods EP and Laplace for example. But SVA is basically the common core of the package used by the rest of the methods, it's not "just another solving algorithm", that's why I would not put it in a module. My proposal would be instead to have all the core common objects in the main module and every algorithm Laplace, EP and more in their own module.

[st--]

For the same reason we've got LaplaceApproximationModule - to separate the different subcomponents of the ApproximateGPs package. No?

Yeah but that's not the argument you raised initially.

It was part of the argument I was trying to make! 😅

But SVA is basically the common core of the package used by the rest of the methods, it's not "just another solving algorithm", that's why I would not put it in a module.

I disagree both with the premise and the conclusion...
Contra premise: Laplace doesn't use SVA, and EP shouldn't use SVA (it'd be more numerically stable and faster to implement it making use of the specific EP structure). I'm not convinced what would make SVA more than "just another approximation algorithm".
Contra conclusion: accepting that EP depends on SVA, it should mainly depend on it in terms of the public API - which is easily handled by using .ApproximateGPs: SparseVariationalApproximation. What would we gain from having all the SVA internals within the "umbrella" package?

[theogf]

Contra premise: Laplace doesn't use SVA, and EP shouldn't use SVA (it'd be more numerically stable and faster to implement it making use of the specific EP structure). I'm not convinced what would make SVA more than "just another approximation algorithm".

Sorry, I was somehow convinced that SVA was a core element and was used by everything else. Than, all you said makes sense

[st--]

The only potential downside I would see is that it might look a bit weirder to see the fully-qualified names in the API docs (see https://juliagaussianprocesses.github.io/ApproximateGPs.jl/previews/PR96/api/)

[st--]

On the other hand, that keeps related functions next to each other, and perhaps it would actually be more helpful to have different sub-pages for each of the approximations?

[theogf]

I really like the idea of having one page per approximation.

[st--]

Ah that comes from :
If strict=true (or strict=:missing_docs or strict=[:missing_docs, ...]) is also set then the build will fail if any missing docstrings are encountered.

I wish it would just give a meaningful error message instead of just saying "error" (and not even hinting that you might want to look at warnings)...

Update: I've opened an issue JuliaDocs/Documenter.jl#1756

[willtebbutt]

lgtm

[st--]

Note: @reexport also exports the submodule itself! This is not what we want. The only solution I could come up with is to explicitly qualify the @reexport using .Foo: bar, baz.

[st--]

One thing I don't like about this yet is that it took away all the docstrings for methods-from-other-packages (such as AbstractGPs.posterior) defined for the types of ApproximateGPs. I'm not quite sure why that would be...

[theogf]

Wait you cannot access them from the REPL or from Documenter?

[st--]

Wait you cannot access them from the REPL or from Documenter?

From Documenter. Something about how it discovers functions? Maybe because I changed to Private = true? 🤔

It all works fine from the REPL (see tests, notebooks passing)!